Fast path in SET if the expiration time is expired #865

enjoy-binbin · 2024-08-02T09:36:27Z

If the expiration time passed in SET is expired, for example, it
has expired due to the machine time (DTS) or the expiration time
passed in (wrong arg). In this case, we don't need to set the key
and wait for the active expire scan before deleting the key.

Compared with previous changes:

If the key does not exist, previously we would set the key and wait
for the active expire to delete it, so it is a set + del from the perspective
of propaganda. Now we will no set the key and return, so it a NOP.
If the key exists, previously we woule set the key and wait
for the active expire to delete it, so it is a set + del From the perspective
of propaganda. Now we will delete it and return, so it is a del.

Adding a new deleteExpiredKeyFromOverwriteAndPropagate function
to reduce the duplicate code.

A simple benchmark test:

./src/valkey-benchmark -P 32 -c 100 -n 10000000 -r 1000000000 set __rand_int__ value exat 100

old: 252474.23, new: 829256.12

If the expiration time passed in SET is expired, for example, it has expired due to the machine time (DTS) or the expiration time passed in (wrong arg). In this case, we don't need to set the key and wait for the active expire scan before deleting the key. Compared with previous changes: 1. If the key does not exist, previously we would set the key and wait for the active expire to delete it, so it is a set + del from the perspective of propaganda. Now we will no set the key and return, so it a NOP. 2. If the key exists, previously we woule set the key and wait for the active expire to delete it, so it is a set + del From the perspective of propaganda. Now we will delete it and return, so it is a del. A simple benchmark test: ``` ./src/valkey-benchmark -P 32 -c 100 -n 10000000 -r 1000000000 set __rand_int__ value exat 100 old: 252474.23, new: 829256.12 ``` Signed-off-by: Binbin <binloveplay1314@qq.com>

codecov · 2024-08-02T09:52:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.66%. Comparing base (b12668a) to head (58c086c).
Report is 32 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #865      +/-   ##
============================================
+ Coverage     70.61%   70.66%   +0.05%     
============================================
  Files           112      112              
  Lines         61515    61514       -1     
============================================
+ Hits          43436    43468      +32     
+ Misses        18079    18046      -33

Files with missing lines	Coverage Δ
src/cluster.c	`88.35% <ø> (+0.29%)`	⬆️
src/db.c	`88.50% <100.00%> (+0.07%)`	⬆️
src/expire.c	`96.42% <100.00%> (-0.09%)`	⬇️
src/server.h	`100.00% <ø> (ø)`
src/t_string.c	`96.49% <100.00%> (-0.02%)`	⬇️

... and 8 files with indirect coverage changes

src/t_string.c

Signed-off-by: Binbin <binloveplay1314@qq.com>

Test failures

Signed-off-by: Binbin <binloveplay1314@qq.com>

enjoy-binbin · 2024-08-08T05:34:31Z

ok, the test failed because the old restore use lazyfree_lazy_server_del to delete the expired key (not the lazy expire)

i am going to leave store alone.

Signed-off-by: Binbin <binloveplay1314@qq.com>

tests/unit/type/string.tcl

src/db.c

Signed-off-by: Binbin <binloveplay1314@qq.com>

src/cluster.c

madolson

Saw there was some back and forth about the specifics, but the latest version looks good to me outside of a minor wording suggestion.

Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Signed-off-by: Binbin <binloveplay1314@qq.com>

If the expiration time passed in SET is expired, for example, it has expired due to the machine time (DTS) or the expiration time passed in (wrong arg). In this case, we don't need to set the key and wait for the active expire scan before deleting the key. Compared with previous changes: 1. If the key does not exist, previously we would set the key and wait for the active expire to delete it, so it is a set + del from the perspective of propaganda. Now we will no set the key and return, so it a NOP. 2. If the key exists, previously we woule set the key and wait for the active expire to delete it, so it is a set + del From the perspective of propaganda. Now we will delete it and return, so it is a del. Adding a new deleteExpiredKeyFromOverwriteAndPropagate function to reduce the duplicate code. Signed-off-by: Binbin <binloveplay1314@qq.com> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>

