Accounts read cache evictor polls instead of checking a channel #3891
+30
−50
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brooksprumo
commented
Dec 3, 2024
Comment on lines
-206
to
-208
| if self.data_size() > self.max_data_size_hi { | ||
| self.send_evict(); | ||
| } |
There was a problem hiding this comment.
This is the main win. Now the foreground thread, when it calls store(), doesn't need to check the data size and send an evict message.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
In #3867, there was a discussion that prompted me to consider if there was a need for a channel at all, now that we're using
try_recv()instead ofrecv()in the evictor: #3867 (comment)Using a channel here now seems redundant, and only adds unnecessary work. We still check every 100 ms without a channel, so can we remove the channel entirely?
Summary of Changes
Remove the channel for the accounts read cache evictor. We're already polling every 100 ms anyway.
Results
I ran this PR against master, similar to what I showed in #3867 (comment).
These graphs are over 1 hour. Purple is this PR, blue is master.
Here's time to store. Looks the same to me:
Total size of the cache. Again, looks the same to me:
Wakeups are the same, as expected:
Overall, this PR's perf looks similar to master. I wouldn't expect much difference. We may only see a meaningful difference where there would've been contention in
store()when sending an eviction message.