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 unused valDup #443

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

Conversation

eliblight
Copy link

It turns out valDup is totally unused in the codebase

Copy link

codecov bot commented May 6, 2024

Codecov Report

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

Project coverage is 68.45%. Comparing base (168da8b) to head (a956b50).

Current head a956b50 differs from pull request most recent head f30c6bc

Please upload reports for the commit f30c6bc to get more accurate results.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #443      +/-   ##
============================================
- Coverage     70.22%   68.45%   -1.78%     
============================================
  Files           109      109              
  Lines         59956    61680    +1724     
============================================
+ Hits          42104    42221     +117     
- Misses        17852    19459    +1607     
Files Coverage Δ
src/blocked.c 91.86% <100.00%> (-0.01%) ⬇️
src/cluster_legacy.c 74.69% <ø> (-11.77%) ⬇️
src/config.c 77.54% <ø> (-0.78%) ⬇️
src/db.c 87.47% <100.00%> (-0.80%) ⬇️
src/eval.c 55.45% <ø> (-0.57%) ⬇️
src/expire.c 96.28% <ø> (-0.24%) ⬇️
src/functions.c 95.62% <100.00%> (+0.03%) ⬆️
src/kvstore.c 96.39% <100.00%> (-0.40%) ⬇️
src/latency.c 81.49% <ø> (+1.33%) ⬆️
src/module.c 9.34% <ø> (-0.31%) ⬇️
... and 9 more

... and 70 files with indirect coverage changes

@madolson
Copy link
Member

madolson commented May 8, 2024

As mentioned on slack, can you run the performance test again with more keys and post the updated performance here. As long as there is a meaningful difference, I think it would be worth pulling it.

@eliblight
Copy link
Author

For random key and small packets we will see some improvement. For larger packet the improvement is overshadowed by the overall load and is no longer measurable (though logic say it must be there however small).

@madolson
Copy link
Member

@eliblight Can you fix the DCO complaint?

deps/hiredis/async.c Outdated Show resolved Hide resolved
src/dict.c Outdated Show resolved Hide resolved
@madolson
Copy link
Member

Sorry for taking a bit to get back around to this, can you address the merge conflicts and update the PR? I'm happy with it otherwise.

@eliblight eliblight force-pushed the try-to-remove-dup branch 3 times, most recently from 20c8e67 to 3153bc0 Compare May 25, 2024 14:25
Signed-off-by: Eran Liberty <[email protected]>
PR by keeping the dict in dictSetVal and not using it.
... but mainly to make @madolson happy

Signed-off-by: Eran Liberty <[email protected]>
@eliblight
Copy link
Author

@madolson your turn

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.

None yet

3 participants