Closed metanivek closed 1 year ago
To expand a bit on my comments, did you consider keeping the hashtables separate (since they cause type issues), but sharing the Q.t
list across the different LRUs? (so that they have the ability to make room from other hashtables when needed)
The type q: (key * 'a) Q.t
is an issue, but I believe it could be replaced by q: (unit -> unit) Q.t
instead (a closure to free an element from whichever hashtable it was in and subtracting the removed weight from the total used space...)
(Note that this is just a thought that popped into my head! I don't think there's a strong incentive to do things differently as the current solution works nicely :) )
Ah, thanks for sharing your extra thoughts! I didn't think about changing the core LRU implementation too much since I wanted to keep options of switching it in the future (maybe a modified cachecache
or some lock-free version, for instance). If it looks too complicated to bring the extensible type into the LRU, I think we should push it out to a future time when we evaluate the actual LRU implementation.
At a high-level, this PR:
irmin-pack
into oneWhen deciding how to count the memory usage of objects in the LRU, two paths were evaluated: using
Obj.reachable_words
or usingrepr
's size function. The former was too slow in replay benchmarks, so that latter is used. The size is used as-is for commits and contents, and a correction factor, based on benchmark observations, is applied to inode sizes.The definition of "large" for contents objects is 20kB. This seems like a reasonable value based on previous analysis of object size which indicated that less than 0.1% of contents objects are larger than 512B. Note: the previous weight based limit would allow any object < ~500kB into the LRU (based on Octez's default configuration of 5000 entry cap), but lowering the cap allows more objects into the cache.
Conclusions based on benchmarks (to be pushed to the benchmarks repo once I finish tidying the analysis and running a final bench):
cachecache
's LRU for this, but with some modifications to its code, it may provide better scaling. I ran its benchmarks against the modifiedirmin
LRU in this PR, and it does seem to scale better.The final benchmark I want to run is an 80mb, but here is some high-level analysis.Snippet of benchmark stats:
Here is memory usage comparisons. You can see:
entry-100k
and500mb
regress in performance (not shown isentry-60k
but it also regresses in perf)entry-30k
has the best performance (but 80mb is almost the same in perf and memory)entry-15k
has almost equivalent memory usage as3.7.1
but better performance