Reduce active defrag test latency by lowering hit threshold by asagege · Pull Request #2553 · valkey-io/valkey

asagege · 2025-08-27T02:40:43Z

Changed the defrag hit threshold from 512 to 1 in the defragLaterStep & defragStageKvstoreHelper function to reduce defrag latency. (idea from Jim)

Previously, the defrag loop would continue processing up to 512 successful defragmentations before checking if the time limit was exceeded. Now it checks after every single successful allocation move.
The trade-off is slightly more frequent time checks, but the time check (~19ns) is negligible compared to the actual defragmentation work (even a single 8-byte reallocation takes ~43ns and the allocatorShouldDefrag function call takes ~49ns per block). This overhead is minimal compared to the latency improvement gained from better time management during active defragmentation.
Also revert the change from https://github.com/valkey-io/valkey/pull/2421/files to test.
Solved compilation issue with unsigned by changing the type of the local variable prev_defragged to match the type of server.stat_active_defrag_hits

Closes #2444

Signed-off-by: Alina Liu <alinalq@dev-dsk-alinalq-2b-2db84246.us-west-2.amazon.com>

codecov · 2025-08-27T02:59:37Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.23%. Comparing base (40fe422) to head (7263a19).
⚠️ Report is 9 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2553      +/-   ##
============================================
+ Coverage     72.04%   72.23%   +0.19%     
============================================
  Files           126      126              
  Lines         70615    70619       +4     
============================================
+ Hits          50872    51015     +143     
+ Misses        19743    19604     -139

Files with missing lines	Coverage Δ
src/defrag.c	`82.33% <100.00%> (ø)`

... and 12 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: Alina Liu <alinalq@dev-dsk-alinalq-2b-2db84246.us-west-2.amazon.com>

JimB123

Looks good. I agree that we should check the time after every defrag hit. The time check is negligible in cost, and is small compared with the defragmentation itself - which is variable depending on block size.

In any case, it's worth noting that the old logic is strange. It allowed for 512 actual defragmentations, while only allowing for 64 scans (a check without defragmentation).

re: lowering the latency check back to 5ms. If we see additional failures, we should consider that they might be due to OS scheduling. We can consider checking the main-thread's CPU time rather than elapsed clock time.

JimB123

(note we need to address the compilation issue)

Signed-off-by: Alina Liu <alinalq@dev-dsk-alinalq-2b-2db84246.us-west-2.amazon.com>

JimB123

Looks good now.

zuiderkwast

LGTM!

To check if it gets flaky with 5 ms, I kicked off a daily job for this PR: https://github.com/valkey-io/valkey/actions/runs/17310428082

enjoy-binbin

Make sense to me.

