Skip IO-thread read-done followup unless update_state sync-invokes handlers (9.0)

[quanyeyang]

Follow-up of #3335 (9.0 port of #3611). Fixes a regression introduced by that PR, mirroring upstream #4401.

After the postpone-mask method landed, processIOThreadsReadDone() started postponing READ and returning needs_post_read_update = 1 for every non-ACCEPTING completed read. That second phase (lookupClientByID + processPendingCommandAndInputBuffer + connUpdateState) is only required when update_state may synchronously invoke handlers. For other transports it is per-completion overhead, hitting small payloads with io-threads and shallow pipelines the hardest.

This restores the original gate without bringing struct client into the connection driver:

• ConnectionType.sync_handlers_in_update_state (0 by default)
• Set only where update_state can sync-call handlers (RDMA)
• Mask is still computed in networking.c from IO state
• connUpdateState() still runs immediately, including ACCEPTING

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4ee78858-3eda-41e6-872f-351bb11617fc

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (21ba658) to head (a0f551e).

Additional details and impacted files

@@     Coverage Diff     @@
##   9.0   #4414   +/-   ##
===========================
===========================

🚀 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.

[github-actions]

Benchmark ran on this commit: a0f551e

RPS Benchmark Comparison: 9.0 vs 760a0a1

7 significant change(s)

• ✅ +29±5% GET rps pipe=1 threads=9 (data_size=16)
• ✅ +25±3% SET rps pipe=1 threads=9 (data_size=16)
• ✅ +24±5% SET rps pipe=1 threads=9 (data_size=128)
• ✅ +23±1% GET rps pipe=1 threads=9 (data_size=128)
• ✅ +6±2% GET rps pipe=10 threads=9 (data_size=128)
• ✅ +6±4% GET rps pipe=10 threads=9 (data_size=16)
• ✅ +4±3% SET rps pipe=10 threads=9 (data_size=128)

9 with no significant change

Click to expand full comparison tables

data_size = 128

	% Change	Test	9.0	760a0a1	9.0 stats	760a0a1 stats
➖	-1±2%	GET rps P1 T1	218K	216K	n=3, σ=1.50K, CV=0.7%, CI95%=±1.7%, PI95%=±3.4%	n=3, σ=1.15K, CV=0.5%, CI95%=±1.3%, PI95%=±2.6%
✅	+23±1%	GET rps P1 T9	933K	1.149M	n=3, σ=1.24K, CV=0.1%, CI95%=±0.3%, PI95%=±0.7%	n=3, σ=4.56K, CV=0.4%, CI95%=±1.0%, PI95%=±2.0%
➖	-0±2%	GET rps P10 T1	1.104M	1.102M	n=3, σ=8.44K, CV=0.8%, CI95%=±1.9%, PI95%=±3.8%	n=3, σ=4.17K, CV=0.4%, CI95%=±0.9%, PI95%=±1.9%
✅	+6±2%	GET rps P10 T9	1.81M	1.927M	n=3, σ=12.8K, CV=0.7%, CI95%=±1.8%, PI95%=±3.5%	n=3, σ=7.18K, CV=0.4%, CI95%=±0.9%, PI95%=±1.9%
➖	+1±2%	SET rps P1 T1	208K	209.5K	n=3, σ=1.73K, CV=0.8%, CI95%=±2.1%, PI95%=±4.1%	n=3, σ=557, CV=0.3%, CI95%=±0.7%, PI95%=±1.3%
✅	+24±5%	SET rps P1 T9	1.005M	1.24M	n=3, σ=9.68K, CV=1.0%, CI95%=±2.4%, PI95%=±4.8%	n=3, σ=15.2K, CV=1.2%, CI95%=±3.0%, PI95%=±6.1%
➖	-1±2%	SET rps P10 T1	1.003M	996K	n=3, σ=4.70K, CV=0.5%, CI95%=±1.2%, PI95%=±2.3%	n=3, σ=6.94K, CV=0.7%, CI95%=±1.7%, PI95%=±3.5%
✅	+4±3%	SET rps P10 T9	1.5802M	1.64M	n=3, σ=669, CV=0.0%, CI95%=±0.1%, PI95%=±0.2%	n=3, σ=20.1K, CV=1.2%, CI95%=±3.0%, PI95%=±6.1%

