Fix cache line false sharing on per-IO-thread stat counters by YangboLong · Pull Request #14907 · redis/redis

YangboLong · 2026-03-22T03:25:50Z

stat_io_reads_processed[] and stat_io_writes_processed[] were per-IO-thread arrays inside struct redisServer that suffer from false sharing. This PR moves the two stat counters into the IOThread struct, which is already __attribute__((aligned(CACHE_LINE_SIZE))). Each IO thread's counters now sit on a separate cache line, eliminating the cross-thread contention.

Added io_reads_processed and io_writes_processed fields to IOThread struct
Removed stat_io_reads_processed[] and stat_io_writes_processed[] from struct redisServer
Made IOThreads[] non-static with extern declaration in server.h
Updated callers across server.c, networking.c, replication.c

Note

Medium Risk
Touches multi-threaded I/O statistics accounting and makes IOThreads globally visible, which could introduce subtle concurrency or linkage issues if any thread ID/indexing assumptions are wrong.

Overview
Moves per-IO-thread read/write processed counters out of struct redisServer and into the cache-line-aligned IOThread struct (io_reads_processed, io_writes_processed) to reduce false sharing under threaded I/O.

Updates all call sites to increment/read/reset these counters via IOThreads[tid] (including main-thread replication reads) and exposes IOThreads[] as a non-static global with an extern declaration in server.h.

^{Reviewed by Cursor Bugbot for commit 4db925b. Bugbot is set up for automated code reviews on this repo. Configure here.}

augmentcode · 2026-03-22T03:29:43Z

🤖 Augment PR Summary

Summary: This PR reduces cache-line false sharing for per-IO-thread counters by moving them out of struct redisServer into cache-line-aligned storage.

Changes:

Introduce IOThreadStats aligned to CACHE_LINE_SIZE and a global io_thread_stats[] array.
Move stat_io_reads_processed, stat_io_writes_processed, and io_threads_clients_num into the new per-thread struct.
Update IO-thread client assignment and read/write accounting to use io_thread_stats across iothread.c, networking.c, and replication.c.
Adjust server init/reset and INFO “Threads” reporting to read the new counters.

_{🤖 Was this summary useful? React with 👍 or 👎}

augmentcode

Review completed. 2 suggestions posted.

Comment augment review to trigger a new review at any time.

augmentcode · 2026-03-22T03:29:44Z

    server.repl_good_slaves_count = 0;
    server.last_sig_received = 0;
-    memset(server.io_threads_clients_num, 0, sizeof(server.io_threads_clients_num));
+    memset(io_thread_stats, 0, sizeof(io_thread_stats));


memset(io_thread_stats, 0, ...) clears redisAtomic members via raw byte writes; with the C11 _Atomic backend this can be undefined/fragile compared to using the atomicSet* APIs. Consider initializing the atomic members using the atomic helpers instead of bulk memset.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

augmentcode · 2026-03-22T03:29:44Z

