Faster Cache #121
Faster Cache #121
Conversation
Codecov Report
@@ Coverage Diff @@
## master #121 +/- ##
==========================================
+ Coverage 87.98% 88.24% +0.26%
==========================================
Files 44 42 -2
Lines 1190 1191 +1
Branches 164 140 -24
==========================================
+ Hits 1047 1051 +4
- Misses 97 100 +3
+ Partials 46 40 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
You can actually see the negative effect in the Doorman coverage here as all our small number of requests are entering the read lock before the write lock is established. It seems to me we need a single lock which is upgradeable. Any and all advice is most welcome. |
|
Have you considered trying a lock free (ish because of the Lazy) implementation? Something like |
|
@deanmarcussen I don't quite follow I'm afraid. |
Sorry not well explained - late on a Friday. Easier to show than explain. Here's a working branch for ideas, not stress tested, or perf tested. The idea is to maintain a thread safe dictionary of lazy tasks, rather than use a lock. Ignore if you don't like https://github.com/ThisNetWorks/ImageSharp.Web/tree/lock-free |
|
@deanmarcussen More me than you mate, I'm not firing on all cylinders. Thanks for having a look! I'd love to go lock free actually! Might be best to base any changes on this branch though as I've changed the cache lookup method to speed it up. Though if I’m reading your code right your changes would negate the need for an lru cache. |
@JimBobSquarePants Trying to get time to look at this properly today. From pulling this branch it looks like the LRU cache is using similar concepts to what I was proposing for the AsyncLock, but to achieve a different result. So to clarify, is the LRU is to provide cached access to source metadata and source resolver - not the actual stream but just the resolver? I think you would still want threadsafe locklike behaviour read around the cache, and write safe when not in the cache? I'll try and update based of this branch later today |
|
Thanks @deanmarcussen
They cache both resolvers and the metadata returned. Reading values from the cache is actually fairly IO intensive and by caching the result in an expiring LRU cache I can cut that all out.
Yeah, I reviewed and still think we need that. The current read/write lock is allowing for too many queued writes though. Basically if I have 10 requests sent at the same time the first 7 end up processing the file and the last 3 (plus any future requests) use the cached result. This is in extreme (artificial) circumstances of course so I might be worrying about nothing but I still don't like it. |
|
New branch @JimBobSquarePants https://github.com/ThisNetWorks/ImageSharp.Web/tree/dm/lock-free-cache What's the best way to test this? Draft PR or will you run it? I see options for improving it - the Lazy is useful on the Writer, to prevent multiple writes, but may not be necesary on the Reader. But worth seeing if it has anything to gain over your existing
I'm not sure where this race is, and if what I've done achieves anything about it But with this, we should see only one write occuring (at least I hope so.) They could potentially merge into one set of Workers, which produce a func for reading (then cached), and perform the write if not read? Still getting my head around the LRU cache ;) |
|
Thanks @deanmarcussen I'll have a look tonight. This test is useful for tracking reads and writes. I just debug it and add some break points. Comment out the AzureTestImage theory data if you want to just test the physical provider. |
|
If you need me to run a benchmark let me know which branch |
|
@deanmarcussen Just pulled down your branch and all tests are passing. In addition I also saw the correct number of writes whic was awesome. Cranked up the numbers and everything still looks good. Curious to see how it profiles. |
|
Whew! And cool. I need to tweak something tomorrow I think. The write func should probably check the read dictionary for operations and await it as well. Let's see if we can bench it after :) |
|
@JimBobSquarePants Pushed the tweak I wanted to. The test you pointed out has been quietly running this morning, with some tweaks to process 10+ images of a larger size (10MB to a width of 200px), and either a new image (cache busted) or the same image from the cache (every odd number task). I can't fault it. In terms of performance this is not a great way to bench it, but I applied the same test run to this branch here four or five times and the test run averages around 43s. On my branch it drops to around 15s per test run! MacOS i7 Quad Core 24GB |
|
@deanmarcussen Can you open a PR targeting this branch then? I'll merge it then @sebastienros will be able to profile this branch and tell us what the numbers are. |
Lock free Read/Write Identical Request Implementation
|
@sebastienros This branch |
|
🎉 It's faster than static files, at least on a cached file. Went from 2K to 60K, with super low latency too. |
Holy Moly! @deanmarcussen I owe you any beverage of your choice! @sebastienros 0.1% errors is fine by me just now. We can diagnose at another time. |
Mind blown - don't know what I was expecting, but nothing like that. Awesome :) |
|
Let us know when this is on nuget so we can test it in OC. |
|
It's on MyGet just now. I'll push to NuGet in the next day or two once I've double checked a few other things. https://www.myget.org/feed/sixlabors/package/nuget/SixLabors.ImageSharp.Web/1.0.0-rc.3.16 |
Prerequisites
Description
Introduces a time aware least recently used cache to store results of cache lookups. This removes most of the IO required to check whether an image is in the cache for subsequent requests for the same URL.
I'm extremely concerned about the way our AsyncKeyLock works now though. Cache lookups are now so fast we're getting misses as the next request for the same URL is attempting to read the cache. This leads to multiple writes which are safe but inefficient.