Skip to content

Remove boxing for awaited custom awaiters - #3

Open
anurag6569201 wants to merge 1 commit into
qa/agent-dotnet-runtime/pr-03-131342/basefrom
qa/agent-dotnet-runtime/pr-03-131342/head
Open

anurag6569201 wants to merge 1 commit into
qa/agent-dotnet-runtime/pr-03-131342/basefrom
qa/agent-dotnet-runtime/pr-03-131342/head

Conversation

@anurag6569201

Copy link
Copy Markdown

This optimizes calls to AsyncHelpers.UnsafeAwaitAwaiter<TAwaiter>(TAwaiter) with struct awaiters to instead call a new function AsyncHelpers.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 later UnsafeOnCompleted call can then be done without any boxing by extracting it from the continuation.

This introduces a new GT_CONTINUATION_MEMBER_OFFSET which 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 AsyncAwaiter pseudo arg passed to UnsafeAwaitAwaiterInContinuation, 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_OFFSET in 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_OFFSET nodes.

Fix dotnet#119842

Microbenchmark

About 10% improvement, and more importantly, avoids the box that async1 also avoids to gain parity.

using System;
using System.Diagnostics;
using System.Runtime.CompilerServices;
using System.Threading;
using System.Threading.Tasks;

public static class Program
{
    static Action s_continuation;
    static long s_value;

    public static void Main()
    {
        for (int i = 0; i < 10; i++)
        {
            for (int j = 0; j < 100; j++)
            {
                Task t = Foo(100);
                while (!t.IsCompleted)
                    s_continuation();
            }

            Thread.Sleep(100);
        }

        for (int i = 0; i < 50; i++)
        {
            Task t = Foo(10_000_000);
            while (!t.IsCompleted)
                s_continuation();
        }
    }

    private static async Task Foo(int n)
    {
        s_value = 0;

        Stopwatch timer = Stopwatch.StartNew();
        for (int i = 0; i < n; i++)
        {
            await new Awaiter(i);
        }

        if (n > 100)
            Console.WriteLine("Took {0} ms", timer.ElapsedMilliseconds);

        Trace.Assert(s_value == ((long)n * (n - 1)) / 2);
    }

    private struct Awaiter : ICriticalNotifyCompletion
    {
        public int X;

        public Awaiter(int x) => X = x;

        public bool IsCompleted => false;
        public Awaiter GetAwaiter() => this;
        public void GetResult() { }

        public void OnCompleted(Action continuation)
        {
        }

        public void UnsafeOnCompleted(Action continuation)
        {
            s_value += X;
            s_continuation = continuation;
        }
    }
}
-Took 323 ms
-Took 318 ms
-Took 320 ms
-Took 323 ms
-Took 322 ms
-Took 318 ms
-Took 320 ms
-Took 318 ms
-Took 317 ms
-Took 319 ms
+Took 290 ms
+Took 291 ms
+Took 292 ms
+Took 288 ms
+Took 292 ms
+Took 291 ms
+Took 290 ms
+Took 290 ms
+Took 287 ms
+Took 292 ms

Source merge-base: dab7ae578c5fa991d6b04a7a62fa0948be0e6ff6
Source head: 5cef6fbf1597ef27b866e100f10a4a19f54be11d

@shipwright-agent

Copy link
Copy Markdown

⛔ Shipwright · Blocked

Recommendation: do not merge PR #3 · Tier T3
Checks: 0 total · 0 needing attention

Next step: resolve the blocking findings before merge.

Findings (7)

  • 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. · src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.cs:47
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • CRITICAL AwaiterContinuation and AwaiterOffset are stored in RuntimeAsyncStackState and cleared only in Push. · src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.CoreCLR.cs:270
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • CRITICAL StoreAsyncAwaiter removes the awaiter argument from the call and inserts stores into the suspension block. · src/coreclr/jit/async.cpp:2450
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • CRITICAL The runtime helper computes a generic instantiation and uses ComputeRuntimeLookupForSharedGenericToken with pResolvedToken from the original call site.
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • HIGH The new ContinuationMemberType::CustomAwaiterOfLayout and its associated layout compatibility logic are spread across async.cpp, corinfo.h, and the generated wrappers. · src/coreclr/jit/async.cpp:45
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun 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. · src/tests/async/awaitingnoasyncgeneric/awaitingnoasyncgeneric.csproj:1
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun 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 · src/coreclr/inc/jiteeversionguid.h:40
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

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 /shipwright rerun to verify again.

{
ref RuntimeAsyncAwaitState state = ref t_runtimeAsyncAwaitState;
Continuation? sentinelContinuation = state.SentinelContinuation ??= new Continuation();
state.StackState->AwaiterContinuation = &AwaiterOnCompletedFromContinuation<TAwaiter>;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment thread src/coreclr/jit/async.cpp
size_t memberIndex =
m_compiler->GetContinuationMemberIndex(ContinuationMember::CustomAwaiterOfLayout(awaiterLayout));
assert(memberIndex < layout.ContinuationMemberOffsets.size());
assert(layout.ContinuationMemberOffsets[memberIndex] != UINT_MAX);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 */

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant