ShokoAnime / Shokofin

Repository for Shokofin, a plugin that brings Shoko to Jellyfin.
https://shokoanime.com
MIT License
140 stars 17 forks source link

Fixes potential race condition and reduces memory allocations #54

Closed MarkCiliaVincenti closed 4 months ago

revam commented 4 months ago

Also, while I am skeptical about PRs coming out of nowhere, I also do appreciate the PR(s).

MarkCiliaVincenti commented 4 months ago

OK let me briefly explain the potential race condition that exists.

Thread A locks on key 123. Thread B gets the semaphore for key 123 and waits on thread A. Thread A finishes and releases on the semaphore but also removes it from the dictionary. Thread C comes in also trying to get in for key 123, but now there's nothing in the dictionary despite Thread B still running. Now thread B and C are running concurrently.

You're right that I haven't tested this. I should have mentioned that. I have little doubt it will work though. Please do test it.

And sort of new to jellyfin plugins but in a way no since I had authored a plugin (BlurN) for Emby a few years back, from which jellyfin was forked.

As for the library, it's by far the most popular key locking library around, used by Microsoft (kiota, Vipr, apiops), jellyfin, EFCoreSecondLevelCacheInterceptor on which jellyfin also depends, and many more.

revam commented 4 months ago

Simply switching the target branch didn't seem to rebase it properly…

revam commented 4 months ago

Now it's at the right branch at least. I'll test it out tomorrow if it has an impact on performance (be it good or bad, but hopefully good).

revam commented 4 months ago

I removed the band-aid I previously applied to cope with the race conditions, since hopefully it won't be needed if the race conditions are fixed now.

MarkCiliaVincenti commented 4 months ago

Now it's at the right branch at least. I'll test it out tomorrow if it has an impact on performance (be it good or bad, but hopefully good).

My expectations are that it would be marginally slower (because it's doing more stuff, and there's no race condition), but with lower memory allocations.

fearnlj01 commented 4 months ago

For reference, I've done some performance testing just in regards to speed... For the most part, any slowdown looks to be within reason, aside for the one anomalous intial scan.

I note that differences in memory usage has not been measured by these tests.

For each 'action' of the tests, Jellyfin was restarted to replicate worst-case scenarios where there is no cached data.

Results are as below, using a library of 1625 episodes & 132 series:

Using the PR

Initial Scan Refresh All Metadata Scan Media Library
90s 55s 21s
57s 62s 28s
69s 56s 23s
59s 59s 23s
63s -- --
56s -- --

Using the current head of the 10.9 branch

Initial Scan Refresh All Metadata Scan Media Library
57s 59s 31s
56s 59s 24s
59s 55s 22s
59s 57s 21s
56s -- --
MarkCiliaVincenti commented 4 months ago

Cool, ideally for benchmarking you'd use a tool like BenchmarkDotNet though.