-
Notifications
You must be signed in to change notification settings - Fork 703
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
Don't initialize the key buffer in getKeysResult #631
Conversation
Signed-off-by: Madelyn Olson <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #631 +/- ##
============================================
+ Coverage 70.11% 70.22% +0.10%
============================================
Files 110 110
Lines 60038 60047 +9
============================================
+ Hits 42096 42167 +71
+ Misses 17942 17880 -62
|
Signed-off-by: Madelyn Olson <[email protected]>
int numkeys; /* Number of key indices return */ | ||
int size; /* Available array size */ | ||
keyReference *keys; /* Key indices array, points to keysbuf or heap */ |
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.
Although this re-ordering is not strictly needed, it should provide slightly better cache locality as now numkeys, size, and the first couple of keys in the keysbuf will be on the same cache line.
@madolson what's the tool used to capture the time spent per assembly instruction? |
@hpatro just perf and perf report |
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.
Nice!
i think we can also backport it to 7.2? the optimization looks nice |
I'm pretty hesitant to backport stuff that's not strictly a bug fix, since I value stability. But I could be convinced otherwise. |
getKeysResults is typically initialized with 2kb of zeros (16 * 256), which isn't strictly necessary since the only thing we have to initialize is some of the metadata fields. The rest of the data can remain junk as long as we don't access it. This was a bit of a regression in 7.0 with the keyspecs, since we doubled the size of the zeros, but hopefully this recovers a lot of the performance drop.
I saw a modest performance bump for a single shard cluster:
Before:
After:
I think we would see some comparable improvements in the other places where we are using it such as tracking and ACLs.