Test failed because these two PRs #865 and #913. Signed-off-by: Binbin <binloveplay1314@qq.com>

If the expiration time passed in SET is expired, for example, it has expired due to the machine time (DTS) or the expiration time passed in (wrong arg). In this case, we don't need to set the key and wait for the active expire scan before deleting the key. Compared with previous changes: 1. If the key does not exist, previously we would set the key and wait for the active expire to delete it, so it is a set + del from the perspective of propaganda. Now we will no set the key and return, so it a NOP. 2. If the key exists, previously we woule set the key and wait for the active expire to delete it, so it is a set + del From the perspective of propaganda. Now we will delete it and return, so it is a del. Adding a new deleteExpiredKeyFromOverwriteAndPropagate function to reduce the duplicate code. Signed-off-by: Binbin <binloveplay1314@qq.com> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>

Test failed because these two PRs #865 and #913. Signed-off-by: Binbin <binloveplay1314@qq.com>

If the expiration time passed in SET is expired, for example, it has expired due to the machine time (DTS) or the expiration time passed in (wrong arg). In this case, we don't need to set the key and wait for the active expire scan before deleting the key. Compared with previous changes: 1. If the key does not exist, previously we would set the key and wait for the active expire to delete it, so it is a set + del from the perspective of propaganda. Now we will no set the key and return, so it a NOP. 2. If the key exists, previously we woule set the key and wait for the active expire to delete it, so it is a set + del From the perspective of propaganda. Now we will delete it and return, so it is a del. Adding a new deleteExpiredKeyFromOverwriteAndPropagate function to reduce the duplicate code. Signed-off-by: Binbin <binloveplay1314@qq.com> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Signed-off-by: Ping Xie <pingxie@google.com>

…o#980) Test failed because these two PRs valkey-io#865 and valkey-io#913. Signed-off-by: Binbin <binloveplay1314@qq.com> Signed-off-by: Ping Xie <pingxie@google.com>

If the expiration time passed in SET is expired, for example, it has expired due to the machine time (DTS) or the expiration time passed in (wrong arg). In this case, we don't need to set the key and wait for the active expire scan before deleting the key. Compared with previous changes: 1. If the key does not exist, previously we would set the key and wait for the active expire to delete it, so it is a set + del from the perspective of propaganda. Now we will no set the key and return, so it a NOP. 2. If the key exists, previously we woule set the key and wait for the active expire to delete it, so it is a set + del From the perspective of propaganda. Now we will delete it and return, so it is a del. Adding a new deleteExpiredKeyFromOverwriteAndPropagate function to reduce the duplicate code. Signed-off-by: Binbin <binloveplay1314@qq.com> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Signed-off-by: Ping Xie <pingxie@google.com>

…o#980) Test failed because these two PRs valkey-io#865 and valkey-io#913. Signed-off-by: Binbin <binloveplay1314@qq.com> Signed-off-by: Ping Xie <pingxie@google.com>

If the expiration time passed in SET is expired, for example, it has expired due to the machine time (DTS) or the expiration time passed in (wrong arg). In this case, we don't need to set the key and wait for the active expire scan before deleting the key. Compared with previous changes: 1. If the key does not exist, previously we would set the key and wait for the active expire to delete it, so it is a set + del from the perspective of propaganda. Now we will no set the key and return, so it a NOP. 2. If the key exists, previously we woule set the key and wait for the active expire to delete it, so it is a set + del From the perspective of propaganda. Now we will delete it and return, so it is a del. Adding a new deleteExpiredKeyFromOverwriteAndPropagate function to reduce the duplicate code. Signed-off-by: Binbin <binloveplay1314@qq.com> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Signed-off-by: Ping Xie <pingxie@google.com>

