Add SIEVE eviction policy#1226
Conversation
✅ Deploy Preview for salsa-rs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Merging this PR will degrade performance by 12.25%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
9f203e8 to
4ac2194
Compare
|
The concurrent benchmarks look promising. But I need to do a pass over the benchmarks. I don't think they really measure what they should |
4ac2194 to
1188c65
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
45db58f to
3253ca1
Compare
3253ca1 to
21bce20
Compare
|
Okay, I think this is ready. The implementation is a bit more involved because I wanted to minimize overhead and maximize throughput in concurrent use cases. |
Summary
This adds a new SIEVE eviction policy (
eviction(policy = sieve, capacity = 4000)). It is optimized for throughput and should, for most use cases, be more efficient (picking the right items to evict) than LRU. I picked this new genericevictionkeyword so that we can add other eviction policies in the future (generational, within same revision, etc)I decided to remove support for
lru(1000), I'd be okay restoring the syntax but, given the other breaking changes that we made recently, this feels minimal and it also hints users towards using sieve over LRUCloses #1032
Benchmarks
Codspeed benchmarks side to side
The only case where SIEVE is slower is
scan_resistance. This is not unexpected, because SIEVE is not scan resident. This is where an algorithm like W-Tiny-LFU would show better results (at the cost of higher computations in the fast path etc)Running ty on homeassistant:
Now, we only use eviction for parsed_module, which hasn't the most hits and is itself a fairly heavy query. I did the same with
infer_expression_types, which we have more instances of and is also a much lighter query (does very limited work). Here, the overhead becomes noticeable, but is still close to noise:My read is that this is mainly due to the exclusive lock on the insertion path and a limit of 10k is probably also way too low, meaning we push a lot of instances into the eviction queue.
infer_expression_typesis also a much better fit for within-revision GC, because recomputation is so cheap, that it's rarely worth caching. Except for the few instances that are very expensive (expensive control flows, cycle, ...). But that's another side project, which hopefully will build on top of the SIEVE work in this PRAn even more extreme example from ty (
semantic_index)