Improve rootkey cache in pruning: Reduce 4 rootkey fetches to 1 #1029
Conversation
WalkthroughThe pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant NodeDB
participant RootKeyCache
participant Database
Client->>NodeDB: Request Root Key
NodeDB->>RootKeyCache: Check Cache
alt Cache Hit
RootKeyCache-->>NodeDB: Return Cached Root Key
else Cache Miss
NodeDB->>Database: Fetch Root Key
Database-->>NodeDB: Return Root Key
NodeDB->>RootKeyCache: Store in Cache
end
NodeDB-->>Client: Return Root Key
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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)
nodedb.go (1)
726-726: Reinforce code clarity via descriptive variable names.The local variable
rootkeyCachemight be renamed torkcorcacheto maintain consistent naming across the file, but this is a minor style preference.- rootkeyCache := newRootkeyCache() + rkc := newRootkeyCache()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nodedb.go(6 hunks)
🔇 Additional comments (7)
nodedb.go (7)
419-424: Introduce concurrency considerations or confirm locking assumptions.
The rootkeyCache struct maintains a minimal two-element direct-mapped cache for version → rootKey lookups. While this design is efficient for its limited scope, ensure that concurrent access is safeguarded by the enclosing nodeDB's lock to prevent data races on versions, rootKeys, and next.
Would you like a verification script to confirm that all writes to this cache are protected by nodeDB.mtx?
428-434: Efficient direct lookups with manageable complexity.
The getRootKey method properly checks both cache positions before falling back to the DB fetch. This approach is consistent with a simple two-element cache design and should be performant for the intended usage.
444-448: Clear cycling logic, but watch for potential collisions.
Storing the version and rootKey in rkc.versions[rkc.next], then cyclically advancing rkc.next effectively mimics a two-entry FIFO. Confirm that your usage patterns only require caching of at most two versions at a time.
450-455: Initialize arrays with fewer allocations.
Returning a rootkeyCache with defaulted arrays ensures correct initial values and minimal overhead. This is straightforward and readable.
466-466: Ensure consistent usage of the new cache in prune logic.
Using the traverseOrphansWithRootkeyCache method for version deletion is aligned with the new caching mechanism. Verify that all references to root keys in deleteVersion are now consolidated to leverage cache.getRootKey.
1115-1117: Avoid recreating the cache if usage is frequent.
Instantiating a new cache immediately before traverseOrphansWithRootkeyCache may be acceptable if these calls are rare. Otherwise, confirm whether performance gains could arise from reusing a single cache instance across multiple orphan traversals.
Line range hint 1119-1130: Well-structured decoupling with explicit cache parameter.
Passing rootkeyCache as a parameter enables testability and modular design, especially when iterating over orphans in different versions. This is a clean approach that separates caching logic from traversal logic.
|
@Mergifyio backport release/v1.2.x |
|
@Mergifyio backport release/v1.3.x |
✅ Backports have been created
|
✅ Backports have been created
|
(cherry picked from commit 1f9d525)
(cherry picked from commit 1f9d525)
Follow on to: #1026
This improves the rootkey cache used in pruning.
It stores two keys now, and its integrated into the traverse calls.
This speedsup the relevant prune logic in osmosis block sync a total of 27% (relative to prior #1026)
Summary by CodeRabbit