graphdb: add caching for isPublicNode query #10363
Conversation
Summary of ChangesHello @Abdulkbk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a caching mechanism for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces caching for the IsPublicNode query to address a significant performance bottleneck in gossip message processing. The changes are well-structured, including the addition of a new cache, configuration options, and cache invalidation logic at various points where a node's public status might change. I've identified a potential race condition in one of the cache invalidation paths and an opportunity to optimize the cache-aside pattern to prevent thundering herds. Overall, this is a valuable improvement.
| // Store the result in cache. | ||
| s.cacheMu.Lock() | ||
| _, err = s.publicNodeCache.Put(pubKey, &cachedPublicNode{ | ||
| isPublic: isPublic, | ||
| }) | ||
| if err != nil { | ||
| log.Warnf("unable to store node %x in cache: %v", pubKey, err) | ||
| } | ||
|
|
||
| s.cacheMu.Unlock() | ||
|
|
||
| return isPublic, nil |
There was a problem hiding this comment.
There's a potential for a thundering herd problem here. If multiple goroutines call IsPublicNode for the same key that is not in the cache, they will all miss the cache, query the database, and then attempt to write to the cache. To optimize this and prevent redundant database queries, you can re-check the cache after acquiring the write lock. This ensures that only the first goroutine populates the cache, and subsequent ones will use the cached value.
// Store the result in cache. We use a double-checked locking pattern
// here to avoid a thundering herd problem where multiple goroutines
// query the DB for the same key on a cache miss.
s.cacheMu.Lock()
defer s.cacheMu.Unlock()
// Re-check the cache to avoid a race where another goroutine populated
// it between our read-lock release and write-lock acquisition.
if cached, err := s.publicNodeCache.Get(pubKey); err == nil && cached != nil {
return cached.isPublic, nil
}
_, err = s.publicNodeCache.Put(pubKey, &cachedPublicNode{
isPublic: isPublic,
})
if err != nil {
log.Warnf("unable to store node %x in cache: %v", pubKey, err)
}
return isPublic, nil
Abdulkbk
force-pushed
the
ispub-cache
branch
from
eb44288 to
0a81125
Compare
This commit adds the struct we'll use to cache the channel. It also adds the require `Size` method for the lru package.
In this commit, we add publicNodeCache into the sqlstore. We also add the necessary config for initializing the cache. Signed-off-by: Abdullahi Yunus <[email protected]>
In this commit, we first check for the node in our cache before querying the database when determining if a node is public or not.
In this commit, we remove nodes from the node cache in various db method call site which execution could affect the public status of the nodes.
Abdulkbk
force-pushed
the
ispub-cache
branch
from
0a81125 to
85b45d5
Compare
1 participant
fixes #10337
continues #10356
Change Description
From the issue description:
In this PR, we add caching to the
IsPublicNodemethod in theSQLStore. SinceAdding cache will significantly reduce database overhead and accelerate gossip message processing.
Steps to Test
Steps for reviewers to follow to test the change.