+typedef struct __attribute__((aligned(CACHE_LINE_SIZE))) {
+    redisAtomic long long io_reads_processed;  /* Number of read events processed */
+    redisAtomic long long io_writes_processed; /* Number of write events processed */
+    int clients_num;                           /* Number of clients assigned */


clients_num appears to be updated by the main thread (e.g., assignClientToIOThread() / keepClientInMainThread()), so the comment “Each thread updates its own entry” isn’t strictly accurate. With clients_num sharing the same cache line as the per-thread counters, these main-thread updates could still cause cache-line bouncing for busy IO threads.

Severity: low

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

fcostaoliveira · 2026-03-23T02:38:46Z

CE Performance Automation : step 1 of 2 (build) DONE.

This comment was automatically generated given a benchmark was triggered.
Started building at 2026-05-21 22:25:57.618202 and took 87 seconds.
You can check each build/benchmark progress in grafana:

git hash: 4db925b
git branch: YangboLong:fix_cache_line
commit date and time: n/a
commit summary: n/a
test filters:
- command priority lower limit: 0
- command priority upper limit: 10000
- test name regex: .*
- command group regex: .*

You can check a comparison in detail via the grafana link

fcostaoliveira · 2026-03-23T02:39:46Z

CE Performance Automation : step 2 of 2 (benchmark) RUNNING...

This comment was automatically generated given a benchmark was triggered.

Started benchmark suite at 2026-05-27 08:49:02.440283 and took 20976.615196 seconds up until now.
Status: [###################################---------------------------------------------] 43.42% completed.

In total will run 380 benchmarks.
- 215 pending.
- 165 completed:
- 164 successful.
- 1 failed.
You can check a the status in detail via the grafana link

ShooterIT · 2026-03-23T03:35:45Z

makes sense, i guess this optimization take effect when the IO thread is the bottleneck, right?

and would you like to have a look for the following variables

    redisAtomic long long stat_net_input_bytes; /* Bytes read from network. */
    redisAtomic long long stat_net_output_bytes; /* Bytes written to network. */

let each io thread have separate stas too, to check if there is an improvement.
(PS: i have tried, but there is no improvement, maybe i should test when the io thread is bottleneck)

themavik

review test

themavik

Colocating clients_num with the per-thread read/write atomics in io_thread_stats[] keeps hot counters off the same cache line as colder fields. nit: IOThreadStats only helps if the struct uses the same CACHE_LINE_SIZE padding pattern as other globals in server.h.

YangboLong · 2026-03-23T22:02:09Z

makes sense, i guess this optimization take effect when the IO thread is the bottleneck, right?

and would you like to have a look for the following variables
    redisAtomic long long stat_net_input_bytes; /* Bytes read from network. */
    redisAtomic long long stat_net_output_bytes; /* Bytes written to network. */
let each io thread have separate stas too, to check if there is an improvement. (PS: i have tried, but there is no improvement, maybe i should test when the io thread is bottleneck)

Yes I assume the same, when IO threads are concurrently writing their stat counters. For the scalar variables, since they are shared by all threads, true contention but not false sharing, shall we address it in a different PR after seeing the benchmark results first?

ShooterIT · 2026-03-25T12:50:54Z

true contention but not false sharing

yes, it is contention instead of false sharing. I was thinking we separate them into the stats (IOThreadStats) of different thread to avoid contention, it is okay to verify it in future PRs.

ShooterIT · 2026-03-25T12:52:31Z

+ * Stat counters are updated by their respective IO thread;
+ * clients_num is updated by the main thread but infrequently. */
+typedef struct __attribute__((aligned(CACHE_LINE_SIZE))) {
+    redisAtomic long long io_reads_processed;  /* Number of read events processed */


why not put them in struct IOThread

Currently IOThread is just used in iothread.c and IOThreads[] is declared as static instead of extern. Adding the stats counters into IOThread would also make them share cache lines with the other fields. Ideally the stats counters don't want to share cache lines even with clients_num i think, but the performance impact of doing so is probably neglectable.

i think we can remove static key word, I think we should put every variable on its own cache line, it is too waste. besides, besides, we may also add some fields into IOThreadStats, and generally only the io thread itself accesses its' fields

for clients_num, only the main thread accesses and changes them, maybe it is better to put them in server struct.

Are you suggesting a struct layout like below?

typedef struct __attribute__((aligned(CACHE_LINE_SIZE))) { redisAtomic long long io_reads_processed; redisAtomic long long io_writes_processed; redisAtomic long long net_input_bytes; redisAtomic long long net_output_bytes; // Other per-thread entries that are under contention } IOThreadStats;

This sounds fair to me.
@fcostaoliveira are the benchmark results available now?

yes, but i prefer to put them in IOThreads. or put IOThreadStats into IOThread, don't like to have separate variables, cc @sundb

we can handle the io_reads_processed and io_writes_processed first, and mitigate the contention of net_input/output_bytes variables in the future PR.

YangboLong · 2026-03-26T03:03:00Z

true contention but not false sharing

yes, it is contention instead of false sharing. I was thinking we separate them into the stats (IOThreadStats) of different thread to avoid contention, it is okay to verify it in future PRs.

In this PR it's fine too. Just want to see how the benchmark results look like with the current change.

YangboLong · 2026-04-28T02:40:27Z

The initial benchmark used 200 clients with small values, which may not create enough IO contention — results varied across sessions. I retested with 1000 clients and 256-byte values to shift the bottleneck to IO threads. Results are more consistent across repeated runs.

Setup: WSL2, 22 cores. Server: --io-threads 8, pinned to cores 0-8. Client: pinned to cores 9-21. 10 runs each.

	Median	Mean	Delta
Before SET	701,023	692,132
After SET	713,063	706,365	+1.72% median, +2.06% mean
Before GET	700,827	693,965
After GET	712,809	708,086	+1.71% median, +2.03% mean

ShooterIT

generally LGTM, please revert unrelated format changes.

ShooterIT · 2026-04-28T14:56:28Z

@fcostaoliveira could you please trigger some benchmark runs, especially when IO threads are bottleneck, maybe io-threads = 8

fcostaoliveira · 2026-05-06T14:22:25Z

Hi @YangboLong, ran the io-threads=8 string suite against this PR.

Setup

Runner: x86-aws-m7i.metal-24xl-2 (Sapphire Rapids, dedicated, no neighbour)
Topology: oss-standalone-08-io-threads
Baseline: redis/redis@7ecc04f59d (current unstable head)
PR head: YangboLong/redis@d85abc5e
Build: gcc 15.2.0, debian-bookworm
Tests: full string subset of redis/redis-benchmarks-specification that defines oss-standalone-08-io-threads — 31/32 landed (one test, mixed-50-50-with-expiration-pipeline-10-400_conns, failed on the PR-side build during this run; can re-trigger if needed).

Results — single-datapoint, sorted by Δ

Test (link to spec)	Baseline ops/s	PR ops/s	Δ
`5Mkeys-string-mget-100B-30keys-pipeline-10`	147,008	156,566	+6.50 %
`5Mkeys-string-mget-100B-100keys-pipeline-10`	38,570	40,583	+5.22 %
`1Mkeys-string-mixed-50-50-set-get-1KB`	604,593	631,346	+4.42 %
`1Mkeys-string-setget200c-512B-pipeline-10`	2,399,372	2,473,555	+3.09 %
`1Mkeys-string-mixed-50-50-set-get-with-expiration-240B-400_conns`	1,057,289	1,086,003	+2.72 %
`1Mkeys-string-setget2000c-1KiB-pipeline-16`	3,042,558	3,119,517	+2.53 %
`1Mkeys-string-setget2000c-1KiB-pipeline-10`	2,850,402	2,920,985	+2.48 %
`3Mkeys-string-get-with-1KiB-values-40_conns`	741,966	759,175	+2.32 %
`3Mkeys-string-mixed-20-80-with-512B-values-pipeline-10-2000_conns`	2,403,324	2,456,488	+2.21 %
`1Mkeys-string-setget200c-1KiB-pipeline-1`	611,167	624,477	+2.18 %
`3Mkeys-string-get-with-1KiB-values-pipeline-10-400_conns`	2,314,029	2,363,666	+2.15 %
`1Mkeys-string-set-get-short-expiration-512B`	1,253,117	1,280,071	+2.15 %
`3Mkeys-string-get-with-1KiB-values-pipeline-10-2000_conns`	2,481,633	2,523,204	+1.68 %
`5Mkeys-string-mget-512B-5keys-pipeline-10`	823,612	836,259	+1.54 %
`3Mkeys-string-mixed-20-80-with-512B-values-400_conns`	1,006,619	1,018,795	+1.21 %
`5Mkeys-string-mget-100B-5keys-pipeline-10`	902,669	912,999	+1.14 %
`3Mkeys-string-mixed-20-80-with-512B-values-pipeline-10-400_conns`	2,231,919	2,254,116	+0.99 %
`3Mkeys-string-get-with-1KiB-values-pipeline-10-40_conns`	2,012,963	2,031,604	+0.93 %
`3Mkeys-load-string-with-512B-values`	1,001,810	1,004,936	+0.31 %
`3Mkeys-string-mixed-20-80-with-512B-values-pipeline-10-5200_conns`	2,341,967	2,345,590	+0.15 %
`1Mkeys-string-setget200c-512B-pipeline-1`	623,460	624,018	+0.09 %
`5Mkeys-string-mget-10B-100keys-pipeline-10`	26,194	26,133	-0.23 %
`1Mkeys-string-setget200c-1KiB-pipeline-10`	2,440,202	2,426,942	-0.54 %
`1Mkeys-string-setget200c-4KiB-pipeline-1`	491,639	488,356	-0.67 %
`5Mkeys-string-mget-10B-100keys`	31,310	30,997	-1.00 %
`1Mkeys-string-setget200c-4KiB-pipeline-10`	627,099	619,295	-1.24 %
`1Mkeys-string-mixed-50-50-set-get-1KB-pipeline-10`	1,976,434	1,935,470	-2.07 %
`3Mkeys-string-get-with-1KiB-values-400_conns`	994,251	966,828	-2.76 %
`5Mkeys-string-mget-100B-2keys-pipeline-10`	1,585,748	1,518,762	-4.22 %
`3Mkeys-load-string-with-512B-values-pipeline-10`	2,473,333	2,363,874	-4.43 %
`5Mkeys-string-mget-1KiB-5keys-pipeline-10`	466,106	407,357	-12.60 % ⚠️

σ-confirmation on the −12.6 % outlier

Re-ran mget-1KiB-5keys-pipeline-10 once more to check for noise:

Run	Baseline ops/s	PR ops/s
1	466,106	407,357
2	489,970	433,471
mean	478,038	420,414

→ σ-disjoint (max(PR)=433,471 < min(BL)=466,106), Δ_mean = −12.05 %. Reproduces.

This is the only test in the suite that σ-confirmed a regression. Worth noting: it's directionally inconsistent with its sibling 5-key MGET variants on the same value-size axis (mget-100B-5keys-pipeline-10 +1.1 %, mget-512B-5keys-pipeline-10 +1.5 %), so it doesn't fit a "false-sharing-fix-is-a-mixed-bag" story — more likely a side effect of the IOThread struct layout change (e.g. eviction of hot adjacent state from L1 at the larger payload size). Happy to TMA-recon on our profiler runner if you want a microarchitectural breakdown before merging.

Summary

31 tests, arithmetic mean Δ = +0.52 %
12 wins ≥ +2 %, headlined by the high-key MGET pipelined cases (+5–7 %) — exactly the workload where the per-IO-thread stat counter ping-pong was hottest.
5 tests ≤ −2 %: 1 σ-confirmed (mget-1KiB-5keys-pipeline-10, −12 %), 4 single-datapoint dips that don't fit a false-sharing mechanism (likely noise; can re-confirm if you want).

Verdict from our side: net positive, but the σ-confirmed −12 % on mget-1KiB-5keys-pipeline-10 is real and stands as the one caveat to a clean merge. Your call whether to dig further or accept it given the broader wins.

YangboLong · 2026-05-11T20:42:19Z

@fcostaoliveira do you want to trigger another mget-1KiB-5keys-pipeline-10 run with the corrected pipeline config, as you mentioned in the other pr?

paulorsousa · 2026-05-21T13:55:33Z

Hi @YangboLong — re-ran with the corrected pipeline config (the spec was missing --pipeline 10 in clientconfig.arguments; fix at redis/redis-benchmarks-specification#394). Bumped sample size to 5 each side.

Setup

Runner: x86-aws-m7i.metal-24xl-2 (Sapphire Rapids, dedicated)
Topology: oss-standalone-08-io-threads
Baseline: redis/redis@ba1a4b2c (current unstable head)
PR head: YangboLong/redis@d85abc5e
5 datapoints per side

Results — corrected pipeline=10 operating point

Stat	Baseline ops/s	PR ops/s
median	878,146	782,836
mean	881,012	787,294
stdev (cv)	6,867 (0.78 %)	9,655 (1.23 %)
[min, max]	[874,394, 891,029]	[781,494, 804,411]

→ σ-disjoint (max(PR)=804,411 < min(BL)=874,394). Δ_median = −10.85 %, Δ_mean = −10.64 %. Variance is tight on both sides (cv < 1.3 %), so the gap isn't a noise artifact.

Compared to the May 6 numbers (which ran at effective pipeline=1 due to the spec bug): throughput shifted up ~2× as expected for pipeline-10, and the regression magnitude narrowed slightly (−12.6 % → −10.8 % median) but it didn't go away. It also remains specific to this 1KiB × 5-keys × pipeline=10 cell — every sibling pipeline-10 MGET variant on the same matrix went +1–7 %.

paulorsousa · 2026-05-21T14:05:19Z

@YangboLong could you merge unstable into this branch, please? There are ~110 commits since this PR branched, including #15133 (batched MGET/MSET dict prefetch) – directly on the hot path here, so it may be inflating the baseline rather than the PR regressing. Once you do it I can run this specific benchmark again :)

YangboLong · 2026-05-21T22:29:45Z

@paulorsousa please test again.

paulorsousa · 2026-05-22T09:57:10Z

@YangboLong rebase did it. Same 5 datapoints per side on x86-aws-m7i.metal-24xl-2 / oss-standalone-08-io-threads:

	mean ops/s	median ops/s	stdev (cv)
Baseline `ba1a4b2c` (n=10)	879,214	879,084	5,979 (0.68 %)
PR rebased `4db925b2` (n=5)	885,944	887,565	5,687 (0.64 %)

Δ_median = +0.96 %, Δ_mean = +0.77 % — within noise (ranges overlap, cv < 0.7 % on both sides). The previous −10.85 % was the PR branch missing #15133 from the baseline.

LGTM from a perf standpoint on this test 👍

…ne-10 (v0.3.18) (#394) * fix(test-suites): add missing --pipeline 10 to mget-1KiB-5keys-pipeline-10 The clientconfig.arguments for `memtier_benchmark-5Mkeys-string-mget-1KiB-5keys-pipeline-10` was missing the `--pipeline 10` flag (sibling specs at other value sizes — 100B, 512B — all carry it). Without the flag, memtier_benchmark defaulted to pipeline=1, so the test was not exercising the pipelined-MGET path its name advertises. This made the run produced for redis/redis#14907 misleading: the −12.6 % "regression" on this spec was at effective pipeline=1, while every other pipeline-10 MGET sibling on the same matrix showed +1–7 % wins under proper pipelining. Bump to 0.3.18. * revert stale pyproject.toml version bump (keep --pipeline 10 spec fix) The branch bumped pyproject 0.3.17->0.3.18, which now conflicts with main (already at 0.3.23). Restore pyproject.toml to main so only the test-suite fix (adding the missing --pipeline 10 to the pipeline-10 mget spec) remains. --------- Co-authored-by: fcostaoliveira <filipe@redis.com>

augmentcode Bot reviewed Mar 22, 2026

View reviewed changes

sundb added this to Redis 8.8 Mar 23, 2026

github-project-automation Bot moved this to Todo in Redis 8.8 Mar 23, 2026

sundb added the action:run-benchmark Triggers the benchmark suite for this Pull Request label Mar 23, 2026

themavik reviewed Mar 23, 2026

View reviewed changes

ShooterIT reviewed Mar 25, 2026

View reviewed changes

YangboLong requested a review from ShooterIT April 28, 2026 02:41

ShooterIT reviewed Apr 28, 2026

View reviewed changes

ShooterIT approved these changes Apr 29, 2026

View reviewed changes

ShooterIT added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label May 6, 2026

ShooterIT removed the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label May 7, 2026

fcostaoliveira mentioned this pull request May 8, 2026

Batched MGET/MSET dict prefetch with dictType-driven payload hints #15133

Merged

sundb removed this from Redis 8.8 May 21, 2026

sundb added this to Redis 8.10 May 21, 2026

github-project-automation Bot moved this to Todo in Redis 8.10 May 21, 2026

paulorsousa mentioned this pull request May 21, 2026

fix(test-suites): add missing --pipeline 10 to mget-1KiB-5keys-pipeline-10 (v0.3.18) redis/redis-benchmarks-specification#394

Merged

1 task

YangboLong added 6 commits May 21, 2026 18:24

Fix cache line false sharing

bd506d2

Use atomicSet instead of memset

67d6dd5

Address pr comments

6b15f6e

Move per-thread stats into IOThread

432602f

Revert leftover changes

6f52a98

Revert code format changes

4db925b

YangboLong force-pushed the fix_cache_line branch from d85abc5 to 4db925b Compare May 21, 2026 22:25

ShooterIT merged commit 138263a into redis:unstable May 26, 2026
18 checks passed

github-project-automation Bot moved this from Todo to Done in Redis 8.10 May 26, 2026

Uh oh!

Conversation

YangboLong commented Mar 22, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

augmentcode Bot commented Mar 22, 2026

Uh oh!

augmentcode Bot left a comment

Choose a reason for hiding this comment

Uh oh!

augmentcode Bot Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

augmentcode Bot Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

fcostaoliveira commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CE Performance Automation : step 1 of 2 (build) DONE.

Uh oh!

fcostaoliveira commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CE Performance Automation : step 2 of 2 (benchmark) RUNNING...

Uh oh!

ShooterIT commented Mar 23, 2026

Uh oh!

themavik left a comment

Choose a reason for hiding this comment

Uh oh!

themavik left a comment

Choose a reason for hiding this comment

Uh oh!

YangboLong commented Mar 23, 2026

Uh oh!

ShooterIT commented Mar 25, 2026

Uh oh!

ShooterIT Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

YangboLong Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShooterIT Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

YangboLong Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

ShooterIT Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

YangboLong commented Mar 26, 2026

Uh oh!

YangboLong commented Apr 28, 2026

Uh oh!

ShooterIT left a comment

Choose a reason for hiding this comment

Uh oh!

ShooterIT commented Apr 28, 2026

Uh oh!

fcostaoliveira commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YangboLong commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulorsousa commented May 21, 2026

Uh oh!

paulorsousa commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YangboLong commented May 21, 2026

Uh oh!

paulorsousa commented May 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

YangboLong commented Mar 22, 2026 •

edited by cursor Bot

Loading

fcostaoliveira commented Mar 23, 2026 •

edited

Loading

fcostaoliveira commented Mar 23, 2026 •

edited

Loading

YangboLong Mar 26, 2026 •

edited

Loading

fcostaoliveira commented May 6, 2026 •

edited

Loading

YangboLong commented May 11, 2026 •

edited

Loading

paulorsousa commented May 21, 2026 •

edited

Loading