LRU cache utility #1090

chfast · 2024-12-23T10:13:46Z

Adds generic single-threaded implementation of
the least recently used (LRU) cache.

This is to be used to cache the code and code analysis.

codecov · 2024-12-23T10:17:32Z

Codecov Report

Attention: Patch coverage is 72.96296% with 73 lines in your changes missing coverage. Please review.

Project coverage is 93.98%. Comparing base (81c9dda) to head (2938377).

Files with missing lines	Patch %	Lines
test/internal_benchmarks/lru_cache_bench.cpp	0.00%	70 Missing ⚠️
lib/evmone/lru_cache.hpp	93.93%	2 Missing ⚠️
test/unittests/lru_cache_test.cpp	99.40%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1090      +/-   ##
==========================================
- Coverage   94.31%   93.98%   -0.33%     
==========================================
  Files         159      162       +3     
  Lines       17221    17491     +270     
==========================================
+ Hits        16242    16439     +197     
- Misses        979     1052      +73

Flag	Coverage Δ
eof_execution_spec_tests	`23.25% <0.00%> (-0.37%)`	⬇️
ethereum_tests	`26.22% <0.00%> (-0.41%)`	⬇️
ethereum_tests_silkpre	`18.62% <0.00%> (-0.33%)`	⬇️
execution_spec_tests	`20.63% <0.00%> (-0.33%)`	⬇️
unittests	`88.83% <73.50%> (-0.24%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
test/unittests/lru_cache_test.cpp	`99.40% <99.40%> (ø)`
lib/evmone/lru_cache.hpp	`93.93% <93.93%> (ø)`
test/internal_benchmarks/lru_cache_bench.cpp	`0.00% <0.00%> (ø)`

Adds generic single-threaded implementation of the least recently used (LRU) cache. This is to be used to cache the code and code analysis.

rodiazet

I sadly can't complain about anything. Let's ask Copilot then . :)

gumb0 · 2025-01-14T17:20:09Z

lib/evmone/lru_cache.hpp

+
+    using LRUList = std::list<LRUEntry>;
+    using LRUIterator = typename LRUList::iterator;
+    using Map = std::unordered_map<Key, LRUIterator>;


Would it be possible/better to store values in the map entries? Then there'd be just one link LRUEntry => MapNode.
Now it's double-direction: LRUEntry::key => MapNode::key and MapNode::value -> LRUEntry

Ah I see a comment below that there's no performance difference. But it seems more convoluted to me.

Yes, this is possible. Then the LRU list has only the pointer/reference/iterator to the map. And the map is Key => (LRUIterator, Value).

I'm kind of on your side here, but because the performance difference was negligible I decided to keep the "classic" layout for now. There is also small nuance: you cannot have std::list<const Key&> so you need to wrap the reference or use a pointer.

This is to be revisited in case we are going to use intrusive list and/or map iterators.

gumb0 · 2025-01-14T17:26:45Z

lib/evmone/lru_cache.hpp

+    Map map_;
+
+    /// Marks an element as the most recently used by moving it to the back of the LRU list.
+    void move_to_back(LRUIterator it) noexcept { lru_list_.splice(lru_list_.end(), lru_list_, it); }


Nit: the name kinda leaks internal representation (order of elements in the list). Maybe better something like touch(), use(), access() ?

Actually I didn't notice it's a private method, then it's fine

I think I thought about it, e.g. naming it bump_usage(). In the end I decided not be because we also use lru_list_ directly in other places. E.g. we should also replace ru_list_.emplace with something like new_use()?

The main motivation was to wrap splice() which takes me usually some time to figure out.

gumb0 · 2025-01-15T15:19:47Z

lib/evmone/lru_cache.hpp

+            auto node = map_.extract(lru_it->key);
+            swap(node.key(), key);
+            if (auto [it, inserted, node2] = map_.insert(std::move(node)); !inserted)
+            {
+                // Failed re-insertion means the element with the new key is already in the cache.
+                // Rollback the eviction by re-inserting the node with original key back.
+                swap(key, node2.key());
+                map_.insert(std::move(node2));
+
+                // Returned iterator points to the element matching the key
+                // which value must be updated.
+                lru_it = it->second;
+            }
+            lru_it->value = std::move(value);  // Replace/update the value.
+            move_to_back(lru_it);


Juggling of keys is quite confusing, I'd maybe add more comments like

Suggested change

auto node = map_.extract(lru_it->key);

swap(node.key(), key);

if (auto [it, inserted, node2] = map_.insert(std::move(node)); !inserted)

{

// Failed re-insertion means the element with the new key is already in the cache.

// Rollback the eviction by re-inserting the node with original key back.

swap(key, node2.key());

map_.insert(std::move(node2));

// Returned iterator points to the element matching the key

// which value must be updated.

lru_it = it->second;

}

lru_it->value = std::move(value); // Replace/update the value.

move_to_back(lru_it);

auto node = map_.extract(lru_it->key); // node.key() is LRU key

swap(node.key(), key); // node.key() is new key, key is LRU key

if (auto [it, inserted, node2] = map_.insert(std::move(node)); !inserted)

{

// node2.key() is new key (already in cache)

// Failed re-insertion means the element with the new key is already in the cache.

// Rollback the eviction by re-inserting the node with original key back.

swap(key, node2.key()); // key is new key (already in cache), node2.key() is LRU key

map_.insert(std::move(node2));

// Returned iterator points to the element matching the key

// which value must be updated.

lru_it = it->second;

}

lru_it->value = std::move(value); // Replace/update the value.

move_to_back(lru_it);

Yeah, this value update for existing key feature is quite annoying to implement. And in many cases it is never used... I will try to improve the comments.

chfast

I probably should mention O(1) get/put complexity.

chfast · 2025-01-15T15:32:20Z

lib/evmone/lru_cache.hpp

+    Map map_;
+
+    /// Marks an element as the most recently used by moving it to the back of the LRU list.
+    void move_to_back(LRUIterator it) noexcept { lru_list_.splice(lru_list_.end(), lru_list_, it); }


I think I thought about it, e.g. naming it bump_usage(). In the end I decided not be because we also use lru_list_ directly in other places. E.g. we should also replace ru_list_.emplace with something like new_use()?

The main motivation was to wrap splice() which takes me usually some time to figure out.

chfast · 2025-01-15T15:34:53Z

lib/evmone/lru_cache.hpp

+            auto node = map_.extract(lru_it->key);
+            swap(node.key(), key);
+            if (auto [it, inserted, node2] = map_.insert(std::move(node)); !inserted)
+            {
+                // Failed re-insertion means the element with the new key is already in the cache.
+                // Rollback the eviction by re-inserting the node with original key back.
+                swap(key, node2.key());
+                map_.insert(std::move(node2));
+
+                // Returned iterator points to the element matching the key
+                // which value must be updated.
+                lru_it = it->second;
+            }
+            lru_it->value = std::move(value);  // Replace/update the value.
+            move_to_back(lru_it);


Yeah, this value update for existing key feature is quite annoying to implement. And in many cases it is never used... I will try to improve the comments.

chfast · 2025-01-15T15:42:56Z

lib/evmone/lru_cache.hpp

+
+    using LRUList = std::list<LRUEntry>;
+    using LRUIterator = typename LRUList::iterator;
+    using Map = std::unordered_map<Key, LRUIterator>;


Yes, this is possible. Then the LRU list has only the pointer/reference/iterator to the map. And the map is Key => (LRUIterator, Value).

I'm kind of on your side here, but because the performance difference was negligible I decided to keep the "classic" layout for now. There is also small nuance: you cannot have std::list<const Key&> so you need to wrap the reference or use a pointer.

This is to be revisited in case we are going to use intrusive list and/or map iterators.

chfast force-pushed the lru_cache branch 7 times, most recently from 013084f to 738e0d7 Compare December 23, 2024 14:10

LRU cache utility

2938377

Adds generic single-threaded implementation of the least recently used (LRU) cache. This is to be used to cache the code and code analysis.

chfast force-pushed the lru_cache branch from 738e0d7 to 2938377 Compare December 23, 2024 14:16

chfast requested review from gumb0 and rodiazet December 23, 2024 14:17

rodiazet approved these changes Jan 14, 2025

View reviewed changes

gumb0 reviewed Jan 14, 2025

View reviewed changes

gumb0 reviewed Jan 15, 2025

View reviewed changes

chfast commented Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LRU cache utility #1090

LRU cache utility #1090

chfast commented Dec 23, 2024

codecov bot commented Dec 23, 2024 •

edited

Loading

rodiazet left a comment

gumb0 Jan 14, 2025

gumb0 Jan 14, 2025

chfast Jan 15, 2025

gumb0 Jan 14, 2025

gumb0 Jan 15, 2025

chfast Jan 15, 2025

gumb0 Jan 15, 2025

chfast Jan 15, 2025

chfast left a comment

chfast Jan 15, 2025

chfast Jan 15, 2025

chfast Jan 15, 2025

LRU cache utility #1090

Are you sure you want to change the base?

LRU cache utility #1090

Conversation

chfast commented Dec 23, 2024

codecov bot commented Dec 23, 2024 • edited Loading

Codecov Report

rodiazet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 23, 2024 •

edited

Loading