Skip to content

Add code to support doing finer grained movie updates, and use it with JSON-RPC [ticket #14967] #4301

Closed
wants to merge 10 commits into from

7 participants

@cg110
cg110 commented Mar 1, 2014

This request contains updates to support finer grained move updates via the JSON RPC interface SetMovieDetails.

Currently the code deletes most of the existing movie information and rebuilds it. This causes a lot of unnecessary churn when one item is actually changed.

This code adds the ability to update only the items that have changed. This reduces the time for carrying out an update dramatically in many cases. MillhouseVH did some testing on a raspberry pi, and found the timing change for updates in some cases dropped the times from 15s to 150ms:
http://pastebin.com/raw.php?i=7isML2xb

The time improvement varies depending on the updates being done, and the size of the current data.

More details can be seen in the forum thread:
http://forum.xbmc.org/showthread.php?tid=184411

Note the branch being pulled from was a clean branch with the changes cherry picked, and then some tidy up and refactor done.

The code has been tested on windows and compiled on linux.

cg110 added some commits Feb 17, 2014
@cg110 cg110 Initial work on fine-grained DB updates for Movies
Lots of new code here, most of this is from a post on forums:
Things to do:
A few bits of tidy up refactoring, eg CVideoDatabase::GetValueString's 2
versions could do with their core being merged.
Style/whitespace probably isn't quite right
Some of the funcs could probably do with better names
Some docs on how the Update works, as it's rather tied into being called
from JSON
Testing - at the moment I've done some basic testing, but really need to
check that the api works in all cases.

Note the focus is for the available calls from the JSON VideoLibrary.

How does it work:
During the parsing in UpdateVideoTag of the JSON items into the existing
object, a vector of changed items is made.

To try and make the code more readable I've converted the long list of
If's into calls to new wrapper funcs (also called UpdateVideoTag) The
simple wrappers checks that the current value is differnt to the new
value, and only flags it for update, complex lists are always updated.

I'm not happy with how many versions of UpdateVideoTag there are, I
can't escape the feeling some kind of generic would be better, however,
there's no generic conversion on CVariant.

The idea is that the JSON provides the vector of changes into a new API
in the VideoDatabase code.

The new API is called:
UpdateDetailsForMovie

It takes the same params as SetDetailsForMovie, along with the vector of
changes.

The first it does it take the vector and convert it into two lists,
handled by PendingUpdates.

One is a list of the simple changes (in fact a list of VIDEODB_ID enum
values).  This list is checked for types that I guess have changed into
lists, eg genre, directors, although they were once their own field in
the db.  This simple list is used to create the UPDATE SQL call.

The more complex list contains harder to parse items, eg art and tags.

If it doesn't understand one of the changes it falls back to using the
SetDetailsForMovie.

The update work then tries to only do the needed updates.  It starts
with the simple updates, and processes the lists, eg Genre, director,
studios, and deletes the db entries and adds them back in.

It them moves onto the tags and art, and processes them in the same way,
IE only deletes them if needed.

The last part is the SQL Update.  I've added a new version of
GetValueString, which takes the vector of "simple" changes, and will
generate an update of just the items that have changed.

If nothing needs an "UPDATE" one won't be run.
075b3a8
@cg110 cg110 Reapply change that was accidently undone
In the process of setting up the fork, I copied my changed files, and
missed that change 43a62fe hadn't been
applied.

Reapply that change to fix incorrect query which includes idActors
d950287
@cg110 cg110 Update VideoLibrary based on comments from GitHub. Basically sprinkle…
… some const, and rename the functions and the fieldValue.
ab4f6a1
@cg110 cg110 Remove c++ 11 cbegin and cend
Fix compiler errors/warnings for non c++ 11 compilers.
1139eca
@cg110 cg110 Short cut the case of there being no updates to process
If updateDetails is empty return the movie Id.

While the code won't run any SQL, it saves Transacting nothing.
5b0d201
@cg110 cg110 Further performance improvements for updates.
Split artwork changes into altered and removed.

Add some log messages to allow for better timing.

In SetArtForItem only update an item of art if the url has actually
changed.  Saves a number of extra round trips.

Remove code that set the lastplayed and playcount to match any other
found copy of the movie.  It doesn't seem to make sense to do so during
an update, as the update requires the file already exists, so it's not a
new file.
41ec11c
@cg110 cg110 Refactor CVideoDatabase::GetValueString
Both functions now call a common routine to process the value string.

Also correct a missing pass by reference on the newer version of
GetValueString.
254585f
@cg110 cg110 Some code tidy-up before pull request
Fix build failure.
Fix some indentation issues.
Remove some comments/notes to myself.
Merge some CStdString and the = PrepareSql lines.
e717b8e
@cg110 cg110 Fix Log message
Correctly pass log message a char * rather than std::string.
69a2207
@MartijnKaijser
Team Kodi member

fyi did a test compile and it build fine on all platforms.

@cg110
cg110 commented Mar 1, 2014

Thanks for building on all platforms, always hard to be sure there won't be one that gets upset.

It appears there's a lurking issue, so I'm going to add another commit, however, I need to ask people that know about the messaging system to look at it.

So please hold off pulilng this request.

@t-nelson
@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone Mar 2, 2014
@cg110
cg110 commented Mar 6, 2014

Note the performance issues with the events have been tracked down to the IsSamePath code, and so the changes have been reverted in this PR and placed in:
#4345

@cg110 cg110 Possible fix for performance issue on updates
If 300+ updates are carried out too quickly then the message loop
struggles with the refresh events from the Json layer.

It appears that the JSON layer triggers a full refresh, and this causes
pain if you're on the movies view as the refreshes process.

I've added an extra API to allow a FileItem to passed into the refresh
builder, and it then passes that item around as being the thing to
refresh.

This stops the heavy refresh rate, but still slows the UI down while it
gets through the backlog of events.

I've also added code to only push a refresh event if there was actually
a change to the movie.
f9f194e
@cg110 cg110 referenced this pull request Mar 6, 2014
Merged

Speedup FileItem::IsSamePath #4345

@t-nelson t-nelson added the The Hoff label Apr 3, 2014
@mkortstiege
Team Kodi member

What's the status here? @jmarshallnz, @Montellese mind having a look at it again?

@MartijnKaijser
Team Kodi member

there's merge branch commit in it.
so @cg110 please remove the unrelated commits and rebase

@MilhouseVH

As I sit here using JSON to update 531 movies with clearart and logo artwork, a process that looks like it's going to take the best part of an hour or more (when it would take mere seconds with this PR), it crossed my mind if there is any hope of this merging soon?

@jmarshallnz
Team Kodi member

What I don't like about it is the tortuous route from JSON to the db (start with a CVariant map, map that to a CVideoInfoTag and a set of strings indicating what to update, map the second to videodb id's, break those into simple+complex updates, do the actual updates).

