lekma / plugin.video.invidious

Invidious Addon for Kodi
GNU General Public License v3.0
50 stars 7 forks source link

perf: multi thread feed requests #86

Closed SethFalco closed 1 year ago

SethFalco commented 1 year ago

On my RockPro64, the feed was loading very slowly. With 26 subscriptions, it was taking somewhere between 20 and 60 seconds.

This throws in a ThreadPoolExecutor to add concurrency. Unfortunately, it's just threads, I'm uncertain if it's possible to utilize the benefits of asyncio on Kodi. [reference]

However, with this, updating the feed is closer to 6-15 seconds~ on my device. :+1:

Please refer to my docstrings for more details on:

Notes

lekma commented 1 year ago

I like it a lot. I'll try to merge, test and release asap. Thanks

SethFalco commented 1 year ago

Sorry if you already started reviewing it, just did a very minor change!

- if self.__workers__:
+ if self.__workers__ and self.__workers__ > 1:

Forgot, I set workers to os.cpu_count() - 1. so if the device has 2 threads, it'll have a value of 1.

But ofc there's no point using a thread pool, but only setting it to 1 worker, so I make that fallback to the synchronous loop as well. :laughing:

lekma commented 1 year ago

@SethFalco: would you be so kind and test current master and report on the performance (better/worse/same)?

It should be the same except on subsequent calls to the first one (should be better on these).

I removed the cpu count calculation (I prefer to let the module use its default and 5 threads on a mono core/processor should still give a little boost on overlapping io).

I still don't know if we should lock around notify() in the session (I don't know enough about kodi and its thread safety, and try as I might I couldn't create a race condition on notify()).

thanks in advance for your help

SethFalco commented 1 year ago

Yeah sure, I'll get master installed tonight and try it out!

And damn, good catch on the default value. :o I didn't know it had one tbh, and it's a bit more generous than what I was doing. ^-^'

Default value of max_workers is changed to min(32, os.cpu_count() + 4).

https://docs.python.org/3/library/concurrent.futures.html

This will be nice!

SethFalco commented 1 year ago

@lekma Haven't used it extensively, but can respond with some stats from both my laptop and RockPro64.

Laptop (4 cores / 8 threads)

I was on vid.puffyan.us originally, but when updating the feed, it would throw errors due to timeouts. I'm just going to assume that the server is overloaded for now. :thinking:

Using y.com.sb with 32 subscriptions, everything worked perfectly.

Between master and HEAD~1, there was actually a negligible difference, both being around 5-6 seconds.

RockPro64 (4 slow cores + 2 fast cores)

Using y.com.sb with 30 subscriptions, everything worked perfectly.

Similarly to above, both being around 6-7 seconds.


In practice, it seems with the environments/subscriptions above, it's about the same. I didn't monitor anything else, like resource utilization.

I can drop a follow-up later if through casual use my experience differs.