…o#980) Test failed because these two PRs valkey-io#865 and valkey-io#913. Signed-off-by: Binbin <binloveplay1314@qq.com> Signed-off-by: Ping Xie <pingxie@google.com>

hpatro reviewed Aug 2, 2024

View reviewed changes

src/t_string.c Outdated Show resolved Hide resolved

src/t_string.c Outdated Show resolved Hide resolved

enjoy-binbin added 2 commits August 8, 2024 11:51

adding deleteExpiredKeyFromOverwriteAndPropagate

4a742cb

Signed-off-by: Binbin <binloveplay1314@qq.com>

add back tick to milliseconds comment

33eb3b5

Signed-off-by: Binbin <binloveplay1314@qq.com>

madolson previously approved these changes Aug 8, 2024

View reviewed changes

madolson requested a review from hpatro August 8, 2024 05:30

fix test

73cde7a

Signed-off-by: Binbin <binloveplay1314@qq.com>

leave restore alone

e9e089b

Signed-off-by: Binbin <binloveplay1314@qq.com>

hpatro approved these changes Aug 9, 2024

View reviewed changes

tests/unit/type/string.tcl Show resolved Hide resolved

enjoy-binbin requested a review from madolson August 15, 2024 10:43

enjoy-binbin requested a review from zuiderkwast August 22, 2024 15:51

zuiderkwast reviewed Aug 22, 2024

View reviewed changes

src/db.c Outdated Show resolved Hide resolved

Merge remote-tracking branch 'upstream/unstable' into set_expire

fef9cc9

Signed-off-by: Binbin <binloveplay1314@qq.com>

enjoy-binbin added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Aug 23, 2024

remove do_delete arg

c7f74af

Signed-off-by: Binbin <binloveplay1314@qq.com>

madolson reviewed Aug 23, 2024

View reviewed changes

src/cluster.c Outdated Show resolved Hide resolved

madolson approved these changes Aug 23, 2024

View reviewed changes

Update src/cluster.c

58c086c

Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Signed-off-by: Binbin <binloveplay1314@qq.com>

zuiderkwast approved these changes Aug 23, 2024

View reviewed changes

enjoy-binbin added the release-notes This issue should get a line item in the release notes label Aug 24, 2024

enjoy-binbin merged commit e3af1a3 into valkey-io:unstable Aug 31, 2024
56 checks passed

enjoy-binbin deleted the set_expire branch August 31, 2024 14:39

enjoy-binbin mentioned this pull request Sep 2, 2024

Fix set expire test due to the new lazyfree configs changes #980

Merged

enjoy-binbin added a commit that referenced this pull request Sep 2, 2024

Fix set expire test due to the new lazyfree configs changes (#980)

5693fe4

Test failed because these two PRs #865 and #913. Signed-off-by: Binbin <binloveplay1314@qq.com>

madolson pushed a commit that referenced this pull request Sep 3, 2024

Fix set expire test due to the new lazyfree configs changes (#980)

def9ddf

Test failed because these two PRs #865 and #913. Signed-off-by: Binbin <binloveplay1314@qq.com>

lilpoozie2005 approved these changes Sep 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fast path in SET if the expiration time is expired #865

Fast path in SET if the expiration time is expired #865

enjoy-binbin commented Aug 2, 2024 •

edited

Loading

codecov bot commented Aug 2, 2024 •

edited

Loading

enjoy-binbin commented Aug 8, 2024 •

edited

Loading

madolson left a comment

Fast path in SET if the expiration time is expired #865

Fast path in SET if the expiration time is expired #865

Conversation

enjoy-binbin commented Aug 2, 2024 • edited Loading

codecov bot commented Aug 2, 2024 • edited Loading

Codecov Report

enjoy-binbin commented Aug 8, 2024 • edited Loading

madolson left a comment

Choose a reason for hiding this comment

enjoy-binbin commented Aug 2, 2024 •

edited

Loading

codecov bot commented Aug 2, 2024 •

edited

Loading

enjoy-binbin commented Aug 8, 2024 •

edited

Loading