I wonder if it could be simplified by passing the CVariant map (+ a string vector of what to update) to save diving via CVideoInfoTag - not really sure?

@Montellese what do you think?

@cg110
cg110 commented Aug 6, 2014

Yeah, the current change was really a minimal change to try and fit with the existing code.

It would make more sense to get a list/map of item changed and the new version of the item, and then do those updates without populating a CVideoInfoTag. But it's a much bigger chunk of work to do that.

Certainly it'd be good to have one mapping of the json param name into the database layer name (and the db column name) and just update the needed fields. Particularly if we can be smart and use generic/types to work out what to do for the columns. I did have some thoughts on trying to do that, as I prefer to make the simplest to maintain code (even if it's a little crafty) than mountains of copy, paste tweak.

I probably should revisit, but I've not had time to look at this kind of thing for a while.

@jmarshallnz
Team Kodi member

Yeah - my guess a cheap win to improve things short-term would be to have an indication as to whether any of the link tables have changed or not. If not, it's a simple movie table update (just do all fields). If so, you add in the link table(s) that have changed.

My guess is that would give 90% or more of the speed improvement while keeping code changes minimal at this point (targetting Helix) I think? Later on we can then make it more generic and simplify the data flow a bit.

@cg110
cg110 commented Aug 7, 2014

How far along into helix are we? other than messing the branch up with a merge commit (which I just rolled back, I suspect a rebase to current master is needed) what I had here was relatively minimal. Still more than you said, but certainly attempted to keep the changes small.

And yes you're right most of the speed gain is not deleting all the linked items, to then relink them back in as the code currently does. Especially when the changed item is just a field on the movie.

I'd still like to get one table that maps the json name, to the db name/id to the db column name so it's one place to maintain/update.

@jmarshallnz
Team Kodi member

Quite far along, in that this is the last merge window for features and larger changes. I have no problem with a simpler version of this going in, applying to more than just movies ofc.

The idea for the simpler version is basically just do everything the same with the exception of the simple mapping - i.e. just do the complex/link tables in the vector you pass across as changed fields. Rationale is that then you don't need the string<->db field mapping for now, and we can spend the time getting it more generic afterwards. That way the UpdateDetailsForMovie() takes in a CVideoInfoTag, art, and a list of link tables to update, so the changes to both the JSON stuff and CVideoDatabase are even more minimal.

@jmarshallnz
Team Kodi member

I've got it rebased up to master (but not tested/changed in any way other than adding a comment on the bit that wasn't completely obvious without reading the wider change) here:

https://github.com/jmarshallnz/xbmc/tree/finer-grained-updates

I'll go through it tonight and see if the simplification I had in mind really does drop the changes down, or whether we should just go with what you have anyway.

@Montellese
Team Kodi member

I agree with @jmarshallnz that for now just special casing the link table changes would probably provide the biggest gain for the least changes in the code.

I'm not sure if directly mapping JSON properties to DB columns would be the right way to go. It would introduce yet another mapping of those properties for which there already are mappings like:

  • CVideoInfoTag members --> DB columns (in VideoDatabase.h)
  • CVideoInfoTag members --> JSON properties (through CVideoInfoTag::Serialize())
  • CVideoInfoTag members --> DatabaseField enum values --> DB columns (through CVideoInfoTag::ToSortable() and DatabaseUtils)

IMO the cleanest (maybe not the fastest though) approach would be the generic mapping of JSON properties --> CVideoInfoTag members through a Deserialize() implementation (i.e. the opposite of the already existing Serialize()) and then we could use one of the existing mapping to DB columns. What we are still missing then is how to know which properties of CVideoInfoTag have changed. Being able to track changes (mainly "has this property changed") would be a huge improvement in general as it could be used in several places. But obviously it's also difficult to implement...

@cg110
cg110 commented Aug 7, 2014

I was thinking more about a table that has all the mapping info in, and is used to then build the other map tables. IE add a column to the database, it encourages someone to think do we need a json name for the column.

However, the change could have the scope cut back to just the link tables, it catches the simple updates as it has to map the json items anyway, so the simple changes fell out of the mix.

@t-nelson
@cg110
cg110 commented on f9f194e Aug 7, 2014

Note to self on this commit. Should pass CFileItemPtr into the NotifyItemUpdated, so we only construct one CFileItem, rather than doing one on the stack, and then copying it onto the heap.

@cg110
cg110 commented Aug 10, 2014

Closing this off, as I think the bulk of the benefit has been taken into #5170.

@cg110 cg110 closed this Aug 10, 2014
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.