Skip to content

improve the performance of imported_keylist deconstruction#239

Merged
filipecosta90 merged 1 commit intoredis:masterfrom
DarrenJiang13:fast-free-keylist
Jul 22, 2025
Merged

improve the performance of imported_keylist deconstruction#239
filipecosta90 merged 1 commit intoredis:masterfrom
DarrenJiang13:fast-free-keylist

Conversation

@DarrenJiang13
Copy link
Contributor

@DarrenJiang13 DarrenJiang13 commented Oct 24, 2023

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.

YaacovHazan
YaacovHazan requested changes Nov 1, 2023
View reviewed changes
YaacovHazan
YaacovHazan requested changes Nov 2, 2023
View reviewed changes
@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 •
edited
Loading

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

DarrenJiang13 commented Jan 18, 2024

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

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

filipecosta90 commented Jul 22, 2025

@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.

@fcostaoliveira fcostaoliveira mentioned this pull request Sep 5, 2025
Merged
@BrewTestBot BrewTestBot mentioned this pull request Sep 5, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@YaacovHazan YaacovHazan YaacovHazan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@DarrenJiang13 @filipecosta90 @YaacovHazan