…40 (#2574) Delete the out-of-date comment explaining why changed latency from 5 to 40 in #2421 , which is a leftover of #2553 Signed-off-by: Alina Liu <liusalisa6363@gmail.com>

…o#2553) Changed the defrag hit threshold from 512 to 1 in the `defragLaterStep` & `defragStageKvstoreHelper` function to reduce defrag latency. (idea from Jim) 1. Previously, the defrag loop would continue processing up to 512 successful defragmentations before checking if the time limit was exceeded. Now it checks after every single successful allocation move. 2. The trade-off is slightly more frequent time checks, but the time check (~19ns) is negligible compared to the actual defragmentation work (even a single 8-byte reallocation takes ~43ns and the allocatorShouldDefrag function call takes ~49ns per block). This overhead is minimal compared to the latency improvement gained from better time management during active defragmentation. 3. Also revert the change from https://github.com/valkey-io/valkey/pull/2421/files to test. 4. Solved compilation issue with unsigned by changing the type of the local variable `prev_defragged `to match the type of `server.stat_active_defrag_hits` Closes valkey-io#2444 --------- Signed-off-by: Alina Liu <alinalq@dev-dsk-alinalq-2b-2db84246.us-west-2.amazon.com> Co-authored-by: Alina Liu <alinalq@dev-dsk-alinalq-2b-2db84246.us-west-2.amazon.com>

…40 (valkey-io#2574) Delete the out-of-date comment explaining why changed latency from 5 to 40 in valkey-io#2421 , which is a leftover of valkey-io#2553 Signed-off-by: Alina Liu <liusalisa6363@gmail.com>

Changed the defrag hit threshold from 512 to 1 in the `defragLaterStep` & `defragStageKvstoreHelper` function to reduce defrag latency. (idea from Jim) 1. Previously, the defrag loop would continue processing up to 512 successful defragmentations before checking if the time limit was exceeded. Now it checks after every single successful allocation move. 2. The trade-off is slightly more frequent time checks, but the time check (~19ns) is negligible compared to the actual defragmentation work (even a single 8-byte reallocation takes ~43ns and the allocatorShouldDefrag function call takes ~49ns per block). This overhead is minimal compared to the latency improvement gained from better time management during active defragmentation. 3. Also revert the change from https://github.com/valkey-io/valkey/pull/2421/files to test. 4. Solved compilation issue with unsigned by changing the type of the local variable `prev_defragged `to match the type of `server.stat_active_defrag_hits` Closes #2444 --------- Signed-off-by: Alina Liu <alinalq@dev-dsk-alinalq-2b-2db84246.us-west-2.amazon.com> Co-authored-by: Alina Liu <alinalq@dev-dsk-alinalq-2b-2db84246.us-west-2.amazon.com>

…40 (#2574) Delete the out-of-date comment explaining why changed latency from 5 to 40 in #2421 , which is a leftover of #2553 Signed-off-by: Alina Liu <liusalisa6363@gmail.com>

…o#2553) Changed the defrag hit threshold from 512 to 1 in the `defragLaterStep` & `defragStageKvstoreHelper` function to reduce defrag latency. (idea from Jim) 1. Previously, the defrag loop would continue processing up to 512 successful defragmentations before checking if the time limit was exceeded. Now it checks after every single successful allocation move. 2. The trade-off is slightly more frequent time checks, but the time check (~19ns) is negligible compared to the actual defragmentation work (even a single 8-byte reallocation takes ~43ns and the allocatorShouldDefrag function call takes ~49ns per block). This overhead is minimal compared to the latency improvement gained from better time management during active defragmentation. 3. Also revert the change from https://github.com/valkey-io/valkey/pull/2421/files to test. 4. Solved compilation issue with unsigned by changing the type of the local variable `prev_defragged `to match the type of `server.stat_active_defrag_hits` Closes valkey-io#2444 --------- Signed-off-by: Alina Liu <alinalq@dev-dsk-alinalq-2b-2db84246.us-west-2.amazon.com> Co-authored-by: Alina Liu <alinalq@dev-dsk-alinalq-2b-2db84246.us-west-2.amazon.com> Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>

…40 (valkey-io#2574) Delete the out-of-date comment explaining why changed latency from 5 to 40 in valkey-io#2421 , which is a leftover of valkey-io#2553 Signed-off-by: Alina Liu <liusalisa6363@gmail.com> Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>

Alina Liu added 2 commits August 26, 2025 19:21

Check endtime for each allocation move

bf034b7

Signed-off-by: Alina Liu <alinalq@dev-dsk-alinalq-2b-2db84246.us-west-2.amazon.com>

Decrease latency limit in defrag to test

f54c6af

Signed-off-by: Alina Liu <alinalq@dev-dsk-alinalq-2b-2db84246.us-west-2.amazon.com>

github-actions Bot assigned asagege Aug 27, 2025

github-advanced-security AI found potential problems Aug 27, 2025

View reviewed changes

Comment thread src/defrag.c Fixed

Fix unsigned arithmetic issue

858e1de

Signed-off-by: Alina Liu <alinalq@dev-dsk-alinalq-2b-2db84246.us-west-2.amazon.com>

JimB123 approved these changes Aug 27, 2025

View reviewed changes

JimB123 requested changes Aug 27, 2025

View reviewed changes

Change unsigned type to fix compilation issue

7263a19

Signed-off-by: Alina Liu <alinalq@dev-dsk-alinalq-2b-2db84246.us-west-2.amazon.com>

asagege requested a review from JimB123 August 27, 2025 18:50

JimB123 approved these changes Aug 27, 2025

View reviewed changes

zuiderkwast approved these changes Aug 28, 2025

View reviewed changes

enjoy-binbin approved these changes Aug 29, 2025

View reviewed changes

zuiderkwast merged commit d6e011f into valkey-io:unstable Aug 29, 2025
52 checks passed

madolson reviewed Sep 2, 2025

View reviewed changes

Comment thread tests/unit/memefficiency.tcl

asagege mentioned this pull request Sep 2, 2025

Delete the previous comment explaining why changed latency from 5 to 40 #2574

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Reduce active defrag test latency by lowering hit threshold#2553

Reduce active defrag test latency by lowering hit threshold#2553
zuiderkwast merged 4 commits into
valkey-io:unstablefrom
asagege:defragStep

asagege commented Aug 27, 2025 •

edited by zuiderkwast

Loading

Uh oh!

Uh oh!

codecov Bot commented Aug 27, 2025 •

edited

Loading

Uh oh!

JimB123 left a comment

Uh oh!

JimB123 left a comment

Uh oh!

JimB123 left a comment

Uh oh!

zuiderkwast left a comment

Uh oh!

enjoy-binbin left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

asagege commented Aug 27, 2025 • edited by zuiderkwast Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JimB123 left a comment

Choose a reason for hiding this comment

Uh oh!

JimB123 left a comment

Choose a reason for hiding this comment

Uh oh!

JimB123 left a comment

Choose a reason for hiding this comment

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

enjoy-binbin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

asagege commented Aug 27, 2025 •

edited by zuiderkwast

Loading

codecov Bot commented Aug 27, 2025 •

edited

Loading