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

Building ListItem lists is very slow #604

Closed
dagwieers opened this issue Nov 29, 2019 · 15 comments · Fixed by #606
Closed

Building ListItem lists is very slow #604

dagwieers opened this issue Nov 29, 2019 · 15 comments · Fixed by #606
Labels
bug Something isn't working
Milestone

Comments

@dagwieers
Copy link
Collaborator

Describe the bug

Compared to earlier VRT NU versions, building a ListItem list of 50 or 300 items has become unbearably slow (more than 5 seconds) on Raspberry Pi. This is with local HTTP caching, so it is not due to the network.

The performance degradation appears to happen as part of the metadata handling.

@dagwieers dagwieers added the bug Something isn't working label Nov 29, 2019
@dagwieers dagwieers added this to the v2.3.0 milestone Nov 29, 2019
@dagwieers
Copy link
Collaborator Author

dagwieers commented Nov 29, 2019

You can compare listing "My most recent" with VRT NU v2.0.0, v2.1.1 or v2.2.3 (all sub-second) to master branch (more than 7 seconds).

Listing "All programs" takes 25 seconds.

@dagwieers
Copy link
Collaborator Author

It seems unrelated to resumepoints, even when disabled you see this effect.

@dagwieers
Copy link
Collaborator Author

dagwieers commented Nov 29, 2019

Here is a diff between v2.2.3 and the master branch: v2.2.3...master

@dagwieers
Copy link
Collaborator Author

It is also unrelated to our change from json.load(urlopen(req)) to json.loads(to_unicode(urlopen(req).read()))

@dagwieers
Copy link
Collaborator Author

Still fast: 34240cd
Very slow: 0b9b130

The problematic change was: #575 😞

@dagwieers
Copy link
Collaborator Author

dagwieers commented Nov 29, 2019

I cannot find exactly why passing a kodi-instance with methods is so much faster than calling discrete functions...

https://stackoverflow.com/questions/37472419/speed-static-methods-vs-class-method

@mediaminister
Copy link
Collaborator

Definitely related to reuselanguageinvoker #560 (comment)

@dagwieers
Copy link
Collaborator Author

But we follow the recommendations and we no longer store values in long-lived instances. In fact we have no long-lived instances anymore in the plugin. And no global variables.

If I disabled reuselanguageinvoker I still have the same performance problems. Performance problems only exists with building ListItem lists of content. The time is directly proportional to the number of items in the virtual directory when showing video content, the TV guide schedule has a listing of 37 items that is not affected (no metadata?).

@dagwieers
Copy link
Collaborator Author

Problem was easily spotted by profiling. Every invocation of Addon() not only rereads the settings, but also reparses po language files. 😞

@mediaminister
Copy link
Collaborator

mediaminister commented Nov 29, 2019

In https://github.com/pietje666/plugin.video.vrt.nu/tree/34240cdc9819818ab54836d7077805982ccefed2 KodiWrapper is only imported once and than referenced throughout the entire addon. It's obvious that this is faster than the current approach where you import the same functions again and again.

I think we should return to the previous situation. You only need to call Addon().getSetting instead of a stored addon variable to get realtime settings.

@dagwieers
Copy link
Collaborator Author

dagwieers commented Nov 29, 2019

Successive and selective imports should not have an impact as this is cached.
The overhead should be unnoticable.

ncalls  tottime  percall  cumtime  percall filename:lineno(function)
588200    0.387    0.000    4.018    0.000 /usr/lib/python2.7/site-packages/polib.py:1476(process)
189435    0.369    0.000    2.338    0.000 /usr/lib/python2.7/site-packages/polib.py:1583(handle_ct)

Obviously this is our own xbmc stubs doing this, so I might be hunting ghosts...

@dagwieers
Copy link
Collaborator Author

dagwieers commented Nov 29, 2019

So moving ADDON = Addon() into the global context, the performance problems are solved.
It would be fine to keep this out of the addon_entry.py, but we could update the global ADDON from get_setting as we run instantiate Addon() anyway.

Doesn't look nice (very Kodi-specific) but would be most efficient. It really sucks that there is no proper way to re-read the settings :-/

Which brings me to something I was playing with.

  • Cache add-on settings in Windows properties
  • Update cached settings from service.py when changed

In this case we wouldn't need to keep the cache (Windows properties) fresh or have a ttl mechanism. We could perform actions when specific settings change (a more generic solution for the credentials changes) and this would be most efficient.

It does add to the complexity of the add-on though...

@mediaminister
Copy link
Collaborator

So moving ADDON = Addon() into the global context, the performance problems are solved.

But is it slower than with KodiWrapper?

@dagwieers
Copy link
Collaborator Author

I doubt it. I haven't profiled on Kodi itself, only tested the solution after profiling locally.
It is easier for me to do local testing on (Friday) evenings.

@dagwieers
Copy link
Collaborator Author

And I guess I was lucky that my stub implementation showed a similar behaviour as xbmcaddon, because I don't think it's exactly the same issue, but a similar overhead pattern. The fix I propose does not fix it for the stubs 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants