-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Optimize] Cache the recently queried committees #3106
Conversation
Signed-off-by: ljedrz <ljedrz@gmail.com>
Signed-off-by: ljedrz <ljedrz@gmail.com>
}; | ||
|
||
// Insert the committee into the cache. | ||
self.committee_cache.lock().push(round, committee.clone()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ljedrz this only works for the match
case on Some
. It does not work for the committee from None
.
Can you scope the cache for only committees from Some
and profile the performance again?
(If you want to go the extra mile, you can also try adding a cache
for the underlying impl of self.current_committee()
, but this extra overhead may not be worth the complexity. The majority of calls will route to Some
I believe)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed; I ran the branch locally and all I can see are still cache hits. As for the current committee, I agree that its overhead is low; we can always go back to that idea in the future if need be.
Signed-off-by: ljedrz <ljedrz@gmail.com>
@@ -35,18 +37,23 @@ use std::{ | |||
}, | |||
}; | |||
|
|||
/// The capacity of the LRU holiding the recently queried committees. | |||
const COMMITTEE_CACHE_SIZE: usize = 16; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for using 16
?
This committee cache should just need the rounds near the ones being processed, but not sure what a good number would be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really; the idea was just that it would be small (for low overhead), but still large enough to cover all possible hits. It could probably be a bit smaller, but any resulting difference should be negligible.
Committee queries are very prominent in CPU profiles, even with small committee sizes - they are performed quite often, and the deserialization involved is relatively expensive.
This PR adds a simple, small LRU cache that speeds them up; I've double-checked its results by obtaining a new CPU profile (which - as expected - is very different), and ensuring that the cache gets hit (which happens quite a lot).