No actionable comments were generated in the recent review. 🎉

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.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Codecov Report

❌ Patch coverage is 98.41270% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 76.60%. Comparing base (f7b236d) to head (c15b111).

@@             Coverage Diff              @@
##           unstable    #3938      +/-   ##
============================================
- Coverage     76.68%   76.60%   -0.09%     
============================================
  Files           162      162              
  Lines         80731    80727       -4     
============================================
- Hits          61910    61842      -68     
- Misses        18821    18885      +64     

@coderabbitai Can you please review this for the perf regression fix for #3544 (reverted in #3756 due to ~20% SET regression) with the performance fix from #3760.

#!/bin/bash
# Fetch the PR diff
gh pr view 3938 --repo valkey-io/valkey --json files,body,title

# Get the diff
gh pr diff 3938 --repo valkey-io/valkey 2>/dev/null | head -3000

#!/bin/bash
# Check io_private_inbox initialization for thread 0
rg -n "io_private_inbox" --type c

# Check tryOffloadFreeObjToIOThreads full context
rg -n "tryOffloadFreeObjToIOThreads" --type c -A 20 -B 5

# Check freeClient return value semantics
rg -n "int freeClient" --type c -A 5

# Check stat_active_time usage
rg -n "stat_active_time" --type c -C 3

# Check decrRefCount for thread safety concerns
rg -n "decrRefCount" src/io_threads.c -C 3

# Check if copyright header was intentionally removed from queues.h
git log --oneline -5 -- src/queues.h

@roshkhatri: Thanks for the detailed PR description and benchmark data — the root-cause analysis is solid. Below is my review.

Overall Assessment

The core perf fix is correct and well-motivated. The decrRefCount path in the IO thread is safe because tryOffloadFreeObjToIOThreads gates on refcount == 1 and OBJ_ENCODING_RAW | OBJ_STRING, so the IO thread always takes the single-owner fast-path through freeStringObject → sdsfree, which is safe with jemalloc's cross-thread zfree. The trySendWriteToIOThreads deferral of last_header = NULL to post-enqueue also correctly closes the race. Queue sizing moving to runtime parameters is a clean improvement.

A few things worth addressing:

1. evictClients — "Evicting client" log fires before outcome is known

// src/networking.c
serverLog(LL_NOTICE, "Evicting client: %s", ci);
if (freeClient(c)) server.stat_evictedclients++;

The log message is printed before freeClient(c) is called. If the client is protected or has pending IO, freeClient returns 0 and the client is only scheduled for async close — but the log already says "Evicting client". This can mislead operators into thinking the eviction happened immediately. Consider logging after the fact, or differentiating between immediate and async:

sds ci = catClientInfoString(sdsempty(), c, server.hide_user_data_from_log);
if (freeClient(c)) {
    serverLog(LL_NOTICE, "Evicting client: %s", ci);
    server.stat_evictedclients++;
} else {
    serverLog(LL_NOTICE, "Scheduling async eviction for client: %s", ci);
}
sdsfree(ci);

2. evictClients — stat_evictedclients semantics change

Previously stat_evictedclients was incremented unconditionally for all eviction attempts (including async). Now it counts only immediate frees. This is arguably more correct, but it's a behavioral change in the metric that operators and monitoring tools may rely on. Worth a comment in the code explaining the new semantics, and potentially a changelog/docs note.

3. queues.h — Copyright/license header accidentally removed

The diff removes lines 1–4 of queues.h:

-/*
- * Copyright (c) Valkey Contributors
- * All rights reserved.
- * SPDX-License-Identifier: BSD-3-Clause
-```
This looks unintentional — all other files in `src/` retain the license header.

---

### 4. Minor: `_Static_assert` message fix is correct

```diff
-_Static_assert(JOB_REQ_COUNT <= 8, "JOB_REQ_COUNT must not exceed 7 for pointer arithmetic");
+_Static_assert(JOB_REQ_COUNT <= 8, "JOB_REQ_COUNT must not exceed 8 for pointer arithmetic");

Good catch — the old message said "7" but the limit is 8. ✓

5. Minor: IO_IGNITION_MAIN_THREAD_ACTIVE_PERCENT is still compile-time

The PR converts queue sizes to runtime parameters, but the ignition threshold remains a compile-time #define. This is fine for now, but if you ever want to allow runtime tuning (e.g., via config set), keeping it alongside the other IO-thread knobs would be natural. Not blocking.

Summary

The three items above are the ones I'd recommend addressing before merge. The perf fix and cleanups are solid work.

Benchmark ran on this commit: c6b38ac

RPS Benchmark Comparison: unstable vs 4a035db

1 significant change(s)

• ✅ +7±2% SET rps pipe=1 threads=9 (data_size=16)

15 with no significant change

@akashkgit please review when you get a chance.