Allow cache to manage entries of varying size #1434
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1434 +/- ##
==========================================
- Coverage 89.63% 89.63% -0.01%
==========================================
Files 343 343
Lines 29951 29997 +46
Branches 3315 3322 +7
==========================================
+ Hits 26847 26887 +40
- Misses 1957 1959 +2
- Partials 1147 1151 +4 ☔ View full report in Codecov by Sentry. |
|
| _totalSizeNonPinned = | ||
| MemorySize::bytes(_totalSizeNonPinned.getBytes() - | ||
| sizeInMap.getBytes() + newSize.getBytes()); |
There was a problem hiding this comment.
| _totalSizeNonPinned = | |
| MemorySize::bytes(_totalSizeNonPinned.getBytes() - | |
| sizeInMap.getBytes() + newSize.getBytes()); | |
| _totalSizeNonPinned = _totalSizeNonPinned- sizeInMap + newSize; |
- a short comment that we cannot use
+=becauseMemorySizecan't (even temporarily) undeflow.
| MemorySize::bytes(_totalSizeNonPinned.getBytes() - | ||
| sizeInMap.getBytes() + newSize.getBytes()); | ||
| sizeInMap = newSize; | ||
| if (_totalSizePinned <= _maxSize) { |
There was a problem hiding this comment.
The if is completely redundant and can be removed.
| using Vec = std::vector<int>; | ||
| [[maybe_unused]] auto vectorSizeGetter = [](const auto& pointer) { | ||
| return pointer->size() * sizeof(int) * 1_B; | ||
| }; | ||
|
|
There was a problem hiding this comment.
All such global stuff in .cpp files should be in the anonymous namespace { .... }.
| ASSERT_TRUE(cache.contains(1)); | ||
|
|
||
| cache.recomputeSize(1); | ||
|
|
There was a problem hiding this comment.
Document here, why exactly which element is evicted.
| /// and apply the diff to the internal size records. If the value grew too big | ||
| /// for the cache in the meantime it is removed, if it grew but fits in the | ||
| /// cache other values might be evicted instead. In case the value shrinks it | ||
| /// is also removed because then it will be incomplete so it no longer makes | ||
| /// sense to cache it. |
There was a problem hiding this comment.
Clarify that if it suffices, then other entries are evicted, only if this wouldn't suffice, the current entry itself is erased.
| erase(key); | ||
| return; | ||
| } |
There was a problem hiding this comment.
It seems like the value has shrunk, delete it case is missing!
Breakout part of #1350