Remove boxing for awaited custom awaiters - #3
anurag6569201 wants to merge 1 commit into
Conversation
Source PR: dotnet#131342 Source head: 5cef6fb
⛔ Shipwright · BlockedRecommendation: do not merge PR #3 · Tier
Findings (7)
Fireworks usage: 49,152 input · 987 output · 50,139 total tokens · $0.0115 · 17s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
| { | ||
| ref RuntimeAsyncAwaitState state = ref t_runtimeAsyncAwaitState; | ||
| Continuation? sentinelContinuation = state.SentinelContinuation ??= new Continuation(); | ||
| state.StackState->AwaiterContinuation = &AwaiterOnCompletedFromContinuation<TAwaiter>; |
There was a problem hiding this comment.
Shipwright · CRITICAL
The new awaiter-in-continuation path stores a struct awaiter into the continuation and later reinterprets raw continuation bytes as TAwaiter via Unsafe.As.
Impact: The new awaiter-in-continuation path stores a struct awaiter into the continuation and later reinterprets raw continuation bytes as TAwaiter via Unsafe.As. If the awaiter layout contains GC references, the GC bitmap must be exact; any mismatch between the JIT-computed layout and the runtime's view of the continuation object will cause heap corruption or premature collection. The diff adds bitmapBuilder.SetType for t…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| [NonVersionable] | ||
| public void Push(RuntimeAsyncStackState* stackState) | ||
| { | ||
| stackState->AwaiterContinuation = null; |
There was a problem hiding this comment.
Shipwright · CRITICAL
AwaiterContinuation and AwaiterOffset are stored in RuntimeAsyncStackState and cleared only in Push.
Impact: AwaiterContinuation and AwaiterOffset are stored in RuntimeAsyncStackState and cleared only in Push. If a suspension is reused or resumed through a path that does not call Push, stale AwaiterContinuation/AwaiterOffset can be used for a different awaiter, causing the wrong continuation callback or an out-of-bounds offset to be invoked. The diff does not show clearing these fields on resume or reuse paths.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| size_t memberIndex = | ||
| m_compiler->GetContinuationMemberIndex(ContinuationMember::CustomAwaiterOfLayout(awaiterLayout)); | ||
| assert(memberIndex < layout.ContinuationMemberOffsets.size()); | ||
| assert(layout.ContinuationMemberOffsets[memberIndex] != UINT_MAX); |
There was a problem hiding this comment.
Shipwright · CRITICAL
StoreAsyncAwaiter removes the awaiter argument from the call and inserts stores into the suspension block.
Impact: StoreAsyncAwaiter removes the awaiter argument from the call and inserts stores into the suspension block. For non-FIELD_LIST awaiters it uses gtNewStoreValueNode with the awaiter layout, but for FIELD_LIST it manually stores each field. If the awaiter is a struct with overlapping fields, explicit layout, or padding, the field-by-field store may not reproduce the original value, and the later Unsafe.As<byte, TAwa…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| @@ -1,4 +1,4 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk.IL"> | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
There was a problem hiding this comment.
Shipwright · HIGH
The test project changes from Microsoft.NET.Sdk.IL to Microsoft.NET.Sdk for many async tests, but the diff does not show any corresponding build validation or explanation.
Impact: The test project changes from Microsoft.NET.Sdk.IL to Microsoft.NET.Sdk for many async tests, but the diff does not show any corresponding build validation or explanation. This could silently change how these tests are compiled and mask regressions in the IL-based async test coverage.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| 0x36d1, | ||
| 0x475f, | ||
| {0x97, 0xb9, 0x3b, 0x2e, 0xeb, 0x45, 0x33, 0xb0} | ||
| constexpr GUID JITEEVersionIdentifier = { /* 305cdd16-2cee-49af-ae7c-2537eb37cae9 */ |
There was a problem hiding this comment.
Shipwright · HIGH
The new JIT/EE interface method getAwaitAwaiterInContinuationCall is added to the JITEE version GUID, but the diff does not show a corresponding version check or fallback for older
Impact: The new JIT/EE interface method getAwaitAwaiterInContinuationCall is added to the JITEE version GUID, but the diff does not show a corresponding version check or fallback for older runtimes. Mixing a new JIT with an older EE or vice versa will result in calling a vtable slot that does not exist, causing a crash or arbitrary code execution.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
This optimizes calls to
AsyncHelpers.UnsafeAwaitAwaiter<TAwaiter>(TAwaiter)with struct awaiters to instead call a new functionAsyncHelpers.UnsafeAwaitAwaiterInContinuation<TAwaiter>(int offset). The idea is that the JIT ensures that the awaiter will be present in the continuation at the specified offset. The laterUnsafeOnCompletedcall can then be done without any boxing by extracting it from the continuation.This introduces a new
GT_CONTINUATION_MEMBER_OFFSETwhich is used to solve the linking problem where we do not know the offset into the continuation until much later. The async transformation is responsible for replacing this node with a constant after it knows the offset.There is a new
AsyncAwaiterpseudo arg passed toUnsafeAwaitAwaiterInContinuation, and expanded in the suspension path by the async transformation to be stored in the continuation at the right offset.I have a couple of use cases for
GT_CONTINUATION_MEMBER_OFFSETin mind, so I have made the mechanism to represent the type of member easily expandable (particularly inlining needs this as well).This PR also removes configurable continuation reuse. This is now unconditionally enabled, to simplify the expansion of
GT_CONTINUATION_MEMBER_OFFSETnodes.Fix dotnet#119842
Microbenchmark
About 10% improvement, and more importantly, avoids the box that async1 also avoids to gain parity.
Source merge-base:
dab7ae578c5fa991d6b04a7a62fa0948be0e6ff6Source head:
5cef6fbf1597ef27b866e100f10a4a19f54be11d