Conversation
The hit count is bounded by the wall-clock duration of the AOF replay, so the threshold measures runner speed rather than correctness. Assert that defrag ran and relocated allocations during loading instead. Fixes valkey-io/valkey#3954 Signed-off-by: Madelyn Olson <matolson@amazon.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Active defrag - AOF loadingassertsactive_defrag_hits > 80000right afterdebug loadaof, but that count is bounded by how long the replay takes in wall-clock time: defrag only runs fromwhileBlockedCron, which returns early unless a millisecond has elapsed (src/server.c:1822), and its duty cycle is computed as a percentage of the elapsed wait (src/defrag.c:1071). The replay duration is set bykey-load-delay -25, a 1-in-25 sampledusleep(1)per replayed command (src/aof.c:1705,src/debug.c:2619), so the threshold really measures how expensiveusleep(1)is on the runner. Making that sleep 16x rarer (key-load-delay -400) drops the count to 33k-70k and fails 5 out of 5 loops locally on an unmodified build. This asserts the property the comment above it already describes, that defrag ran and relocated allocations during loading, and drops the absolute count.Details
Reproduction
Emulating a runner where
usleep(1)is cheap. Locally on x86-64,active_defrag_hitsscales with the replay wall time:key-load-delayactive_defrag_hitstotal_active_defrag_time(ms)-25(as committed)-100-4000(no sleep)validate_latency 500on the line above passes in every one of those runs, so nothing else in the test is disturbed.Applied as a scratch patch to make the knob settable:
Before, 5 of 5 loops fail:
After, same knob, 5 of 5 loops pass:
The
putsand thekey-load-delayknob are scratch instrumentation and are not in this PR.Why not raise or lower the threshold
1c5572fed(#2402) already ratcheted this once, from> 100000down to> 80000, with the message "During AOF loading, we only hit 95k items rather than 100k in one defrag test". Any absolute number picked from an observed value is a number about one machine. There is no floor that both survives a fast runner and means anything: the observed range here spans 974 to 133929 hits on a single unchanged binary, purely as a function of the replay duration.Why the failures are 32-bit only
Each hit is one relocation gated by the jemalloc hint, and a pointer with no hint increments
misses, nothits(allocatorShouldDefrag,src/defrag.c:169). Which pointers jemalloc offers depends on slab utilization, and slab packing for these 250-byte objects differs on a 32-bit build, which lands 32-bit runners closer to the threshold to begin with.What is still asserted
config resetstatontests/unit/memefficiency.tcl:292zeroestotal_active_defrag_time(src/server.c:2854) along with the hit counters, andactivedefrag nocloses the window on the line afterdebug loadaof, so all three values still cover only thedebug loadaofwindow. A regression where defrag never starts while blocked, or starts and relocates nothing, still fails. Held at the extreme: even withkey-load-delay 0, wheretotal_active_defrag_timerounds to 0 ms,active_defrag_hitswas ~1000, so the check has real margin against a "defrag never ran" regression.This was generated by AI but verified, with love, by a human.