Skip to content

stream: group bool fields at tail of serverStream to eliminate false sharing with mu - #9361

Merged
mbissa merged 2 commits into
grpc:masterfrom
gidotencate:stream-group-bools-server-stream
Aug 27, 2026
Merged

stream: group bool fields at tail of serverStream to eliminate false sharing with mu#9361
mbissa merged 2 commits into
grpc:masterfrom
gidotencate:stream-group-bools-server-stream

Conversation

@gidotencate

@gidotencate gidotencate commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Reorder `serverStream` fields so both bool fields are grouped at the tail of the struct, and move `mu` to a separate cache line from the tail bools.

Why

`serverStream` had 2 `bool` fields placed between aligned fields, each forcing alignment padding before the next field:

Field Padding after
`recvFirstMsg` 7 B (before `int`, align 8)
`serverHeaderBinlogged` 3 B (before `sync.Mutex`, align 4)

Total: 8 B wasted. Grouping both bools at the tail eliminates the interior padding, reducing `serverStream` from 256 B → 248 B, saving 8 B per server-side RPC stream unconditionally.

Additionally, `serverHeaderBinlogged` is written unsynchronised in `SendHeader` and `Send` (it is documented as not needing synchronization). In the tail-grouped layout, the bools land at offset 240+ — which was the same cache line as `mu` (offset 232, cache line 3: 192–255). Concurrent writes to `serverHeaderBinlogged` invalidate the cache line before every `mu.Lock()`, adding 5–21× latency overhead under concurrent load.

Moving `mu` to immediately before `trInfo` (which it guards) places it at offset 184 (cache line 2: 128–191). The tail bools remain at offset 240+ (cache line 3), so the false-sharing is eliminated. No size change from this step — `serverStream` stays at 248 B.

See benchmarks in #9349.

Benchmark — `mu.Lock()` latency with stressor goroutines writing `serverHeaderBinlogged` concurrently:

Stressors Original layout After tail-group only After moving mu to CL2
0 3.9 ns 3.9 ns 3.7 ns
1 20.6 ns (5.2×) 20.1 ns (5.1×) 3.8 ns (~1×)
2 46.1 ns (11.7×) 45.8 ns (11.7×) 3.7 ns (~1×)
4 81.9 ns (20.8×) 79.9 ns (20.4×) 3.7 ns (~1×)

Code clarity

Both bools are unguarded. The bool group uses a single `// Not guarded by mu` section, preserving the existing per-field comments. `mu` is placed immediately before `trInfo` (which it guards) with a comment explaining the cache-line placement.

Closes #9349

RELEASE NOTES:

  • core: reorder `serverStream` fields to eliminate alignment padding (saving 8 bytes per server-side RPC stream) and cache-line false sharing between `serverHeaderBinlogged` and `mu` (up to 21× `mu.Lock()` speedup under concurrent load).

…ent padding

Two bool fields in serverStream were placed between aligned fields, each
forcing alignment padding before the next field:

  recvFirstMsg           @ 168  7 B padding (before int, align 8)
  serverHeaderBinlogged  @ 240  3 B padding (before sync.Mutex, align 4)

Grouping both bools at the tail reduces serverStream from 256 B to 248 B,
saving 8 B per server-side RPC stream unconditionally.

Fixes grpc#9349
@gidotencate
gidotencate force-pushed the stream-group-bools-server-stream branch from 8ba065c to ea29db8 Compare August 24, 2026 16:17
@gidotencate

Copy link
Copy Markdown
Contributor Author

@easwars @mbissa Could one of you assign this PR (and linked issue #9349) to @gidotencate? I don't have write access to assign myself.

@codecov

codecov Bot commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.50%. Comparing base (9d1988d) to head (b91c821).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9361      +/-   ##
==========================================
+ Coverage   87.45%   87.50%   +0.05%     
==========================================
  Files         425      425              
  Lines       30297    30303       +6     
==========================================
+ Hits        26496    26517      +21     
+ Misses       3801     3786      -15     
Files with missing lines Coverage Δ
stream.go 88.43% <ø> (+0.23%) ⬆️

... and 25 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@eshitachandwani eshitachandwani left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM , adding @mbissa as a second reviewer.

Comment thread stream.go Outdated
@eshitachandwani
eshitachandwani requested a review from mbissa August 26, 2026 08:58
@gidotencate
gidotencate force-pushed the stream-group-bools-server-stream branch from 6058ba1 to 869d5bc Compare August 26, 2026 10:01
@gidotencate

Copy link
Copy Markdown
Contributor Author

Done, removed in the latest commit.

serverHeaderBinlogged (written unsynchronised in SendHeader and Send) and
mu shared cache line 3 (192–255) after the initial tail-grouping commit.
Concurrent writes to serverHeaderBinlogged invalidate the cache line before
every mu.Lock(), adding ~5–21× overhead under concurrent load.

Moving mu to immediately before trInfo places it at offset 184 (cache line 2:
128–191).  The tail bools remain at offset 240+ (cache line 3), so stressor
writes no longer affect mu.Lock() latency.

Benchmark before this commit (serverHeaderBinlogged stressor):
  stressors=0   3.9 ns
  stressors=1  20.1 ns  (5.2×)
  stressors=4  79.9 ns (20.4×)

Benchmark after (different cache lines):
  stressors=0  3.7 ns
  stressors=1  3.8 ns  (~1×)
  stressors=4  3.7 ns  (~1×)

No size change: serverStream remains 248 B.

Fixes grpc#9349
@gidotencate
gidotencate force-pushed the stream-group-bools-server-stream branch from 869d5bc to b91c821 Compare August 26, 2026 11:00
@mbissa

mbissa commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request reorganizes the fields of the serverStream struct in stream.go to group boolean fields at the end, eliminating alignment padding and reducing the struct's memory footprint. There are no review comments, and we have no additional feedback to provide.

@mbissa mbissa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@mbissa
mbissa merged commit 31ccdcc into grpc:master Aug 27, 2026
19 checks passed
@gidotencate
gidotencate deleted the stream-group-bools-server-stream branch August 31, 2026 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: Performance Performance improvements (CPU, network, memory, etc)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

stream: serverStream has 2 scattered bools wasting 8B and causing false sharing with mu

4 participants