Skip to content

Gui settings optimizations #1252

Merged
merged 4 commits into from Oct 7, 2012

5 participants

@Karlson2k
Team Kodi member

When XBMC has several skins, than it has a thousandths of settings.
I removed ".ToLower()" from settings search as it can take up to 75% CPU time when searching for setting.

GUISettings is used very frequently, so it's important to have fast retrieval of setting.

@MartijnKaijser
Team Kodi member

IIRC we wanted to move away from one guisettings.xml and make it just like add-ons now work. So on settings.xml for each skin

@Karlson2k
Team Kodi member

Think that's right idea.
But anyway optimization does matter because GUISettings is used everywhere. So it'll be useful, specially on low-powerful platform, like Android.

@jmarshallnz
Team Kodi member
  1. Skin settings don't use these routines at all, so all you're doing is speeding up XBMC's normal settings, of which there isn't that many anyway. Further, they shouldn't be used all that often anyway (i.e. shouldn't be used every frame like skin settings are).

  2. CStdString vs basic_string will make bugger all difference. With that said, using std::string seems more sensible either way.

  3. std::unordered_map is Cxx11 (or TR1), everything else in XBMC is not. Thus, IMO this is not acceptable unless we've guaranteed that we're not using a compiler that won't have this feature.

  4. std::unordered vs std::map may not offer much advantage in the case of the order of 100 elements - it would depend on the hashing speed compared to the number of compares otherwise needed (log(n)).

@jmarshallnz
Team Kodi member

Oh, and it wasn't obvious, but +1 on the removal of ToLower. Just drop 050e826

@Karlson2k
Team Kodi member

@jmarshallnz 1. Agree. But what drawback of optimization, even if it isn't highly used? :)
2. std::string should be equal to std::basic_string, but std::basic_string is explicitly used as parent class for CStdString so it should be more compatible (does not depend on library implementation). Using std::string (or basic string) as a key is definitely faster as comparison of keys is faster and one-time key objects creation is faster (when using "char *" for search).
3. Both MSVC and GCC has unordered_map. They both also has hash_map, which if even a little bit faster (from my tests). Mat be we should use hash_map?
4. In real situation on Win platform std::unordered_map is faster. But I can agree, that it could change after migrating Skin settings from GUISettings. Or it wouldn't change. :) We could test.

@ghost
ghost commented Aug 7, 2012

requiring c++-11 is an absolute NO GO at this point in time. try again in 5 years or so. while indeed newer gcc support it, we build on tons of platforms and not all of those have updated toolchains. plus some of them use clang.

@Karlson2k
Team Kodi member

@jmarshallnz Done.
Is it worth creating separate PR for map with std::string as key?

@Karlson2k
Team Kodi member

@cptspiff We have hash_map as alternative.

@ghost
ghost commented Aug 7, 2012

hash_map is not part of the stl..

@Karlson2k
Team Kodi member

@cptspiff Agree. But it's included everywhere for years.
And unordered_map is included too, but OK, if we want to make it compiler-compatible as much as possible, I removed it already from this PR.

@ghost
ghost commented Aug 7, 2012

yeah, it's probably a usable candidate.

@Karlson2k
Team Kodi member

@cptspiff Did you mean 'hash_map' or this PR? :)
By the way, GUISetting counts now 223 setting.

@ghost
ghost commented Aug 7, 2012

i meant hash_map can be used to make this pr a usable candidate for inclusion :)

i agree, if we can optimize, we should.

@Karlson2k
Team Kodi member

I decided to do as @jmarshallnz recommends.
I removed 'map' change from this PR (as it contains other changes too).

I'll create a separate PR for 'hash_map', but it's not so straightforward as on MS it's in stdext namespace, on GCC it's in other namespace.
As I see for MacOS, iOS and Android we use GCC too, but anyway it can be complicated.

@jmarshallnz
Team Kodi member

In that case hash_map is a no-go. With 250-odd items in the map I doubt it'd be much faster anyway, though that would depend how good the hash is, ofcourse.

As for whether CStdString or std::string should be used, a std::string would be preferred - again, there wouldn't be much (if any) measureable performance difference between the two (one is a wrapper for the other).

@Karlson2k
Team Kodi member

@jmarshallnz Already excluded all 'map's from this PR. :)

About CStdString - yes, it's just a wrapper, but it wrap not only creation of object when search begins, but it wrap every comparison during key search.
From my tests - using of std::string is lower time less than or about 10%.

@davilla
davilla commented Aug 7, 2012

I'm for replacing CStdString usage with std::string. I think the one thing missing in std::string and that's CStdString.format.

@Karlson2k
Team Kodi member

@jmarshallnz, @cptspiff Now this PR drop only .ToLower() transformation.
It's ready to go!

@jmarshallnz
Team Kodi member

There's a bug introduced here, in that skins can query boolean settings via System.GetBool(). Switching to lowercase only will mean you need to lowercase that string (GUIInfoManager.cpp) as well.

There may well be other things that access similarly - it would be worth reviewing all calls to g_guiSettings.Get*() to make sure nothing breaks.

@jmarshallnz
Team Kodi member

You'll either want to drop 7aa45a2 from this one or #1253.

@Karlson2k
Team Kodi member

@jmarshallnz if one of them will be merged, I'll remove it from second one.
Currently seems that this one have more chances to be merged.

@Karlson2k
Team Kodi member

