Skip to content
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

Merged
merged 2 commits into from
Jun 14, 2024

Conversation

madolson
Copy link
Member

@madolson madolson commented Jun 11, 2024

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.

image

I saw a modest performance bump for a single shard cluster:

Before:

./src/valkey-benchmark --threads 2 -n 20000000 -r 100000 -d 128 -P 25 -t set
Summary:
  throughput summary: 518672.19 requests per second
  latency summary (msec):
          avg       min       p50       p95       p99       max
        2.328     0.312     2.495     3.231     3.519    11.831

After:

./src/valkey-benchmark --threads 2 -n 20000000 -r 100000 -d 128 -P 25 -t set
Summary:
  throughput summary: 551040.12 requests per second
  latency summary (msec):
          avg       min       p50       p95       p99       max
        2.191     0.840     2.367     2.999     3.207     6.503

I think we would see some comparable improvements in the other places where we are using it such as tracking and ACLs.

Copy link

codecov bot commented Jun 12, 2024

Codecov Report

Attention: Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 70.22%. Comparing base (e65b2d2) to head (93ab629).

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     
Files Coverage Δ
src/acl.c 88.90% <100.00%> (+<0.01%) ⬆️
src/cluster.c 87.77% <100.00%> (+0.01%) ⬆️
src/db.c 88.32% <ø> (ø)
src/server.c 88.92% <100.00%> (+<0.01%) ⬆️
src/server.h 100.00% <100.00%> (ø)
src/tracking.c 98.97% <100.00%> (+<0.01%) ⬆️
src/module.c 9.65% <0.00%> (-0.01%) ⬇️

... and 8 files with indirect coverage changes

int numkeys; /* Number of key indices return */
int size; /* Available array size */
keyReference *keys; /* Key indices array, points to keysbuf or heap */
Copy link
Member Author

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.

@hpatro
Copy link
Collaborator

hpatro commented Jun 12, 2024

@madolson what's the tool used to capture the time spent per assembly instruction?

@madolson
Copy link
Member Author

@hpatro just perf and perf report

Copy link
Member

@PingXie PingXie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@enjoy-binbin
Copy link
Member

i think we can also backport it to 7.2? the optimization looks nice

@enjoy-binbin enjoy-binbin requested a review from zuiderkwast June 14, 2024 04:35
@madolson
Copy link
Member Author

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.

@madolson madolson merged commit 6faa48a into valkey-io:unstable Jun 14, 2024
18 checks passed
@madolson madolson added release-notes This issue should get a line item in the release notes performance labels Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance release-notes This issue should get a line item in the release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants