A few database/query optimizations #1085

Merged
merged 9 commits into from Jul 7, 2012

8 participants

@Montellese
Team Kodi member

vdrfan and myself took the time to log all the SQL queries we run in XBMC (especially during startup etc) and we were surprised by the huge number of queries that are run when XBMC starts (without doing a single action as a user). We have taken a look at the queries that appeared most in the log and tracked them down. We have done the following optimizations (in the order of the commits):

  1. On every CDatabase::Open we check the version of the database we are opening to see if we need to update the database. These queries make up about 20% of all queries that are run in XBMC. Once a database has been updated, we will never have to update it again while XBMC runs so there's no need to check for it every time. Using a map we remember which databases we have already opened (and updated) and don't run the version query anymore if the db is already in the map.
  2. Adding bookmark.timeInSeconds and bookmark.totalTimeInSeconds to the video views saves us from having to make those calls to retrieve a video's resume point manually. This saves at least one query for every video when listed in the GUI.
  3. In the recently added job for recently added album songs we iterate over all retrieved songs and retrieve their album id and album thumb (two extra queries per song). The album id is already available in CMusicInfoTag (just not exposed) so we can save that query completely. Furthermore as we retreive multiple songs from the same album they can all use the same album cover so we can just remember the cover of the current album until the album changes and only then retrieve the album's cover. These two changes saved 190 (of 195) queries for the recently added album songs widget in my setup.
  4. see 5.
  5. jmarshall added a new seasons table in his recent work and that seasonid is needed to retrieve season thumbs. By storing the idSeason value in the video views and in CVideoInfoTag we can save a query for every episode. Furthermore we can save some queries in recently added episodes by also remembering the season and show thumb for every season/show and only querying the database if we haven't come across that season/show yet. This saves about half of the queries for the recently added episodes widget.
  6. the implementation for recently added albums retrieves the values for rating, year, artist and album title manually with 1 query per attribute (i.e. 4 queries per album) even though that data is already available in the CAlbum object's we get from the database.
  7. When retrieving sets, we retrieve all movies that are part of a set and put the set together based on those movies (to get additional metadata like rating, year etc). In that case we don't need to retrieve the stream details because we don't use them and they'll be retrieved when browsing into the set in the GUI. So we can save those queries.
  8. When a user adds addon shortcuts to the confluence home screen, the addon title and icon are constantly retrieved from CGUIInfoManager to be able to show them in the GUI. During that query we also check against the addon database, if the addon is disabled even though we don't use that information, so that check is unnecessary. This saves a lot of queries because they are repeated periodically as long as the user is on the home screen. This is even worse for the global search addon which has been integrated into the confluence home screen by default. For every frame (i.e. 60 times per second) we query the enabled/disabled state of the addon from the database (SYSTEM_HAS_ADDON). We haven't looked into that yet, but maybe someone can shed some light on it.
  9. When changing a view/window in the GUI we have to retrieve the next view/viewstate which includes queries to the ViewState database. Instead of doing one query to retrieve the view state we run the same query 3-4 times in different places. Especially on embedded devices these queries are rather costly and slow down the changing of views/windows. This commit is highly experimental because neither vdrfan nor myself have any real knowledge about this process.

All in all with these changes the number of queries on startup (before doing a single user interaction) goes down to 20-30%. On fast systems there's no real noteable speed-up because some of those queries are not expensive at all but on embedded devices (where I/O (i.e. accessing the sqlite database file) is more time intensive and computation power more scarce) it could speed up the startup process and some loading times of item lists.

@mkortstiege mkortstiege was assigned Jun 17, 2012
@garbear
Team Kodi member

Off topic: Something that struck me as weird is that databases aren't loaded on startup, they're loaded "just in time." Thus, a database update isn't done on app load, it's done when the skin first uses "HasContent(Video)". Just something I found weird, and something that made debugging the DB creation/update process a little more difficult.

@theuni
Team Kodi member

nice work! can't comment on the code, though I applaud the effort.

@Montellese
Team Kodi member

OK I've pushed a few changes/fixes (as extra commits which can be squashed later on):
1. I removed the last commit about the view states as it is not correct and will take more time to look into.
2. I've added a comment in CGUIInfoManager concerning the retrieval of addon information and not checking whether the addon is disabled or not.
3. I've removed CMusicInfoTag::GetArtistId() as requested.
4. I've moved the calls to CVideoDatabase::GetStreamDetails to CVideoThumbLoader. During some of the test I ran when I re-wrote the sorting implementation I noticed that about 20-30% of the time needed to retrieve a list of movies is caused by retrieving the streamdetails for all the movies. So this might speed things up a bit. It would replace the commit about removing an unnecessary call to GetStreamDetails when retrieving movie sets.

Still open are the discussion on database updates and on the season/show thumb in recently added episodes.

@pieh
Team Kodi member

Could we also add some flags to item lists like "has already fetched metadata" and "has already fetched resume point" (or single flag after commit that joins bookmark table to movie/episode/musicvideo views)?

I would certainly use these flags to determine if we can return or need to fetch metadata and playcount here (the "if clause" on L364 is to determine if we already queried database): pieh@0f63058#L0L364

I could also make retrieving resume points for files mode smarter (it's not actually working in master - just in my branch). Instead of checking and quering every item as now in thumbloader, i could do single query for files+bookmarks for pathId of directory item list is representing.

I'll supply patch if that would be ok

@jmarshallnz
Team Kodi member

@pieh You could probably tap the resume stuff into the routine that grabs all playcounts for the current listing - IIRC that does a similar thing using a single query - see LoadVideoInfo()

@Montellese
Team Kodi member

@jmarshallnz I saw you comment in the thread about ShowThumb and SeasonThumb for recently added episodes and added a commit that removes those two for now. With that we can also get rid of the optimizations I came up with for that part.

That mainly leaves the database update procedure to be optimized in this PR. Should I remove that commit (because it will be fixed later on) or should I add the necessary locks around my optimizations or is there a specific plan on how to do it?

@Albinoman887

i dont know if this PR is related to my problem but as of recently i had to redo my entire media server to had to rescan my libraries (what a pain) anyways i noticed xbmc no longer grabs the codec info of the movies or tv shows automaticily anymore you have to actually play the file. I remember a bug like this a long time ago before 10.1 will this PR fix that?

@Montellese
Team Kodi member

AFAIK you don't have to play a video file you just have to view a GUI listing which contains the file and then XBMC will automatically read the streamdetails (codec, resolution, ...) of the video. AFAIK it has never been the way that XBMC grabs this information during the scanning process (don't know why though).

@Albinoman887
@jmarshallnz
Team Kodi member

@Montellese: I'd just remove season thumb - show thumb still kinda makes sense I think (balances with show fanart).

As for the db update, it's up to you - you can either protect the map with a lock or drop and we'll isolate with a proper db update thing. For a proper db update, I think we'd need to:

  1. Have a static indicating that db's were OK to open (and critical section to guard).
  2. Have a function in each db to do the update.
  3. Call said function as early as possible on app start.

It might be as simple as dividing CDatabase::Open() into two functions, one that does OpenAndUpdate() and the other that just does Open(). The latter would just check the static for whether the db is up to date and fail if not, else call Connect(currentDB, false). I think the OpenAndUpdate() would handle correctly setting up the advancedsettings and handle fallback to sqlite etc.

@Montellese
Team Kodi member

@jmarshallnz I've re-added the ShowThumb for recently added episodes (and squashed it with the commit that removed it) and I've added a lock to CDatabase::UpdateVersion()

@Montellese
Team Kodi member

So do we want to wait till someone has the time to come up with a proper fix for the database update or should I rebase & squash the commits so they can be merged?

@jmarshallnz
Team Kodi member

IMO rebase + squash and push it in.

@Montellese
Team Kodi member

OK I've rebased and squashed the commits. There were a few conflicts in RecentlyAdded.cpp (see 27e3814#commitcomment-1544120) so I'll wait till that's settled.

@Montellese
Team Kodi member

Okay I rebased again. I had to re-arrange the optimization for retrieving cover and fanart of albums in recently added album songs because the code changed from manual lookups to calling CMusicThumbLoader::LoadItem.

