[bambi73]

Thanks for explanation,

@Resume point: maybe it was measurement error, i got a lot of spikes and these 200-300ms was just around my "error threshold"
@Fanart: caching of results will be of couse best but isn't it possible to simply store null URL in ART table for fanart in case I want to say "there will never be fanart"?
@Loading data from DB: in last commit you just moved DB opening/closing to thread Start/Finish events. What about taking it even further and load all data (what GetArtForItem() does but for all items) at the start and thus limit 1600 queries to just 1? What tradeoffs does it have? I know that it may be slower for first item and cost some memory. Maybe it'll work poorly for some filtered lists?
@1600 items, though, 1800ms sounds about right: yes, you are right, it's fine for what it does, but ... I simply don't like how it affects GUI. From my point of view all GUI related operations (IMHO fanart/thumb loading belongs there) should be as responsive as possible. "Brute force" approach doesn't work well for them, there always will be someone with slower machine or even more entries in library. Idea what I was talking about at Friday was to make this background loading driven by GUI. Basically let GUI tell to background loader where to start. I experimented with it little bit

https://github.com/bambi73/xbmc/commit/a...3928bead5a

Please note that it's crude piece of code (I'm mostly Java developer) which should only serve as proof of concept. I tested it only on TV Show library list so maybe it'll broke thing somewhere else

@Caching: I think it's way to go, i was quite surprised that it was loading things again and again

Cheers
Bambi

[jmarshall]

@Fanart: yes, you can do that if you wish. We do this for the thumb for example so that the back-compat code runs only once. However, that won't stop it hitting the disk to search for local art. To do that you'd have to set something non-empty but not loadable - eg "-" will likely do the trick. The problem with doing this generally is if the user goes and adds a piece of local art it's not picked up by XBMC automatically. This may not be a problem - after all, if they add a piece of local art when online art is in use, XBMC continues to use the online art anyway, so this may well be a reasonable solution.

