-
Notifications
You must be signed in to change notification settings - Fork 42
optimize epg updating by updating/persisting only changes to tags... #377
Conversation
at(iTagPtr)->Persist(false); | ||
if (size() > 0) | ||
{ | ||
/* For some reason, mysql fails if we use CommitInsertQueries, so we are using single updates here */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for what reason? what's the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't bother to dig into that, since the function is now deprecated :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really, since you're doing conceptually the same with your transaction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I remember now that CEpgInfotag->Persist() called a Delete in the database (which I removed in this commit) before the Insert/Replace. Maybe this is the reason CommitInsertQueries fails? (but then, why would it work with sqlite?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, the query object should only hold insert/replace/update queries. if it works/worked on sqlite3, then this was undocumented and untested behaviour
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any use in the CommitInsertQueries thing (compared to proper transactions). It is quite restricting, since it is not returning unique ids (if any). Why do you avoid using proper transactions (in general)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any objections against using transactions in general, I objected against how you are using it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I scanned the pvr code and found only 1 call to BeginTransaction, in pvr database table creation. To me, this looks like it is being avoided ;-)
right, please restore the epg copy trick, remove the comment in xbmc/epg/Epg.cpp:831 (it's probably because you didn't commit the queries before sending other type'd queries, like deletes, and it's not used like that anymore anyway after your changes), and I'll pull it after one final review. |
…stead of deleting/inserting all tags from scratch on every update) should improve performance of epg updates due to the fact that most of the work is done in-memory
PR updated, please review |
thanks, merged |
optimize epg updating by updating/persisting only changes to tags...
...(instead of deleting/inserting all tags from scratch on every update)
should improve performance of epg updates due to the fact that most of the work is done in-memory.
Tested this overnight in 2 installations (both sqlite and mysql) and it worked ok in both. This is almost the first time that I check xbmc on the next day (after leaving it on overnight) and epg is not messed-up one way or another.
Please check