@jmarshallnz
Team Kodi member

If we're leaving b5b9fa8 in, then I think we mayaswell keep the season art there as well. I'm not sure whether it's worth it otherwise?

Also, abb44bf should probably be split to removing the season thumb separate (or keep it in and we can remove it later?)

@Montellese
Team Kodi member

I don't really care whether we keep the season art/thumb or not. Personally I'd always prefer to see either the tvshow thumb or the episode thumb and never the season thumb in recently added episodes but I guess that comes down to personal preferences.

@jmarshallnz
Team Kodi member

Just leave it in then and I'll kill it off later once we properly handle banners+posters?

Montellese and others added some commits Jun 17, 2012
@Montellese Montellese recentlyadded: save queries for season id and season/show thumbs
Use the idSeason value available through episodeview instead of having to
retrieve it for every episode. Furthermore remember the already retrieved
season/show thumbs to save querying the same images again.
f95c66b
@Montellese Montellese recentlyadded: use values from CAlbum instead of manually querying th…
…em one by one for every album
60ba8b1
@mkortstiege mkortstiege videodb: do not retrieve streamdetails for set items - the actual str…
…eamdetails will be queried for the single items later on
30e9f45
@mkortstiege mkortstiege fixed: do not query the addon state when asking the guiinfomanager fo…
…r addon title and/or icon

This commit removes tons of useless queries made from the mainloop when using addon shortcuts on
the home screen. As the infomanager is asked for the icon or title only, it's not needed to check
whether the addon is enabled or disabled all the time.
5c10498
@Montellese Montellese move call to GetStreamDetails to CVideoThumbLoader fcb6107
@Montellese Montellese recentlyadded: remember the cover and fanart of an album
When retrieving multiple songs of the same album for recently added album
songs, remember the album's cover and fanart and therefore avoid querying
and retrieving the same cover and fanart multiple times.
ea3ea36
@Montellese
Team Kodi member

OK I re-added the SeasonThumb for recently added episodes and removed that one query for the albumid as it will never be executed anyway.

@jmarshallnz
Team Kodi member

Looks good - pull it in.

@Montellese Montellese was assigned Jul 7, 2012
@Montellese
Team Kodi member

Seeing that you have started working on the real db update fix should I remove that optimization from this PR to save you a rebase?

@jmarshallnz
Team Kodi member

Doesn't worry me either way - I'm sure before it goes in I'll need to rebase anyway :)

@Montellese Montellese merged commit 35c9f14 into xbmc:master Jul 7, 2012
@koying
Team Kodi member

Hi,

It looks that between this and some later thumb loading optimizations, the streamdetails are actually never fetched, and thus streamdetails is always NULL from JSON.

Not sure how to best solve this...

Team Kodi member

We'll probably have to manually get the stream details for all the video items that JSON-RPC wants to return to the client (but only if streamdetails are requested by the client).

Team Kodi member

Another solution would be to keep the "GetStreamDetails" as they were, but adding a bool to CVideoInfoTag indicating whether the details were already fetched, to avoid duplicate db fetches (trying this now)

Or do both...

Team Kodi member

The reason why I moved the call to the thumbloader is that there are several calls to GetMoviesByWhere (and therefore GetDetailsForMovie) which fetch the streamdetails (which causes an additional query and therefore additional processing time) but don't need the streamdetails. The problem isn't knowing whether the streamdetails were already retrieved or not. We want to do as few SQL queries as possible. Making the necessary calls to GetStreamDetails from JSON-RPC will not increase the number of SQL queries compared to how it worked before this commit . There will still be the same amound of SQL queries per movie but they will be triggered from a different location.

Team Kodi member

Re as few SQL as possible, I understand that.
I meant to do:

bool CVideoDatabase::GetStreamDetails(CVideoInfoTag& tag) const
{
  if (tag.m_iFileId < 0)
    return false;

  if (tag.m_streamDetailsLoaded)
    return tag.m_streamDetails.HasItems();

But, obviously, only retrieving stream details when requested doesn't harm either ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment