Add selective caching of infobools in list items

[bavison]

As noted in a TODO comment attached to CGUIInfoManager::GetBoolValue, caching
of infobools (both expression and single bools) is disabled whenever a
CGUIListItem pointer is provided. This is true whenever visibility is
evaluated for a control which is part of a list item (which is responsible
for a large portion of the controls in many windows), which obviously hurts
caceh efficiency. However, many of the bools (including the leaf nodes of
expression infobools) aren't actually dependent upon which list item they're
attached to.

This patch adds a flag to each InfoBool object to indicate whether its value
depends upon the list item; if this isn't set, then the cache is re-enabled
even if a CGUIListItem pointer is provided. The flag is calculated at parse
time because it's quite tricky to evaluate - it applies to bools in any of
the following categories:

• a statically definied boolean condition in the range LISTITEM_START to
  LISTITEM_END - for example, "ListItem.IsResumable"
• a (dynamically allocated) multi-info bool, that is to say, the result of a
  string or integer operation on a list item attribute - for example
  "substring(Listitem.mpaa,Rated R)"
• an expression that uses one or more of the above - for example
  "Window.IsVisible(Videos) + !ListItem.IsResumable"

I measured the effect while the Videos window of the default skin was open
(but idle) on a Raspberry Pi, and this reduced the CPU usage by 3.2% from
45.1% to 41.9%:

          Before          After
          Mean   StdDev   Mean   StdDev  Confidence  Change
IdleCPU%  45.1   1.2      41.9   0.5     100.0%      +7.7%

[jmarshallnz]

While I like the basic idea, the implementation is a bit messy, mostly due to the passing of information between CGUIInfoManager and InfoSingle/Expression. That we have the infomanager include in the infobool header suggests some cleanup may be required.

The idea was that eventually the InfoBool pointer would be returned on Register(), so that the call from controls (and from InfoExpression) would be done directly rather than holding these within the infomanager. Shared pointers being returned would guard against them being pulled out from underneath those controls on clearing of the infomanager (I note that technically there's no guard against this atm - while we check the integer range, we presume that the integer points to the same infobool as it did on Register).

If this was done, I think it would allow the dependency in the InfoBool header to be removed?

[bavison]

I've changed the return type on CGUIInfoManager::Register as you suggested. As you'll see, it opened rather a can of worms, and a very large number of files ended up needing changes. It might be cleaner as a separate patch, which the listitem-independent cache patch would then depend upon, but for now I've tacked it on the end of the same branch so you can say if it's what you had in mind.

I've also removed the bool * parameter which I previously added to CGUIInfoManager::Register; because it now returns an InfoBool * rather than an integer ID, InfoExpression::Parse is able to inspect the node's InfoBool::m_listItemDependent member directly.

You might notice that I've changed InfoBool::m_listItemDependent from being a protected member to a public one. I found I got errors otherwise (presuambly because although InfoExpression derives from InfoBool, I was accessing the member from within a different object?)

[jmarshallnz]

That was the can of worms I was expecting :)

The nice thing about doing it this way is it means we could just do info->Get() later on (we'd need to take care of the updatetime ofc). With a virtual baseclass it goes someway to removing the dependency on the infomanager from guilib. Further, we could look in the future at returning info-specific InfoSingle's: e.g. one that deals only with ListItem.* for example, making it more efficient as there's no need for gigantic switch or ifelse jumping through multiple functions.

I think the main problem with the approach is ownership of them pointers. ATM CGUIInfoManager owns their lifespan, but none of the objects that are getting them from the infomanager know about that necessarily. As things stand, as far as I can tell only the m_xmlIncludeConditions are longer lived than the infomanager - and only because it's not being cleared on window onload, which could be changed. However, it's feasible that some other classes might at some point wish to hold InfoBool pointers and for those to extend over skin reload. IIRC any other classes use EvaluateBool ATM.

If we were concerned about this, using shared_ptr's might be the way to go - we wouldn't need to be concerned about lifetime, though it would mean things are a little messier include-wise (i.e. we'd probably need to include a file in the headers - atm we could forward-declare), though that include could be lightweight (definition of baseclass + typedef).

Any thoughts/comments/criticisms would be most appreciated.

[bavison]

Well, I already had to insert #include "interfaces/info/InfoBool.h" in a few places to make this version compile, so a simple approach would be to insert

#include <boost/shared_ptr.h>
typedef boost::shared_ptr<InfoBool> InfoBoolPtr;

into InfoBool.h, and use InfoBoolPtr in place of InfoBool *.

As I understand it, the advantage of using CGUIInfoManager::Register is that it enables the de-duplication of logically equivalent InfoBool objects. This is something that a shared_ptr can't do by itself - it just reference-counts pointers to objects with the same address, not objects that compare equal themselves.

If you want to use de-duplicated InfoBool objects whose lifetime is indefinite (rather than ones that are flushed on reload), isn't the correct approach to use a separate static CGUIInfoManager object?

[jmarshallnz]

You don't actually need the includes - you could forward define them and put the include in the .cpp instead, but that's a minor point - and indeed, with shared_ptr you could still forward define them, though using a light header with the appropriate include for shared_ptr and the typedef would be cleaner.

Regarding de-duplicates, yes, CGUIInfoManager handles that, and that is essential to the optimisation - before the register stuff was introduced, every instance of an infobool was evaluated separately: Now at least they're done only once a frame (which is still way too frequent, but at least there's an upper bound now!)

What shared_ptrs give you is shared ownership. i.e. if CGUIInfoManager clears out it's list, any others that still hold a pointer can still dereference it without issue.

Now, CGUIInfoManager is global already, so we have a single object, so perhaps needn't actually clear out the info bools during skin unload anyway? Indeed, it would be slightly inefficient to do so - if we have dangling shared_ptr's that are actually called, then having those around would mean we potentially have duplicates of the same information.

What we could do, then, is hold the shared ptr's in infomanager, then on Clear() check if they're unique(). If so, we remove them. If not, we leave them in the store as they're still being used. This I think would offer the best solution to both long-lived InfoBool's and ensuring they're unique. We would probably want to reset their state if they're still being used, however.

i.e. we change to something like:

typedef boost::shared_ptr<InfoBool> InfoPtr;
void CGUIInfoManager::Clear()
{
  CSingleLock lock(m_critInfo);
  m_skinVariableStrings.clear();

  // erase any info bools that are unused
  m_bools.erase(remove_if(m_bools.begin(), m_bools.end(),
                          std::mem_fun(&InfoPtr::unique)),
                m_bools.end());
  // log which ones are used
  for (vector<InfoPtr>::const_iterator i = m_bools.begin(); i != m_bools.end(); ++i)
    CLog::Log(LOGDEBUG, "Infobool '%s' still used by %ul instances", i->m_expression, i.use_count());
}

[bavison]

OK, here's a new version. It functions as I think you intended, though I was unable to find a form of the member function object syntax which the compiler would accept as a parameter to remove_if - it didn't like what you wrote - so I replaced the construct with what (to my eyes) is the simpler, longhand implementation.

[jmarshallnz]

Yeah - I think you need mem_fun_ref() as you're operating on the shared_ptr itself. A minor detail anyway :)

Will take a good look.

[jmarshallnz]

Overall I quite like it. If you also like the idea, would you have time to rebase the changes so that the infobool stuff is done first, with the listitem stuff on top of that? If not, I can do so.

Minor stuff to cleanup would be to remove reset()'s in constructors.

[bavison]

Refactored as requested. I've been having terrible trouble trying to install additional skins though, so haven't been able to characterise what happens when switching skin.

[jmarshallnz]

Thanks! I'll take a good look through and have a play tonight :)

[jmarshallnz]

Had a play and decided on a bit of cleanup - see:

https://github.com/jmarshallnz/xbmc/commits/more_caching_of_bools_in_listitems

The last couple commits are an indication of what we could do in the future. I'd appreciate your thoughts. ATM it's likely the top commit is less efficient in that we invalidate all infobools (mark them dirty, not re-evaluating them), not just those we call Get() on, but it means you don't have to divert via the infomanager when fetching information. An alternate to this would be to call out from Get() to get the current time from the infomanager/elsewhere and use that for testing whether to call Update(). Typically there isn't much of a gain dependence-wise, as most classes that call Get() are also calling Register() anyway, but that might change later, and reducing inter-dependency is always useful.

In the future, we could store separate caching times per boolean (some info change extremely rarely, and many others don't require frame-perfect timing).

During skin change everything is cleaned up based on the clean logs this produces.

[bavison]

Minor point - I noticed that the log line in CGUIInfoManager::Clear uses %ul in the format string where it presumably should have been %lu.

[jmarshallnz]

Good point - will fix.

Any comments on the general implementation?

[bavison]

Actually, the more I think about it, the more I reckon there's really no need to maintain both a last-update time and a dirty flag in InfoBool. Of course, to be able to use the last-update time, you need to know what the current time is, which is why we've been having to call indirectly via CGUIInfoManager, but once we have the InfoBool::m_dirty flag, I think both InfoBool::m_lastUpdate and CGUIInfoManager::m_updateTime become redundant, and your new function InfoBool::SetTime would be better renamed InfoBool::SetDirty (or perhaps InfoBool::Invalidate).

I get your point about wanting to be able to invalidate a subset of infobools less frequently, but I don't think hanging onto a concept of a global time (that I prefer to think of as a version counter) helps in that respect - if anything, you'd really want the version counter of different infobools to increment at different rates. Switching to using only a dirty flag saves us from having to worry about this.

Now, if you could identify a group of infobools which are always invalidated in lock step and at a very high rate, and for which you call InfoBool::Get less frequently (so that the version number would have jumped by a large amount between calls to InfoBool::Get) then you might end up spending CPU cycles by iterating over those bools to set their dirty flags multiple times between each InfoBool::Get call. But I have now tested your patches at they currently stand, and I couldn't measure any statistically significant time difference (which is the really important thing from my point of view anyway). Your current code iterates over all bools on every call of CGUIInfoManager::ResetCache, which is as bad as it can get, so if this hasn't slowed it down measurably, I say forget about version numbers, and use dirty flags exclusively instead.

How would you like me to proceed? I could squash your patches (plus remove InfoBool::m_lastUpdate and CGUIInfoManager::m_updateTime) into the first of my two commits, for example.

[jmarshallnz]

Yes, as things stand currently we could drop the timers and set the dirty flag directly. That would also work for any slow-update data that's pushed into the infomanager, as we could set the dirty flag at that point instead of every frame. If it's pulled though like everything else is currently, we'd need a counter or timer of some form for slow-update data. I guess that could be in the derived class InfoSlow or whatever though, where SetDirty() checks a counter internally and noop's.

It's probably just as quick for me to make that change and do a fresh PR I guess.

[bavison]

My offer to combine our patches is still open. This and a few of my other PRs (#3674, #3677, #3709 and #3717) seem to have been stalled for a week or two, and it would be good to get them into the December merge window.

[jmarshallnz]

If you have time, go for it - I'm in the middle of building stuff in RL.

I haven't yet reviewed the other two - will look at them tonight to gauge impact for Gotham.

[bavison]

There you go, all merged and (briefly) tested.

[jmarshallnz]

Thanks - I merged the splitted up commits instead (good to have it a little more bisectable).