Repository navigation
perf(runtime): wake the send pump through a reusable claim-token signal (issue #240) - #241
Conversation
The pump's idle wake-up raced two retained channel reads with Task.WhenAny, allocating two AsTask wrappers, the WhenAny promise, and continuation closures on every wake. Replace it with a reusable WakeupSignal (the DeadlineReadRace claim-token pattern without the timer): the pump publishes an arm token and re-checks both queues before sleeping, writers claim the token and complete the value-task source, and Stop/fault paths signal the pump to exit. The dual-queue protocol-progress isolation and the deadline-race path for custom MaxLatency are unchanged.
…token The initial signal design let the pump abandon an armed value task when the post-arm queue re-check found frames and re-arm immediately. With RunContinuationsAsynchronously, the abandoned arm's continuation could still be queued on the thread pool when the core was reset, tripping the ManualResetValueTaskSourceCore completion sentinel (InvalidOperationException, SIGABRT) — observed as the CI Load Smoke crash. Every completion is now arbitrated by one per-arm token CompareExchange: writers claim the token before SetResult, and WaitAsync consumes any signal that arrived before the arm was published (latched) through the same exchange. The pump always awaits the armed value task, so a continuation fully runs before the next arm, each arm completes exactly once, and a pre-arm signal can never leave the await hanging. Verified with 10x the CI sharedmemory smoke on the baremetal host plus the pump test classes and the full unit suite.
SunSi12138
left a comment
There was a problem hiding this comment.
No blocking findings. The second commit fixes the abandoned-arm MRVTSC reset hazard by making the pump always await the armed ValueTask and arbitrating completion through _armToken; the current PR Quick and CodeQL runs are green.
One non-blocking performance/correctness-of-the-primitive observation: Signal() always writes _signaled = 1 before attempting to claim the current _armToken, but a successful current-arm claim never consumes/clears that latch. Therefore every normal writer wake that completes an already-armed waiter leaves _signaled == 1; after the pump drains back to empty, the next WaitAsync() consumes that stale latch and completes synchronously, producing one deterministic extra empty pump iteration per real idle wake. This does not lose frames or double-complete the same arm, but it is unnecessary work in a PR whose purpose is to optimize the wake path. Please consider making the latch represent only a pre-arm/unclaimed signal (while preserving the signal-before-arm race), and add a deterministic regression test for the wake primitive/race if practical.
Also, the PR description still says the pump does a post-arm queue re-check, but the current head deliberately removed that re-check and relies on the latched pre-arm signal instead; the description should be updated to match the final protocol. Since the performance numbers were collected before the second commit changed the wake protocol, re-running the key c512 allocation measurement on the final head would make the evidence fully closed.
…nal residue - Move the nested private WakeupSignal out of RpcSession.SendPump into src/SharpLink.Runtime/WakeupSignal.cs as a top-level internal type so the wake primitive can be unit-tested directly. - Signal() now claims the arm token first and only writes the _signaled latch when no arm is claimable. A successful claim therefore leaves no stale latch behind, so the next WaitAsync can no longer complete spuriously and force an extra empty pump iteration per real wake (review feedback on #241). - Add WakeupSignalTests with three deterministic regression tests: claimed-arm residue, signal-before-arm latch, concurrent writers.
|
@SunSi12138 三条意见均已落实(commit
|
SunSi12138
left a comment
There was a problem hiding this comment.
Blocking finding on the new 90de99c WakeupSignal protocol: changing Signal() to claim-first / latch-second re-opens a lost-wakeup window between the pump's empty-queue check and WaitAsync() arm publication.
Concrete interleaving:
- Pump observes both queues empty and is about to call
WaitAsync(). - A producer enqueues a frame and enters
Signal(), reads_armToken == 0, then pauses before writing_signaled = 1. - Pump enters
WaitAsync(): resets the core, publishes arm tokenT, thenInterlocked.Exchange(ref _signaled, 0)reads 0. - Producer resumes, writes
_signaled = 1, and returns without re-checking the newly published arm. WaitAsync()returns a pending ValueTask backed by armT; the frame is already queued, but nobody completedT. With no later send/stop/fault signal, the pump can sleep indefinitely.
So the PR description's guarantee that a signal arriving with no claimable arm is consumed by the next WaitAsync() is not true for this crossing race: the latch write can occur just after that next wait has already consumed the latch. The new sequential SignalArrivingBeforeTheArm... test does not cover this interleaving.
Please make arm publication + pending-signal state participate in one atomic protocol (for example a single atomic state representing idle/latched/armed), or use an equivalent retry handshake after latching that cannot double-complete an arm. Add a deterministic regression for the exact crossing race if practical.
The previous stale-latch issue is otherwise addressed, the PR description/performance evidence is substantially improved, and the final-head PR Quick + CodeQL runs are green. Minor doc-only note: the description still says CI is '待结算', but both runs have completed successfully.
The claim-first/latch-second protocol introduced a lost-wakeup window: a writer could observe the idle state, pause, and write the latch just after WaitAsync had published the arm and consumed the (still empty) latch, leaving the queued frame with a pending arm that nobody ever completes. Encode the whole protocol in a single atomic state word: 0 = idle, 1 = a latched signal, and any value >= 2 = an armed waiter (arm token + 1). WaitAsync publishes the arm and consumes a pending latch in one atomic exchange; a writer either claims the live arm with one CAS or latches with Interlocked.Or, which keeps a concurrently published arm intact, and a re-check loop claims any arm the latch lands on, so a signal crossing the arm-publication boundary still completes the pending arm instead of being lost. Claiming always returns the state to idle, preserving the no-stale-latch property for the next arm. Add a deterministic regression test for the exact crossing interleaving via a test-only BeforeLatchWrite seam.
|
@SunSi12138 丢唤醒交叉窗口已确认属实,
|
SunSi12138
left a comment
There was a problem hiding this comment.
Re-review of final head 15cef89: the previous blocking lost-wakeup crossing is fixed. Folding latch + arm into one atomic state closes the writer-observes-idle / arm-publishes / late-latch window, and LateLatchCrossingArmPublicationStillCompletesTheArm deterministically exercises that exact interleaving. I do not see a remaining lost-wakeup, same-arm double-SetResult, or MRVTSC reset-before-completion blocker. PR Quick (including Load/Chaos Smoke) and CodeQL are green on this head.
One non-blocking state-encoding concern remains. _state documents arm values as token + 1, while the latch operation is Interlocked.Or(ref _state, 1). For an even arm value, e.g. generation 1 => state 2, OR-ing the latch bit changes it to 3 — which is also the nominal state of generation 2. So the latch write does not literally keep the arm token intact, and there is a small ABA/spurious-wake scenario: a delayed latch-path writer can turn arm 2 into 3, another writer claims/completes that arm, the pump later re-arms generation 2 as state 3, and the delayed writer's re-check can then CAS that new arm to idle and complete it even if its queued work was already drained. I don't see this causing a lost wake or duplicate completion of the same arm, so I am not treating it as blocking, but it weakens the generation-token invariant and the stated no-spurious-wake property. A cleaner encoding would reserve bit 0 exclusively for Latched, e.g. publish arms as (++generation << 1) and let OR 1 mark the same arm without aliasing the next generation.
Minor test-hygiene note: the direct WakeupSignal tests currently re-arm after checking IsCompletedSuccessfully without consuming the prior ValueTask via GetResult/await. Production does consume every arm before re-arming, so consuming the completed ValueTask in tests would model the actual MRVTSC contract more closely.
…ve bit 0 for the latch With token + 1 arm values, OR-ing the latch bit onto an even arm aliased the next generation's nominal armed value (gen1 | latch == 3 == gen2 nominal), weakening the per-generation token invariant. Publishing arms as ++generation << 1 keeps every armed value even, so arm | 1 stays that same arm and can never alias another generation. Tests now consume every completed arm via await before re-arming, matching the production pump's always-await MRVTSC contract.
|
@SunSi12138 两条意见均已落实(
最终 head
PR 描述已同步(协议段落、数字表、测试说明)。CI(quick/Analyze C#/CodeQL)在 |
SunSi12138
left a comment
There was a problem hiding this comment.
Re-review of final head 8b6c5c9: no blocking findings.
The previous arm/latch alias concern is resolved cleanly: armed states are now ++generation << 1, so bit 0 is reserved exclusively for the latch. arm | 1 remains tied to the same generation and cannot alias another generation's nominal arm. The prior crossing lost-wakeup invariant remains intact because the latch write still preserves a concurrently published arm and the re-check loop claims it. The tests also now consume each completed ValueTask before re-arming, matching the production pump's always-await MRVTSC contract.
PR Quick (including unit/integration/load/chaos smoke) and CodeQL are green on this head, and dev has not moved.
One documentation-only nit: the performance table still lists c512 as 836 B/op, while the final-head note below reports runs 828/841; their arithmetic mean is 834.5 B/op (roughly 835), so those two figures should be made consistent. This is non-blocking.
|
@SunSi12138 文档 nit 已修:性能表 c512 改为 835(−3.4%),与下方注释的 828/841(均值 834.5)一致;仅更新 PR 描述,无新提交。 |
Closes #240(分配回归排查的落地修复部分)
背景
issue #240 的排查经专家复核 +
GetAllocatedBytesForCurrentThread探针闭环,终审结论:回归 100% 归因于
8d4a1c9(issue #163 进度隔离)引入的双读 idle-wakeup 路径——WaitToReadAsync().AsTask()×2 +Task.WhenAny+AwaitFirstReadAsync异步助手,每次泵唤醒固定分配 ~312B(探针实测 802,148 次唤醒 × 312B = 250MB),低并发 unary 下
每个 RPC 触发 ~2 次唤醒。排水/Flush/TryPeek 均经探针证伪排除。
修改
WakeupSignal(src/SharpLink.Runtime/WakeupSignal.cs,internal,可直接单测):DeadlineReadRace 同款 claim-token +
ManualResetValueTaskSourceCore无定时器变体。最终协议(8b6c5c9):整个协议编码进单一原子状态字(0 = idle,1 = 已闩锁信号,
偶数 ≥2 = 已武装 arm(
++generation << 1),bit 0 专属闩锁位)。WaitAsync用一次原子 exchange 同时发布 arm 并消费先到闩锁;writer 要么一次 CAS 认领存活 arm,要么
Interlocked.Or写闩锁——Or 落在武装 arm 上得到arm | 1,仍是同一个 arm,不会与任何其它代的武装值别名;随后的复查循环认领闩锁落位到的任何 arm,因此闩锁写在 arm 发布
之后才落地(评审指出的丢唤醒交叉窗口)也仍会完成该 pending arm 而非丢失;认领一律
把状态归零,成功唤醒不为下一臂残留陈旧闩锁。泵永远 await、永不弃置;每 arm 恰好一次
SetResult(初版 CI Load Smoke 的 CompletionSentinel 崩溃已由 09ae81c 修复;claim-first
协议由 15cef89 重构为原子状态,8b6c5c9 再按评审意见消除 token+1 编码的跨代别名)。
WakeupSignalTests4 个确定性回归测试:成功认领不留残留、arm 前信号被闩锁同步消费、并发双写不双完成、晚到闩锁跨越 arm 发布仍必须完成该 arm(经测试专用
BeforeLatchWrite接缝精确复现评审给出的交叉交错)。测试对每个已完成的 arm 都先await消费再 re-arm,与生产泵的 always-await MRVTSC 契约一致。WaitForFramesAsync/AwaitFirstReadAsync与"被弃读观测"复杂度(issue Chaos smoke intermittently fails with unobserved SendPump "Transport output completed." tasks during server restart #216)。MaxLatencydeadline 竞态路径、双队列隔离语义原样保留。实测(242 裸机,taskset 8-15,gcServer=1,A/B ×2,最终 head 干净构建 + 校验和验证)
(最终 head 8b6c5c9:c1 两轮 856/856;c512 两轮 828/841,均值 834.5;BDN 16B/256B 四轮均为 760 B。)
全面反超 v1.1.1;与修复前协议持平——唤醒协议改动不改变稳态 B/op。
验证
请 review。