@jmarshallnz Decided to drop 7aa45a2 as this PR can go without it.

@jmarshallnz
Team Kodi member

You need to address the comment above first:

#1252 (comment)

@Karlson2k
Team Kodi member

@jmarshallnz Yes, I missed this one.
Already done several times with "Confluense", "Touched" and some other skins.
As all get/set function write message in case of "not found" by "CLog::Log(LOGDEBUG,"Error: Requested setting (%s) was not found. It must be case-sensitive", strSetting);", all possible errors can be easily detected.

@jmarshallnz
Team Kodi member

The much better option, given that little other values in the skin XMLs are case-sensitive, is to ToLower() it in GUIInfoManager.cpp. It might be possible to do this when you place the string into the lookup vector when parsing the info label to save it being done every frame. You'll also want to check any other usage of this. Examples would be HTTP-API (not really a high priority as it will be gone for Frodo), JSON-RPC, and python.

@Karlson2k
Team Kodi member

JSON-RPC and python already superficially checked by simply using several addons and JSON control app.
I'll look into GUIInfoManager.cpp.
But if settings supposed to be lowercase and at first and second looks all work just fine, should we introduce any additional hacks for potentially wrong designed addons, skins and anything other components?
Isn't it better to use right design?

@jmarshallnz
Team Kodi member

All infolabels are case-insensitive. You've just made one that is case sensitive. Thus, you've broken the design.

Whether some add-ons and some clients of JSON-RPC work or not, we need to verify that all will work by checking it forces case-sensitivity, or makes it case-insensitive at the source.

@jmarshallnz
Team Kodi member

@Karlson2k if you update infolabels and possibly also add-ons (i.e. the python usage of the api) then I have no problem with this going in.

@Karlson2k
Team Kodi member

@jmarshallnz OK, I'll look into it.

@Karlson2k
Team Kodi member

@jmarshallnz Found http-api use of g_guiSettings.
Could you point me to python and infolabel usage of g_guiSettings?

@topfs2
Team Kodi member
topfs2 commented Sep 3, 2012

Btw, just need to state this even if its removed from the pull request anyways :). A hashmap trades performance for memory. For a hashmap to be roughly o(1) it needs to use roughly twice the amount of memory. So it all depends on what is preferred, i.e. one should only use hashmaps if the memory increase is negligable compared to the get/set speed, and you have memory to spare

@jmarshallnz
Team Kodi member

@Karlson2k Grep for SYSTEM_GET_BOOL in GUIInfoManager.cpp. You'd ideally ToLower() the passed in param when parsing system.getbool (just drop it from the lookup table and add the case in the same place system.memory et. al. are).

@Karlson2k
Team Kodi member

@jmarshallnz Is 1e73614 fine?

@jmarshallnz
Team Kodi member

It's OK, but it would be much nicer if it wasn't done per-frame. To do this, add something like this further up:

if (prop.name == "getbool")
{ // settings use lowercase keys
return AddMultiInfo(GUIInfo(system_param[i].val, ConditionalStringParameter(StringUtils::ToLower(prop.param), true)));
}

then remove getbool from the system_param lookup table above.

@Karlson2k
Team Kodi member

@jmarshallnz Think that 2a5236f is even better solution.

@jmarshallnz
Team Kodi member

No, that won't work. Some parameters are case sensitive.

@Karlson2k
Team Kodi member

OK, I'll fix it.

@Karlson2k
Team Kodi member

@jmarshallnz Finally. Remove commented out getbool from system_param?

@jmarshallnz
Team Kodi member

Yeah, remove from the map is fine.

@Karlson2k
Team Kodi member

@jmarshallnz 123b567 is fine?

@jmarshallnz
Team Kodi member

Looks fine, yes. Thanks :)

@jmarshallnz jmarshallnz commented on an outdated diff Oct 7, 2012
xbmc/settings/GUISettings.cpp
if (it != settingsMap.end())
{ // old category
return ((CSettingBool*)(*it).second)->GetData();
}
// Backward compatibility (skins use this setting)
- if (lower == "lookandfeel.enablemouse")
+ if (strcmp(strSetting,"lookandfeel.enablemouse") == 0)
@jmarshallnz
Team Kodi member
jmarshallnz added a note Oct 7, 2012

Please use strncmp for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Karlson2k added some commits Aug 6, 2012
@Karlson2k Karlson2k Make SetXXX methods case-sensitive.
This will make key search faster by 50%-75%.
This also can potentially break some bad-designed code, but in my test all is working fine.
4ddbc49
@Karlson2k Karlson2k Make GetXXX methods case-sensitive (as described in code).
This will make search faster by 50%-75%.
This also can potentially break some bad-designed code, but in my test all is working fine.
cd9e237
@Karlson2k Karlson2k Make sure that http-api requests settings from g_guiSettings by lower…
… case names
7fa5c60
@Karlson2k Karlson2k GUIInfoManager: retrieve SYSTEM_GET_BOOL by lower case cecdaa0
@Karlson2k
Team Kodi member

@jmarshallnz Corrected.

@jmarshallnz jmarshallnz was assigned Oct 7, 2012
@jmarshallnz jmarshallnz merged commit d95fd3b into xbmc:master Oct 7, 2012
@jmarshallnz
Team Kodi member

Thanks!

@Karlson2k Karlson2k deleted the Karlson2k:GUI_Settings_optimization branch Oct 15, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.