Convert LFU to LRU cache for blob storage #107130
Conversation
|
Pinging @elastic/es-search (Team:Search) |
|
@elasticmachine run elasticsearch-ci/part-3 |
There was a problem hiding this comment.
Nice work! At a high level, this looks good - though I'm quite interested in what others say. I'm not qualified to approve, so my comments should not block - feel free to take them as ideas/suggestions :)
There are a few ideas that came to mind which are well into the realm of over-optimization. I'd be interested to see how this performs in the micro-benchmarks to see if further optimization is warranted, however.
On that topic, it seems like many of the operations are conceptually concurrent. For instance, we should be able to force-evict swaths of entries concurrently while also adding and removing to the queues concurrently. I'm not sure if the coordination costs are worth it, though (not to mention the code complexity).
I also wonder if there are things that could be done to improve reading/writing back and forth to main memory from the cpu caches (by storing the parts of the data structure which change most in such a way that they can stay cached), or alternatively by bulk updating (waiting for 100 writes to the "front" list before moving 100 entries to the back list all at once with a single set of pointer updates, etc).
But like I said, without benchmarks to show that any of this needs improving, I think it looks great.
| } | ||
| } | ||
| } | ||
|
|
||
| private void unlink(final LFUCacheEntry entry) { | ||
| private void unlink(final LRUCacheEntry entry) { |
There was a problem hiding this comment.
WDYT about testing this explicitly under the following conditions:
entry.listhas one entry, which weunlink()entry.listhas one 2 entries, and weunlink()the firstentry.listhas one 2 entries, and weunlink()the lastentry.listhas one 3 entries, and weunlink()the middle
Maybe this is already done via the other tests in a transitive manner, as I didn't read through all the tests in detail. However, I noticed that neither this function, nor any of the methods that call this function, are under test. (I'm bit paranoid, but trust me I've earned it...)
| assert invariant(entry, true); | ||
| } | ||
|
|
||
| private void pushEntryToFront(final LRUCacheEntry entry) { |
There was a problem hiding this comment.
Along with pushEntryToMiddle/Back(), this seems like the kind of thing that would be good to explicitly test. (Maybe this is already satisfactorily tested through transitive method calls, I confess I didn't read the entirety of the test files.)
I was thinking something along the lines of starting with an empty list, adding up to two entries, and for each added entry asserting that the pointers are correct.
We'd also want a test that sets the size to a low value, and checks that the entries are pushed to the middle list.
WDYT? Is that overkill? Is this behaviour already tested?
There was a problem hiding this comment.
AFAIK this should be tested transitively. I think it's preferable to keep the LRUCache classes private if possible. They also happen to be non-static so are tied to an instance of SharedBlobCacheService.
|
@original-brownbear I added back the LFU cache. It's possible to switch between LFU and LRU with a new additional setting. LRU is the default. I also separated out the tests for LFU and LRU into separate files which accounts for the dramatic change in line count. |
There was a problem hiding this comment.
This looks quite good thanks Jack! I'm wondering if we shouldn't go a little further here though and move to a more lock-free implementation if possible.
If any promotion to the head of the queue requires acquiring a global lock, that's not ideal. Maybe we could just make the moving of an item to the head of the queue lock-free for now, since that's what really matters for read performance, and leave the rest as is for maybe looking into later? WDYT?
| assert invariant(entry, true); | ||
| } | ||
|
|
||
| private void pushEntryToFront(final LRUCacheEntry entry) { |
There was a problem hiding this comment.
I think there should be a way for us to not need this kind of lock any longer. Can't we insert
Because it's basically: point new node at current head, set head to new node, but only if current head is still what we just pointed at (CAS)? No need for locking is there?
And then basically precede that whole operation by evicting the last element in the cache in case we insert a new page instead of just cycling a page to the front?
There was a problem hiding this comment.
Let's chat about this again and if it's still relevant once we start talking about using time.
There was a problem hiding this comment.
Thinking back at past discussions, it seems lock-freedom was a key motivation for going to an LRU so would be good to proof out that this works as part of this (POC?).
|
@original-brownbear @JVerwolf I have removed the middle list from this PR. |
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
This changes our approach to caching for blob storage. This converts the least frequently used cache to a much simpler least recently used cached. The new cache has two lists called
frontandmiddle. New entries are inserted into themiddlelist to avoid thrashing. Any entry accessed another time is moved to the head of thefrontlist. Only the last entry of themiddlelist is checked for eviction when the cache becomes full avoiding degenerate cases of moving through the entirety of the lists as it's very unlikely that anything being currently written to will make it to the end of themiddlelist.