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

Move imports to where they are being used #198

Merged
merged 1 commit into from
Nov 6, 2019

Conversation

dagwieers
Copy link
Collaborator

May be controversial, but if we want to keep inputstreamhelper's
footprint as small as possible to add-ons using it, this may help.

To be discussed.

@dagwieers dagwieers added this to the v0.5.0 milestone Sep 25, 2019
@dagwieers
Copy link
Collaborator Author

dagwieers commented Sep 25, 2019

My main objection to this PR would be that restructuring the code would make things a lot easier and could provide the same benefits. (xbmcgui being an obvious one)

Some of the things we move out of the global scope are actually things we need all over the place, however that may be true for inputstreamhelper, but not necessarily for the add-on importing inputstreamhelper.

We should probably advise add-on developers to only import inputstreamhelper when they need it, rather than globally as well.

@dagwieers
Copy link
Collaborator Author

A fix for xbmcgui is in #202

@dagwieers dagwieers marked this pull request as ready for review October 7, 2019 23:19
@dagwieers dagwieers force-pushed the selective-imports branch 3 times, most recently from 9bb1c4f to 2810c0e Compare November 6, 2019 10:08
@michaelarnauts
Copy link
Contributor

I'm doing this myself sometimes, but has somebody actually benchmarked this if this really is faster?

@dagwieers
Copy link
Collaborator Author

dagwieers commented Nov 6, 2019

@michaelarnauts It makes a difference on the very first startup (when reuselanguageinvoker is used) and on every interaction (when reuselanguageinvoker is not used). When importing urllib3 (when it took 4 seconds) this difference was very apparent and that is when we started using reuselanguageinvoker. On Raspberry Pi this effect is very real.

The VRT NU startup time is a few 100ms, whereas typical add-ons take a second or more. You can feel the difference, and see it from the default spinner (for VRT NU the spinner doesn't even spin). The difference between VRT NU and VTM GO is still there. With the newer upcoming urllib3 this difference will become smaller.

For a utility like inputstreamhelper we shouldn't be adding anything to the start-up time if that is not essential for its use IMO. That is why we dropped urllib3 (some add-ons avoid urllib3 anyway) and why we avoid importing things that are not used. (e.g. kodi-six)

And then we also have #89 to see how we can reduce the overhead of using inputstreamhelper when it is being used.

@dagwieers
Copy link
Collaborator Author

Benchmarks on Raspberry Pi 3 are available from:

The upcoming urllib3 release will bring the current import times of urllib3 from 800ms (v1.25.3) to 350ms (v1.25.6) on Raspberry Pi.

@dagwieers
Copy link
Collaborator Author

dagwieers commented Nov 6, 2019

Also take note that in Kodi Matrix (Python 3) reuselanguageinvoker has been removed. So the current startup time will become the interaction time. xbmc/xbmc#16116 (comment)

@michaelarnauts
Copy link
Contributor

Also take note that in Kodi Matrix (Python 3) reuselanguageinvoker has been removed. So the current startup time will become the interaction time. xbmc/xbmc#16116 (comment)

This is terrible news, but by looking at your linked issue, it was not removed on purpose, so it might come back?

@dagwieers
Copy link
Collaborator Author

dagwieers commented Nov 6, 2019

@michaelarnauts Hmm, I missed that. I get the mails from that thread but didn't link the responses to reuselanguageinvoker`. That is good news!

Update: It has not been reverted yet though, so Matrix is still affected. Should be quite noticeable on Matrix on RPi.

May be controversial, but if we want to keep inputstreamhelper's
footprint as small as possible to add-ons using it, this may help.
@dagwieers
Copy link
Collaborator Author

BTW I also updated the Wiki to advise developers to import inputstreamhelper locally and not globally.

https://github.com/emilsvennesson/script.module.inputstreamhelper/wiki/Integration

@dagwieers
Copy link
Collaborator Author

Ready to be merged.

@mediaminister mediaminister merged commit 121aca1 into emilsvennesson:master Nov 6, 2019
@dagwieers dagwieers modified the milestones: v0.5.0, v0.4.4 May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants