Skip to content

Conversation

@Nicky-2000
Copy link
Contributor

@Nicky-2000 Nicky-2000 commented Jul 30, 2025

This PR fixes a bug in prefetchNextBucketEntries which is used in the hashtable iterator. The current version of prefetchNextBucketEntries does not correctly prefetch the next two buckets that will be iterated over. This is due next_index being incremented twice (once in prefetchNextBucketEntries and again in getNextBucket).

Fix Explanation:

  1. Add check in prefetchNextBucketEntries to ensure we pass the correct 'next_index' to getNextBucket.
  2. The extra increment was removed from getNextBucket.

Performance

The fix was tested in a similar manner as #1501 where these changes were introduced.

All the data below was taken on a
The data below was collected by executing the KEYS * command and Save command on a 64-logical core (32 physical cores, 2 threads per core) AMD EPYC 7B13 processor. The dataset had 50 million keys with a value size of 100 bytes each.

KEYS * Command

Fix Applied Mean Time (seconds) Over 8 trials Std Dev (s) Over 8 Trials
No 6.374759 0.10
Yes 6.184731 0.06

Improvement:

  • Modest 2.98% reduction in command duration,
  • 40% improvement in standard deviation.
  • This shows that the fix slightly speeds up the command and makes its performance more consistent.

Save command improvement

Setup:

  • 64-logical core (32 physical cores, 2 threads per core) AMD EPYC 7B13
  • 50 mil keys in size of 100 bytes each.
  • Running valkey server over standard file system with 402 M/s throughput.
  • crc checksum ON and comperssion ON .

Results

Fix Applied Mean Time (s) Over 8 trials Std Dev (s) Over 8 Trials
No 49.87 0.38
Yes 49.85 0.15

Improvement:

  • No noticeable improvement or regression in average Save performance
  • Performance is more consistent with the fix (standard deviation 62% smaller over the 8 test trials).

prefetches the next 2 buckets.

Signed-off-by: Nicky Khorasani <[email protected]>
@Nicky-2000 Nicky-2000 marked this pull request as ready for review July 30, 2025 16:36
@zuiderkwast
Copy link
Contributor

@NadavGigi PTAL

@codecov
Copy link

codecov bot commented Aug 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.49%. Comparing base (a481fe2) to head (57d1964).
⚠️ Report is 18 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #2394      +/-   ##
============================================
+ Coverage     71.44%   71.49%   +0.05%     
============================================
  Files           123      123              
  Lines         67177    67179       +2     
============================================
+ Hits          47992    48032      +40     
+ Misses        19185    19147      -38     
Files with missing lines Coverage Δ
src/hashtable.c 82.84% <100.00%> (+0.41%) ⬆️

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Nicky-2000 Nicky-2000 requested a review from zuiderkwast August 4, 2025 22:14
@zuiderkwast zuiderkwast merged commit aac3c76 into valkey-io:unstable Aug 5, 2025
51 of 52 checks passed
allenss-amazon pushed a commit to allenss-amazon/valkey-core that referenced this pull request Aug 19, 2025
valkey-io#2394)

This PR fixes a bug in prefetchNextBucketEntries which is used in the
hashtable iterator. The current version of prefetchNextBucketEntries
does not correctly prefetch the next two buckets that will be iterated
over. This is due next_index being incremented twice (once in
prefetchNextBucketEntries and again in getNextBucket).

Fix:

1. Add check in prefetchNextBucketEntries to ensure we pass the correct
   'next_index' to getNextBucket.
2. The extra increment was removed from getNextBucket.

Signed-off-by: Nicky Khorasani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants