Skip to content

improve performance for keys with expiration time #12177

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

Merged
merged 1 commit into from
Jun 14, 2023

Conversation

judeng
Copy link
Contributor

@judeng judeng commented May 15, 2023

This change only affects keys with expiry time.
For SETEX, the average improvement is 5%, and for GET with expiation key, we gain a improvement of 13%.

When keys have expiration time, Redis has an assertion to look up the main dict every time when it touches the expires.
This comes with a performance const, especially during rehash. the damage will be double.

It looks like that assert was added some ten years old, maybe out of paranoia, and there's probably no reason to keep it at that cost.

@judeng
Copy link
Contributor Author

judeng commented May 15, 2023

here benchmark result with duplicate 10 times, for setex, the average improvement is 5%, and for get command, we gain a improvement of 13%

command:

memtier_benchmark -s 127.0.0.1 -p 7000 -t 5 -c 10  --hide-histogram  --key-maximum=100000 --command="setex __key__ 10000 __data__" --test-time=60  --pipeline=50
memtier_benchmark -s 127.0.0.1 -p 7000 -t 5 -c 10  --hide-histogram  --key-maximum=100000 --command="get __key__" --test-time=60  --pipeline=50

results:

setex this thread 519482.8 511375.7 502968 514473.1 510386.9 510380.8 506815.5 520807.6 516531.3 516756.6 5129978
setex unstable 457644.7 493901.4 485095.7 483967.4 492119.1 485894.5 477765.4 483789.1 466823.1 488208 4815208
get this thread 853503 864664.4 856952.4 822671.4 856694.9 839800.9 860599.7 846762.3 873183.1 847229.9 8522062
get unstable 755178.8 749558.7 760419.1 765058.5 726172 785763.1 703592.7 769812 763690.4 768107 7547352

@judeng
Copy link
Contributor Author

judeng commented May 15, 2023

the ci faied with message "Expected '29375' to be less than '29236' (context: type eval line 17 cmd {assert_lessthan $el_sum2 [expr $el_sum1+5000] } proc ::test)", It looks like same with #12169, @CharlesChen888 hi, could you have a look? thank you

@CharlesChen888
Copy link
Contributor

That is a new test for some new metrics in INFO STATS, see #11963. I already allowed a big tolerance in the test, but I guess I need to make it even bigger...

oranagra pushed a commit that referenced this pull request Jun 11, 2023
In #11963, some new tests about eventloop duration were added, which includes time measurement in TCL scripts. This has caused some unexpected CI failures, such as #12169 and #12177, due to slow test servers or some performance jittering.
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

i think i'd rather just drop the assertion if it causes so much damage.
maybe it was needed during development time, but probably not needed anymore.
the complication of adding a server variable, and a debug command doesn't seem worth it.
please include some statement about the measured improvement in the top comment.

@oranagra oranagra added the action:run-benchmark Triggers the benchmark suite for this Pull Request label Jun 11, 2023
@judeng
Copy link
Contributor Author

judeng commented Jun 12, 2023

i think i'd rather just drop the assertion if it causes so much damage.

Totally agree. I just don't know why this assert was added so had to be conservative :-)

@judeng judeng force-pushed the optimize-expire branch from 6417543 to db9f1d0 Compare June 14, 2023 07:27
@oranagra oranagra merged commit 789c33b into redis:unstable Jun 14, 2023
@oranagra oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Jun 14, 2023
@judeng judeng deleted the optimize-expire branch June 28, 2023 06:37
@oranagra oranagra mentioned this pull request Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action:run-benchmark Triggers the benchmark suite for this Pull Request release-notes indication that this issue needs to be mentioned in the release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants