Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various addon/repository improvements, mostly efficiency #4091

Closed
wants to merge 14 commits into from

Conversation

jmarshallnz
Copy link
Contributor

Quite a few commits here, aimed at speeding up repository processing.

The main gain is the dependency speedup on my machine (passing the database object into the function) - I have an SSD though, so it may be different on other machines.

The next largest gain is the broken status updating (the addition of Begin/CommitMultipleExecute and calling that from CRepository::DoWork()), as well as the addition of GetAddonVersion().

The rest allows reduced queries, in particular by utilizing a custom collator within SQLite to ensure that sorting by version works as you'd expect. This is not implemented for mysql, but that's OK as the addons database is SQL-only anyway. I'm sure there's a version that could be done for mysql as well though. Note that it doesn't offer anything much in the way of speedup, so we could drop the four commits implementing this if it doesn't offer much in the way of speedup for others.

There is a possibility to perform GetAddon() single query via

SELECT * FROM addon WHERE id=(SELECT id FROM addon WHERE addonID='my.addon.id' ORDER BY version DESC LIMIT 1)

but this didn't seem to be any faster. Any ideas for improvement most welcome!

(One thing we could consider post-Gotham is just storing the highest version in the database anyway. I'm not sure what the implications here are though - some thought would be needed to make sure we get this right, particularly in regard to the linking of repository and addons)

Lastly, I've included some timing logs for @MartijnKaijser to test with.

@jmarshallnz
Copy link
Contributor Author

@MartijnKaijser a debuglog of you updating the repos would be useful to check we've hit the biggest timesinks on your machine.

jenkins build this please

@stefansaraev
Copy link
Contributor

yay! a very, very good one.

force-refresh without this PR. + commit a02bf0f7ec monkey see - monkey do copypaste:

13:53:21 T:139909059016448   DEBUG: DoWork TIMING: dep 7887, invalid 51556, update 22, broken 8479, getaddon 1001

force-refresh with this PR:

13:28:45 T:140264657991424   DEBUG: DoWork TIMING: dep 686, invalid 130, update 13, broken 72, getaddon 242

and no more nagging busy spinner when I try to browse a "currently-updating" repo

@ace20022
Copy link
Member

There is a possibility to perform GetAddon() single query via

SELECT * FROM addon WHERE id=(SELECT id FROM addon WHERE addonID='my.addon.id' ORDER BY version DESC LIMIT 1)

but this didn't seem to be any faster. Any ideas for improvement most welcome!

You could use a join or rewrite it to not use a sub query. I'm not an expert, so I'm not sure this will be faster.

@jmarshallnz
Copy link
Contributor Author

Updated with a slight improvement to the addon version query.

The main question I have is whether the custom collator is worth it or not. I can do a GetAddonVersion() without the collator that is likely close to being as fast - thus, this isn't really required speed-wise.

The advantage of the collator is it makes things much easier to read code-wise. The disadvantage is the added code for the collation (behind the scenes) and that it also means you can't sort on the version columns outside of XBMC (e.g. in sqlite3) as the collation is XBMC-specific.

@Montellese, @koying your views on that would be appreciated.

(In the meantime, I'll do up another PR with the collator dropped to make sure that it doesn't affect speed too much on slower machines).

@koying
Copy link
Contributor

koying commented Jan 28, 2014

Not much to say technical-wise as it's very sqlite-specific.
Some thoughts, though:

  • From a quick look, adding a custom collation to mysql implies server work, so, realistically, we can't force this
  • Thus, sqlite and mysql would diverge re functionality, which I consider an added risk considering supporting "simple" mysql is already not so easy
  • Even for just sqlite, relying on engine-specific features is an added risk

Bottom-line: From my POV, unless using a custom collation is a real performance booster, I'd rather have code complexity than db black-magic ;)

@jmarshallnz
Copy link
Contributor Author

Thanks @koying - will merge #4098 instead, as it seems to have similar speedup benefits (and the SQL isn't that complicated)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants