Reimplement lru with python dictionary insertion order #16
Conversation
abidsikder
temporarily deployed
to
Pages Preview
GitHub Actions
Inactive
WalkthroughThis pull request introduces changes to the HAMT (Hash Array Mapped Trie) implementation, focusing on simplifying the cache management mechanism. The modifications include updating the cache data structure, removing methods related to timestamp tracking, and introducing a new LRU (Least Recently Used) cache eviction strategy. The project version has been bumped to 2.0.4, and the Changes
Sequence DiagramsequenceDiagram
participant HAMT
participant Cache
HAMT->>Cache: write_node()
Cache->>Cache: Insert node
Cache->>Cache: Perform LRU eviction
HAMT->>Cache: read_node()
Cache-->>HAMT: Return node
Cache->>Cache: Reinsert node (mark as most recent)
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
py_hamt/hamt.py (1)
214-220: Elegant use of Python's dictionary insertion order for LRU implementation!The implementation cleverly leverages Python 3.7+'s guaranteed dictionary insertion order, making the code simpler and more maintainable while removing the dependency on sortedcontainers.
Note: This implementation assumes Python 3.7+ (which is fine given the requires-python >= 3.12 in pyproject.toml).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
py_hamt/hamt.py(1 hunks)pyproject.toml(1 hunks)
🔇 Additional comments (4)
py_hamt/hamt.py (3)
225-226: LGTM! Clean and straightforward cache update logic.The implementation maintains the LRU invariant by adding new nodes as the most recently used entries.
231-244: Well-structured cache hit/miss handling!The implementation properly maintains the LRU order by:
- On cache hit: Re-inserting the node to mark it as most recently used
- On cache miss: Loading from store and adding to cache with eviction check
252-252: Nice readability improvement!Using underscore separator in the numeric literal (10_000_000) improves readability while maintaining the same 10MB default cache size.
pyproject.toml (1)
3-3: Version bump and dependency cleanup look good!The version bump to 2.0.4 is appropriate for the LRU implementation change, and removing the sortedcontainers dependency aligns with the new implementation using Python's built-in dictionary ordering.
This is meant to address #15 by removing the problematic library entirely, while keeping our LRU cache eviction strategy, in an even simpler method.
In 10 runs with the old and new implementation
uv run pytest -s performance_tests/cache_with_ipfsstore.py, we got the following data, for % improvement over no cache:While the averages are different, I am not confident in saying that this is conclusive evidence of a substantial performance, it may be noise. But I think there is enough data to show that we are at least not getting substantially worse, and for a PR that eliminates both a bug, a dependency, and a bunch of code, that's pretty great.
Perform an AI-assisted review on
Summary by CodeRabbit
New Features
Bug Fixes
Chores
sortedcontainersdependency