Skip to content

improve the performance of imported_keylist deconstruction #239

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
Jul 22, 2025

Conversation

DarrenJiang13
Copy link
Contributor

Problem

When I run memtier_benchmark with --data-import (including 500000 keys), it took a long time to quit after benchmark finished.
With pstack, I found the process was waiting in ~imported_keylist()

...
#6  std::vector<imported_keylist::key*, std::allocator<imported_keylist::key*> >::erase (__position=..., this=0x1e685e8) at /usr/include/c++/9/bits/stl_vector.h:1428
#7  imported_keylist::~imported_keylist (this=0x1e685e0, __in_chrg=<optimized out>) at obj_gen.cpp:534
#8  0x0000000000407a1a in main (argc=<optimized out>, argv=<optimized out>) at memtier_benchmark.cpp:1641

Solution

erase() one by one for a dead vector wastes some time. So I replaced erase() with clear(), which makes it much faster.

@DarrenJiang13 DarrenJiang13 force-pushed the fast-free-keylist branch 3 times, most recently from 17956d6 to ff46b67 Compare November 3, 2023 07:07
@DarrenJiang13
Copy link
Contributor Author

DarrenJiang13 commented Nov 3, 2023

Test command

./memtier_benchmark -p 6379 -t 2 -c 8 -n 500000 --ratio 1:0 --data-import ./dump100.csv

Test result:

key numbers time cost to quit after finishing benchmark (second)
before commit 500000 175
after commit 500000 1

Supplement

dump100.csv.zip

@DarrenJiang13
Copy link
Contributor Author

@filipecosta90 hi could you please check this PR? I think it is ready to be merged

@filipecosta90 filipecosta90 merged commit c672f44 into RedisLabs:master Jul 22, 2025
@filipecosta90
Copy link
Collaborator

@filipecosta90 hi could you please check this PR? I think it is ready to be merged

@DarrenJiang13 merged. sorry for missing this one! and thank you! will add a PR to extend test coverage to this as well.

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