Skip to content

Optimize exact XTRIM MAXLEN zero#4161

Open
sarthakaggarwal97 wants to merge 1 commit into
valkey-io:unstablefrom
sarthakaggarwal97:perf/xtrim-maxlen-zero
Open

Optimize exact XTRIM MAXLEN zero#4161
sarthakaggarwal97 wants to merge 1 commit into
valkey-io:unstablefrom
sarthakaggarwal97:perf/xtrim-maxlen-zero

Conversation

@sarthakaggarwal97

@sarthakaggarwal97 sarthakaggarwal97 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Exact XTRIM key MAXLEN = 0 removes every stream entry while retaining the stream key and its metadata. The generic path removes radix-tree nodes individually and repeatedly re-positions its iterator, even though the final entry tree must be empty.

This change adds a fast path for exact scenario.

Workload Baseline RPS Candidate RPS Change
XTRIM MAXLEN = 0 14,875.81 36,910.15 +148.12%

Found this fast path while debugging with Codex!

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

streamTrim() now fast-paths exact unlimited MAXLEN 0 trimming by replacing the radix tree and resetting metadata. A unit test verifies deletion count, key retention, and empty-stream metadata.

Changes

Stream trimming

Layer / File(s) Summary
MAXLEN zero fast path
src/t_stream.c, tests/unit/type/stream.tcl
Exact unlimited MAXLEN 0 trimming clears all entries, resets stream length and first ID, returns the deleted count, and verifies the stream key and metadata remain present.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: cjx-zar, smkher, murphyjacob4, jsoref, ronen-kalish

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: an exact XTRIM MAXLEN=0 optimization.
Description check ✅ Passed The description accurately describes the new fast path and its performance impact.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sarthakaggarwal97
sarthakaggarwal97 marked this pull request as ready for review July 13, 2026 21:19

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/unit/type/stream.tcl (1)

722-732: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Exercise multiple radix-tree nodes.

Two entries do not guarantee multiple nodes, so this test does not prove that the entire tree is cleared. Force a small stream-node-max-entries, add entries across several nodes, then assert XRANGE is empty and radix-tree-keys is zero before restoring the configuration.

Suggested test expansion
     r DEL mystream
-    r XADD mystream 1-0 f v
-    r XADD mystream 2-0 f v
-    assert_equal 2 [r XTRIM mystream MAXLEN = 0]
+    r config set stream-node-max-entries 1
+    for {set j 1} {$j <= 3} {incr j} {
+        r XADD mystream $j-0 f v
+    }
+    assert_equal 3 [r XTRIM mystream MAXLEN = 0]
     assert_equal 1 [r EXISTS mystream]
     set reply [r XINFO STREAM mystream]
     assert_equal 0 [dict get $reply length]
-    assert_equal "2-0" [dict get $reply last-generated-id]
+    assert_equal {} [r XRANGE mystream - +]
+    assert_equal 0 [dict get $reply radix-tree-keys]
+    assert_equal "3-0" [dict get $reply last-generated-id]
     assert_equal "0-0" [dict get $reply recorded-first-entry-id]
+    r config set stream-node-max-entries 100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/type/stream.tcl` around lines 722 - 732, Update the test XTRIM
with MAXLEN 0 keeps an empty stream key to temporarily set
stream-node-max-entries to a small value, add enough entries to span multiple
radix-tree nodes, and restore the configuration afterward. After trimming,
retain the existing stream metadata assertions and also verify XRANGE returns no
entries and radix-tree-keys is zero.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/unit/type/stream.tcl`:
- Around line 722-732: Update the test XTRIM with MAXLEN 0 keeps an empty stream
key to temporarily set stream-node-max-entries to a small value, add enough
entries to span multiple radix-tree nodes, and restore the configuration
afterward. After trimming, retain the existing stream metadata assertions and
also verify XRANGE returns no entries and radix-tree-keys is zero.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0313e46e-71fb-4439-8f7e-82236d05c6c8

📥 Commits

Reviewing files that changed from the base of the PR and between cdf98a2 and 9556b2f.

📒 Files selected for processing (2)
  • src/t_stream.c
  • tests/unit/type/stream.tcl

@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.85%. Comparing base (cdf98a2) to head (9556b2f).

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #4161      +/-   ##
============================================
+ Coverage     76.82%   76.85%   +0.03%     
============================================
  Files           162      162              
  Lines         81433    81441       +8     
============================================
+ Hits          62561    62594      +33     
+ Misses        18872    18847      -25     
Files with missing lines Coverage Δ
src/t_stream.c 94.53% <100.00%> (+0.02%) ⬆️

... and 21 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.

@sarthakaggarwal97

sarthakaggarwal97 commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

@enjoy-binbin any concerns do you see? should we merge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

2 participants