Repository navigation
Docs: say why sim never stages a prepared successor - #2235
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe pull request updates comments in the sim host runner. The comments document exclusive execution, unreachable depth-2 pipelining, dead overlap paths, and the storage shape of ChangesSim runner clarification
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~3 minutes Change: Other Merge Risk: ⚪ Minimal · up to This PR clarifies existing sim behavior and storage layout without changing runtime behavior, so it is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. A rabbit reads the runner’s note Comment |
41d09bf to
279158a
Compare
279158a to
a61429e
Compare
`supports_concurrent_native_prepare_ctx` returns a bare `0` on sim with nothing saying why, and it is the switch that makes every overlap-dependent path in the shared runner base dead code on that platform. The reason is not a policy choice: `simpler_prepare_run` takes `try_acquire_native_run` before it binds and rejects a second run with "another native run is active on this device context", and this base has no `try_reserve_native_run` for a successor to hold — reservation exists only onboard. Depth-2 pipelining is unreachable here, not switched off. The per-slot `host_phase_runs_` array carried the onboard justification, which does not hold on this side: it said a prepared successor binds while its predecessor owns the collectors, and sim has no prepared successor. The array is still the right shape — it keeps the storage identical to the onboard base so the shared host-phase code can index by the descriptor's slot without a per-platform branch — so the comment now says that instead. Comment-only; no behavior change. Both sim platforms compile, clang-format and cpplint are clean.
1 participant
Summary
supports_concurrent_native_prepare_ctxreturned a bare0on sim with nothing saying why — and it is the switch that makes every overlap-dependent path in the shared runner base dead code on that platform. Two comments, no code.Why sim opts out — established, not assumed
It is not a policy choice. Sim prepares under the exclusive execution claim:
simpler_launch_run(c_api_shared.cpp:930)simpler_prepare_run(c_api_shared.cpp:762)Sim's
simpler_prepare_runcallstry_acquire_native_runbefore it binds and rejects a second run with "another native run is active on this device context". And this base has notry_reserve_native_runfor a successor to hold — reservation exists only on the onboard base (device_runner_base.h:122).So depth-2 pipelining is unreachable here, not switched off. That distinction is the thing a reader needs and could not previously get from the
return 0.A stale justification I introduced in #2204
The per-slot
host_phase_runs_array in the sim base carried the onboard reasoning verbatim:True onboard, false on sim — sim has no prepared successor, which is exactly what the
return 0above reports. I copied it across in #2204 without noticing.The array is still the right shape: it keeps the storage identical to the onboard base so the shared host-phase code can index by the descriptor's slot with no per-platform branch. The comment now says that.
Testing
Comment-only; no behavior change.
I deliberately did not run the local scene-test gate: there is no behavior to exercise, the build is the proof, and CI runs the full matrix on any
src/change regardless. Flagging that explicitly rather than implying a gate I skipped.