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

Remove redundant dict_index calculations #1205

Open
wants to merge 3 commits into
base: unstable
Choose a base branch
from

Conversation

nadav-levanoni
Copy link
Contributor

@nadav-levanoni nadav-levanoni commented Oct 21, 2024

We need to start making use of the new WithDictIndex APIs which allow us to reuse the dict_index calculation (avoid over-calling getKeySlot for no good reason).

In this PR I optimized lookupKey so it now calls getKeySlot to reuse the dict_index two additional times.

The benefit here is reduced if the slot is cached in the client, which is generally the case.

Copy link

codecov bot commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.72%. Comparing base (771918e) to head (e940f02).

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1205      +/-   ##
============================================
+ Coverage     70.71%   70.72%   +0.01%     
============================================
  Files           114      114              
  Lines         63090    63091       +1     
============================================
+ Hits          44616    44624       +8     
+ Misses        18474    18467       -7     
Files with missing lines Coverage Δ
src/db.c 88.86% <100.00%> (+0.09%) ⬆️

... and 10 files with indirect coverage changes

@nadav-levanoni
Copy link
Contributor Author

Will add a follow-up PR for keys and scan commands. The impact will be greater because they do not cache the slot in the client.

@xbasel xbasel self-requested a review November 4, 2024 09:53
Copy link
Member

@xbasel xbasel left a comment

Choose a reason for hiding this comment

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

lgtm

Signed-off-by: Nadav Levanoni <[email protected]>
@nadav-levanoni nadav-levanoni changed the title Remove redundant dict_index calculations in lookupKey Remove redundant dict_index calculations Nov 4, 2024
@nadav-levanoni
Copy link
Contributor Author

I added the keysCommand optimization here. I had a cursory look at the scan command, and it might be trickier to find an optimization there.

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