The problem with attempting a single query for a list is that you're retrieving multiple rows per media item. There's no nice way around this - particularly once things are in the thumbloader thread (it's possible to do so in the initial query, but ofcourse you have to ignore the multiple rows which is non-trivial in general), thus the single query per item.

As for the patch, the better idea would be to have it at the item level itself: Essentially there'd be a FillInfo() callback function that could be assigned to the item in the media windows. Then, in GUIBaseContainer when the layouts of the items are updated (GUIListItemLayout perhaps), if the info isn't yet filled, it calls the callback function (and then sets a bool so that it doesn't do it twice). This way the currently visible items are prioritised, no matter where you are in the list or how many there are etc. Disadvantage is that you have to protect everything for threadsafety.

In my opinion, however, simply caching the results is probably a far easier way to go. The small delay of retrieving the items would then only be noticeable if you very quickly move to a place in the list far away from the beginning very quickly on first retrieval. All further retrievals would just hit the cache. The disadvantage is having to maintain that cache (e.g. erasing items from it when things change etc.)

If you wish to investigate, a simple technique is to add a CFileItemList m_itemCache member to the thumbloader with SetFastLookup(true) enabled. Then for each item in LoadItem, check if there's a matching entry in m_itemCache and if so copy it across. If not, run the normal LoadItem code and add it to m_itemCache at the end. (Alternatively, just use a map<string,CFileItem>)

Cheers,
Jonathan

[vicbitter]

Jonathan,
I tried out adding CFileItemList m_itemCache and this has significantly improved performance (after the initial load)...

This is certainly moving in the right direction. Are there any other techniques worth further investigation?

Many thanks

[jmarshall]

The only other things are perhaps looking to save the TextureDatabase open/close's. Problem here is they're in the base class, and the base class is designed for multi-threaded access. So you don't really want a member function being accessed there necessarily (this issue will be eliminated once music is done).

The caching helps either way, so if you've done up a patch I'm happy to take a look.

[bambi73]

Yesterday I tried "single qurery" approach and got quite good results. Query loading into map took ~25ms, so it helped a lot but i still got only down to ~1500ms which was kind of surprise because I commented out almost everything from ThumbLoader.
Anyway I understand that this probably will not work because you must be able to tell if query is for tvshows, episodes, movies etc otherwise it will load whole ART table. And I'm not sure if all lists contains only one media type.

Today I tried cache approach but CFileItemList wasn't working for me. It crashed always at homescreen because (my guess) it tried to free already freed memory on some CFileItem field, but I didn't checked it deeply. So I ended up with

Code:

map<CStdString, map<string, string>*> m_artCache;

and it's working fine. Anyway I still got only down to ~1500ms (same as above) which was surprise too because there was nothing around. After short investigation I found that problem is OnItemLoaded() observer callback. Maybe another candidate for optimalization.
My current cache implementation doesn't support art updating, so maybe vicbitter will post his patch.

EDIT: Forgot to say result, it was ~15ms in second round (when cache is already populated) without observer callback

[jmarshall]

LOL - all that does is invalidates the database cache, for which I see no point whatsoever to be honest. Nice find!

I shall kill it as it should not be required at all.

[vicbitter]

Happy to post my code... Just cut me some slack as I haven't coded in a long time... So functional code but not perfect code...

How is the best way for me to create a patch? I don't have a github account (as I said I haven't coded in anger for a long time).

Just let me know the process!

Finally, regarding the performance improvement, as mentioned there is an initial hit (depending on Library size) but subsequent access is significantly faster. For example, first retrieval of an individual Thumbnail is ~35ms but subsequent retrievals are <1ms... (measurements based on my system)...

[jmarshall]

Yeah, a patch is fine (git format-patch HEAD~1) - no worries if it's a mess, as someone else can carry it forward. No need to create a github acct unless you feel you're likely to want to mess around with other stuff (aka be sucked into the void )

Cheers,
Jonathan

[jmarshall]

Updated master to remove the OnItemLoaded() silliness, so that should speed things up a bit further.

There's 2 more things to do:

1. Take care of caching. The main problem here is if we do store it, we need to flush it out when anything at all changes. It may be that we can incorporate it into the slow directory fetch stuff, so that the thumbloader essentially won't be hit at all. I think the directory fetch stuff should be flushed out at the appropriate points already, but will need to check.

2. Take care of the slow loading of art caused by the incrementing of the use count. This is most likely due to the database needing to be written to - we may be able to queue the writes up instead, and then do them all in a transaction periodically. The trick is doing this off thread frequently enough that the counts are still reasonably accurate.

Patches addressing either will be considered, as usual

Cheers,
Jonathan

[jmarshall]

A potential for number 1 is available here:

https://github.com/jmarshallnz/xbmc/tree...er_results

This basically takes advantage of the fact that the normal directory fetching has a cache. I *think* it should work fine, but I'm not completely certain - particularly for file listings. eg things to look for are to filter the current list (particularly a files based list - I think library lists will filter OK) using the Filter item in confluence on the left and then unfilter it, and check that the resulting list is actually unfiltered. Reason is we're saving the items after the thumbloader has run, which will be the sorted + filtered list, rather than the original full listing. Other things to check are what happens when you add a new file to the folder. As I say, I'm pretty sure that all library based listings should be OK - it'll be files + addon listings etc. that might screw up. Test it thoroughly

There's a couple of tricks we can use if that doesn't work, but first we need to know where it doesn't work. It should refresh the directory (and the thumbloading) if you go out of the directory (i.e. parent up a level) then go back in. It won't refresh just going from the video listing -> playing a video -> back again (not sure about listing -> home -> listing).

Cheers,
Jonathan

[vicbitter]

Yeah, a patch is fine (git format-patch HEAD~1) - no worries if it's a mess, as someone else can carry it forward. No need to create a github acct unless you feel you're likely to want to mess around with other stuff (aka be sucked into the void )

Cheers,
Jonathan

The thumbnail cache patch...

Let me know if you need anything further?

[jmarshall]

The basic idea of the patch is fine, yes, though is still done offthread.

Mind testing the above version of mine? This is done on-thread (i.e. we basically overwrite the cached listing retrieved by the window initially, so thumb URLs will be set effectively instantly).

Cheers,
Jonathan

[vicbitter]

A potential for number 1 is available here:

https://github.com/jmarshallnz/xbmc/tree...er_results

This basically takes advantage of the fact that the normal directory fetching has a cache. I *think* it should work fine, but I'm not completely certain - particularly for file listings. eg things to look for are to filter the current list (particularly a files based list - I think library lists will filter OK) using the Filter item in confluence on the left and then unfilter it, and check that the resulting list is actually unfiltered. Reason is we're saving the items after the thumbloader has run, which will be the sorted + filtered list, rather than the original full listing. Other things to check are what happens when you add a new file to the folder. As I say, I'm pretty sure that all library based listings should be OK - it'll be files + addon listings etc. that might screw up. Test it thoroughly

There's a couple of tricks we can use if that doesn't work, but first we need to know where it doesn't work. It should refresh the directory (and the thumbloading) if you go out of the directory (i.e. parent up a level) then go back in. It won't refresh just going from the video listing -> playing a video -> back again (not sure about listing -> home -> listing).

Cheers,
Jonathan

Jonathan,
I have tested the patch against my library and it has addressed a lot of the performance issues. For example, before when I moved in and out of sets there was a noticeable refresh of the thumbs whereas now they are instantaneous...

The only outstanding issue is on the initial load of the library as it still takes a few seconds to load to the end. I know you have stated "The small delay of retrieving the items would then only be noticeable if you very quickly move to a place in the list far away from the beginning very quickly on first retrieval." but I think it depends on the skin used. For example, I use Neon and view my Movies using the "showcase" view which wraps the library around. So on first view you end up with the first half of the thumbs missing (for a couple of seconds) whilst it loads.



Having said all of that, this is definitely moving in the right direction.

More than happy to test out any other proposed patches as they come along...

Thanks

[Voyager]

I've been playing with the code too, trying to cache the following two methods CVideoDatabase::GetArtForItem and CTextureDatabase::GetCachedTexture. In essence the caching would remember in memory what art is associated for a given item (so the image URLs for a given media type and ID, instead of fetching it from the DB using a SQL query), as well as remember in memory what local texture jpg is associated to that URL (instead of fetching it from the DB using SQL). I was able to scratch off a few ms here and there - sometimes in the best case up to 10-20 ms per item, but on average certainly less than that.

Nevertheless I didn't really see the big performance boost I was expecting with that, so I started to wonder if the JPG loader itself is not the rate limiting factor here. I noticed that when I switch to a skin with small thumbs (e.g. Alaska revisited) it loads much quicker than with the "big" thumbs you have in Confluence.

[jmarshall]

Thumb size is irrelevent - they're all loaded at the same resolution.