listpack: add lpFindInteger() to avoid string conversion in set integ… by liveprasad · Pull Request #3410 · valkey-io/valkey

liveprasad · 2026-03-27T11:54:32Z

Summary

Add lpFindInteger() to listpack.c to avoid an integer→string→integer
round-trip when searching for integers in listpack-encoded sets.

Previously, setTypeAdd() converted integers to strings to call lpFind(),
which then called string2ll() internally to convert back. This resolves
the existing TODO comment at src/t_set.c:168.

Changes

src/listpack.c — add lpFindInteger(lp, p, v, skip)
src/listpack.h — declare it
src/t_set.c — use it when str == tmpbuf; remove TODO comment
src/unit/test_listpack.cpp — add listpackLpFindInteger test

Fixes #3408

…er search When a set command like SADD receives an integer value and the set is listpack-encoded, the previous code converted the integer to a string to call lpFind(), which then converted it back to an integer internally. Add lpFindInteger() that accepts a long long directly and compares against integer-encoded listpack entries without any string conversion. Use it in setTypeAdd() when the value arrived as an integer (str == tmpbuf). Fixes valkey-io#3408 Signed-off-by: liveprasad <prasad.shetkar093@gmail.com>

codecov · 2026-03-30T08:36:19Z

Codecov Report

❌ Patch coverage is 85.71429% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.52%. Comparing base (b83209d) to head (7d2e056).
⚠️ Report is 1 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/listpack.c	73.68%	5 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #3410      +/-   ##
============================================
- Coverage     76.72%   76.52%   -0.20%     
============================================
  Files           159      159              
  Lines         79681    79715      +34     
============================================
- Hits          61135    61003     -132     
- Misses        18546    18712     +166

Files with missing lines	Coverage Δ
src/t_set.c	`98.06% <100.00%> (+<0.01%)`	⬆️
src/unit/test_listpack.cpp	`79.76% <100.00%> (+0.32%)`	⬆️
src/listpack.c	`93.02% <73.68%> (-0.10%)`	⬇️

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

dvkashapov

Not an expert in listpacks, just took a look at lpFind() for comparison and have a couple of questions.

dvkashapov · 2026-03-30T13:10:17Z

+            p += skip_entry_size;
+        }
+        /* Safety: ensure we don't walk past the end */
+        if (p >= lp + lp_bytes) break;


lpFind() has this guard after every add to p:

if (p + 8 >= lp + lp_bytes) lpAssertValidEntry(lp, lp_bytes, p); else assert(p >= lp + LP_HDR_SIZE && p < lp + lp_bytes); if (unlikely(p[0] == LP_EOF)) { /* EOF must only appear at the end of a listpack. */ assert(p + 1 == lp + lp_bytes); break; }

Do we also need LP_EOF check in lpFindInteger()?

Thanks and fixed

dvkashapov · 2026-03-30T13:13:41Z

+            skipcnt--;
+            uint64_t skip_entry_size;
+            lpGetWithSize(p, &ll, NULL, &skip_entry_size);
+            p += skip_entry_size;


lpFind() uses lpSkip(p) with the explicit comment:

/* Move to next entry, avoid use `lpNext` due to `lpAssertValidEntry` in * `lpNext` will call `lpBytes`, will cause performance degradation */ p = lpSkip(p);

Do we need to do the same here to not do a full get? Looks like lpSkip() reads only the encoding to determine entry length.

sarthakaggarwal97 · 2026-03-31T16:38:21Z

    lpFree(lp);
 }

+TEST_F(ListpackTest, listpackLpFindInteger) {


what happens if we try to add "42" (as string) and 42 as an integer. In existing behaviour, "42" should be read as 42 integer as well. Can we add a test so that "42" and 42 are not treated as separate elements?

Added the test case

- Use lpSkip() in the skip branch instead of lpGetWithSize() to avoid unnecessary entry decoding when skipping entries - Add LP_EOF check matching lpFind() pattern for safe listpack traversal - Add test case verifying string "42" and integer 42 find the same entry Fixes valkey-io#3408 Signed-off-by: liveprasad <prasad.shetkar093@gmail.com>

liveprasad

Done

zuiderkwast · 2026-04-10T08:20:12Z

+            if (str == tmpbuf) {
+                p = lpFindInteger(lp, p, llval, 0);


When we lookup by integer, can we even avoid converting to string above?

/* Convert int to string. */ len = ll2string(tmpbuf, sizeof tmpbuf, llval);

I don't know how to structur this in a nice way. Maybe move the listpack-with-integer handlig to the if (!str) { ... } block above?

Good point. To avoid ll2string() entirely for the listpack path,
we'd need to handle listpack inside the if (!str) block before
line 137. Would you prefer that restructuring(Not completely trivial though), or is the current
approach (avoiding the re-conversion inside lpFind) acceptable?

Hey any thoughts?

Signed-off-by: liveprasad <prasad.shetkar093@gmail.com>

sarthakaggarwal97

Thanks! @liveprasad

github-actions Bot assigned liveprasad Mar 27, 2026

dvkashapov reviewed Mar 30, 2026

View reviewed changes

sarthakaggarwal97 reviewed Mar 31, 2026

View reviewed changes

liveprasad commented Apr 8, 2026

View reviewed changes

zuiderkwast reviewed Apr 10, 2026

View reviewed changes

listpack: fix clang-format issues in test_listpack.cpp

172b89f

Signed-off-by: liveprasad <prasad.shetkar093@gmail.com>

liveprasad requested a review from zuiderkwast April 19, 2026 04:46

sarthakaggarwal97 approved these changes Apr 21, 2026

View reviewed changes

Merge branch 'unstable' into fix-3408-add-lpFindInteger

445e7e1

ShubhamTaple mentioned this pull request May 4, 2026

Improve setTypeAddAux() by adding lpFindInteger() redis/redis#15157

Open

Uh oh!

Conversation

liveprasad commented Mar 27, 2026

Summary

Changes

Uh oh!

codecov Bot commented Mar 30, 2026

Codecov Report

Uh oh!

dvkashapov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liveprasad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarthakaggarwal97 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants