Use system_properties.h api instead of getprop #1233
Conversation
|
CI gfxreconstruct build queued with queue ID 28144. |
|
CI gfxreconstruct build # 3105 running. |
|
CI gfxreconstruct build # 3105 passed. |
panos-lunarg
force-pushed
the
panos_use_system_properties_api_instead_of_getprop
branch
from
00c0c86
to
c65ae20
Compare
|
CI gfxreconstruct build queued with queue ID 28300. |
|
CI gfxreconstruct build # 3107 running. |
|
CI gfxreconstruct build # 3107 passed. |
panos-lunarg
force-pushed
the
panos_use_system_properties_api_instead_of_getprop
branch
from
c65ae20
to
cf62b76
Compare
|
CI gfxreconstruct build queued with queue ID 28667. |
|
CI gfxreconstruct build # 3112 running. |
|
CI gfxreconstruct build # 3112 passed. |
framework/util/platform.h
Outdated
| static const prop_info* GetAndroidPropertyInfo(const char* name) | ||
| { | ||
| // According to system_properties.h "Property lookup is expensive, so it can be useful to cache the result of this | ||
| // function.". For that reason we cache property infos. |
There was a problem hiding this comment.
I saw that comment too, how much overhead is it? Was not sure it is worth the added complexity.
framework/util/platform.h
Outdated
| const std::string prop_name = std::string(name); | ||
| const prop_info* pi; | ||
|
|
||
| auto prop_name_info = property_name_info_cache.find(prop_name); |
There was a problem hiding this comment.
is this a problem in respect to multi-threading, or is this code never used concurrently?
There was a problem hiding this comment.
Right, I'd better add some lock here
framework/util/platform.h
Outdated
| std::string command = "getprop "; | ||
| command += name; | ||
| // Note: GetAndroidPropertyInfo caches property info look ups. That means if a property is not set | ||
| // during start up, setting it or changing its value during run time will not be noticed and won't have any effect. |
There was a problem hiding this comment.
that's fine with our usage. But not sure other users wont run into this. For example we did have an issue where the property was not set in the initial run, and that caused issues of differently changing the output name. For that reason we now set the property before we start the app, so no problem for us. But this is a bit of a trap I could see others running into.
There was a problem hiding this comment.
We discussed this at length internally and this comment was the result of the conversation. Otherwise if we want to pick up at frame time the values of properties that were unset at startup we'd have the impact of __system_property_find every frame, at least until it wasn't nullptr. I also don't know (as you asked above) how much performance impact there is here. I gathered from Panos that he was just honoring the guidance in system_properties.h.
There was a problem hiding this comment.
Yeah I didn't measure performance impact, I just followed the hint. I'll do some measurement to get a rough idea
There was a problem hiding this comment.
Did some measurement and I didn't see any difference. I'll drop the cache for simplicity's sake
Use the appropriate Android NDK Api to get property values instead of
launching a new process with popen("getprop ...");
panos-lunarg
force-pushed
the
panos_use_system_properties_api_instead_of_getprop
branch
from
cf62b76
to
4fe54f0
Compare
|
CI gfxreconstruct build queued with queue ID 29329. |
|
CI gfxreconstruct build # 3124 running. |
|
CI gfxreconstruct build # 3124 passed. |
Use the appropriate Android NDK Api to get property values instead of launching a new process with popen("getprop ...");
Fixes #1231