data_size = 16

	% Change	Test	9.0	760a0a1	9.0 stats	760a0a1 stats
➖	-0±3%	GET rps P1 T1	228K	227K	n=3, σ=1.61K, CV=0.7%, CI95%=±1.8%, PI95%=±3.5%	n=3, σ=1.83K, CV=0.8%, CI95%=±2.0%, PI95%=±4.0%
✅	+29±5%	GET rps P1 T9	1.11M	1.442M	n=3, σ=15.6K, CV=1.4%, CI95%=±3.5%, PI95%=±7.0%	n=3, σ=7.04K, CV=0.5%, CI95%=±1.2%, PI95%=±2.4%
➖	+0±4%	GET rps P10 T1	1.22M	1.22M	n=3, σ=17.9K, CV=1.5%, CI95%=±3.7%, PI95%=±7.3%	n=3, σ=12.7K, CV=1.0%, CI95%=±2.6%, PI95%=±5.2%
✅	+6±4%	GET rps P10 T9	2.33M	2.47M	n=3, σ=12.1K, CV=0.5%, CI95%=±1.3%, PI95%=±2.6%	n=3, σ=34.0K, CV=1.4%, CI95%=±3.4%, PI95%=±6.9%
➖	+0±2%	SET rps P1 T1	219.8K	221K	n=3, σ=904, CV=0.4%, CI95%=±1.0%, PI95%=±2.0%	n=3, σ=1.62K, CV=0.7%, CI95%=±1.8%, PI95%=±3.7%
✅	+25±3%	SET rps P1 T9	944K	1.177M	n=3, σ=5.04K, CV=0.5%, CI95%=±1.3%, PI95%=±2.7%	n=3, σ=7.25K, CV=0.6%, CI95%=±1.5%, PI95%=±3.1%
➖	+0±1%	SET rps P10 T1	1.038M	1.040M	n=3, σ=3.62K, CV=0.3%, CI95%=±0.9%, PI95%=±1.7%	n=3, σ=4.39K, CV=0.4%, CI95%=±1.0%, PI95%=±2.1%
➖	+4±4%	SET rps P10 T9	1.74M	1.81M	n=3, σ=24.2K, CV=1.4%, CI95%=±3.4%, PI95%=±6.9%	n=3, σ=13.8K, CV=0.8%, CI95%=±1.9%, PI95%=±3.8%

---

Configuration:

• architecture: aarch64
• benchmark_mode: duration
• clients: 1600
• cluster_mode: False
• duration: 180
• tls: False
• valkey_benchmark_threads: 90
• warmup: 30

Legend:

• Test column: Command, metric, P=pipeline depth, T=io-threads
• Significance: ✅ significant improvement, ❌ significant regression, ➖ not significant, ❔ insufficient data

Statistical Notes:

• CV: Coefficient of Variation - relative variability (σ/μ × 100%)
• CI95%: 95% Confidence Interval - range where the true population mean is likely to fall
• PI95%: 95% Prediction Interval - range where a single future observation is likely to fall

[VinayakGhai]

Wait, upstream had a bug where we kept rescheduling read followups. Skipping it makes sense. But the condition update_state sync-invokes handlers feels like a hack. Is there a cleaner way to track whether the transport needs a post-read update? Let's check if this is safe for TLS connections.

[quanyeyang]

Wait, upstream had a bug where we kept rescheduling read followups. Skipping it makes sense. But the condition update_state sync-invokes handlers feels like a hack. Is there a cleaner way to track whether the transport needs a post-read update? Let's check if this is safe for TLS connections.

Thanks for review, I understand this is a little hack, because in the first approach we introduce this method and for cleaner abtraction we remove this and introduce new performance regression, and also currently unstable uses the same approach in this PR.

maybe we could think out of a clean and elegant way to solve this, but it needs to be synchronized with unstable, then it's ok.

also maybe the function name connUpdateStateMayInvokeHandlers sounds a little hack indeed.

[zuiderkwast]

Thanks,

yes, first let's fix the regression, so I want to merge this ASAP.

Later, we can try to find a better abstraction.