Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Try to reduce cost of Async #101605

Merged
merged 2 commits into from
May 2, 2024
Merged

Try to reduce cost of Async #101605

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4649ccd
Try to reduce cost of Async
MichalStrehovsky Apr 26, 2024
596f681
Undo manual box
MichalStrehovsky Apr 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ...ries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncMethodBuilderCore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,12 @@ internal static string GetAsyncStateMachineDescription(IAsyncStateMachine stateM
return sb.ToString();
}

[MethodImpl(MethodImplOptions.NoInlining)]
internal static void LogTraceOperationBegin(Task t, Type stateMachineType)
{
TplEventSource.Log.TraceOperationBegin(t.Id, "Async: " + stateMachineType.Name, 0);
}

internal static Action CreateContinuationWrapper(Action continuation, Action<Action, Task> invokeAction, Task innerTask) =>
new ContinuationWrapper(continuation, invokeAction, innerTask).Invoke;

Expand Down
88 changes: 46 additions & 42 deletions ...ies/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncTaskMethodBuilderT.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,8 @@ private static IAsyncStateMachineBox GetStateMachineBox<TStateMachine>(
{
ExecutionContext? currentContext = ExecutionContext.Capture();

IAsyncStateMachineBox result;

// Check first for the most common case: not the first yield in an async method.
// In this case, the first yield will have already "boxed" the state machine in
// a strongly-typed manner into an AsyncStateMachineBox. It will already contain
Expand All @@ -168,9 +170,8 @@ private static IAsyncStateMachineBox GetStateMachineBox<TStateMachine>(
{
stronglyTypedBox.Context = currentContext;
}
return stronglyTypedBox;
result = stronglyTypedBox;
}

// The least common case: we have a weakly-typed boxed. This results if the debugger
// or some other use of reflection accesses a property like ObjectIdForDebugger or a
// method like SetNotificationForWaitCompletion prior to the first await happening. In
Expand All @@ -180,64 +181,67 @@ private static IAsyncStateMachineBox GetStateMachineBox<TStateMachine>(
// result in a boxing allocation when storing the TStateMachine if it's a struct, but
// this only happens in active debugging scenarios where such performance impact doesn't
// matter.
if (taskField is AsyncStateMachineBox<IAsyncStateMachine> weaklyTypedBox)
else if (taskField is AsyncStateMachineBox<IAsyncStateMachine> weaklyTypedBox)
{
// If this is the first await, we won't yet have a state machine, so store it.
Copy link
Member

@stephentoub stephentoub Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If we're trying to squeeze water from a stone, this whole block should be extremely cold (only used in debugging scenarios), so if you can come up with a good way to outline it all to a separate helper, that'd be reasonable. Not sure if that'd help much, though, given that stateMachine is generic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it would likely make things worse because it's still the same code, but with extra unwinding/GC tracking data structures because we have a new method.

if (weaklyTypedBox.StateMachine == null)
{
Debugger.NotifyOfCrossThreadDependency(); // same explanation as with usage below
weaklyTypedBox.StateMachine = stateMachine;
weaklyTypedBox.StateMachine = Unsafe.As<IAsyncStateMachine>(RuntimeHelpers.Box(ref Unsafe.As<TStateMachine, byte>(ref stateMachine), typeof(TStateMachine).TypeHandle));
MichalStrehovsky marked this conversation as resolved.
Show resolved Hide resolved
}

// Update the context. This only happens with a debugger, so no need to spend
// extra IL checking for equality before doing the assignment.
weaklyTypedBox.Context = currentContext;
return weaklyTypedBox;
result = weaklyTypedBox;
}

// Alert a listening debugger that we can't make forward progress unless it slips threads.
// If we don't do this, and a method that uses "await foo;" is invoked through funceval,
// we could end up hooking up a callback to push forward the async method's state machine,
// the debugger would then abort the funceval after it takes too long, and then continuing
// execution could result in another callback being hooked up. At that point we have
// multiple callbacks registered to push the state machine, which could result in bad behavior.
Debugger.NotifyOfCrossThreadDependency();

// At this point, taskField should really be null, in which case we want to create the box.
// However, in a variety of debugger-related (erroneous) situations, it might be non-null,
// e.g. if the Task property is examined in a Watch window, forcing it to be lazily-initialized
// as a Task<TResult> rather than as an AsyncStateMachineBox. The worst that happens in such
// cases is we lose the ability to properly step in the debugger, as the debugger uses that
// object's identity to track this specific builder/state machine. As such, we proceed to
// overwrite whatever's there anyway, even if it's non-null.
else
{
// Alert a listening debugger that we can't make forward progress unless it slips threads.
// If we don't do this, and a method that uses "await foo;" is invoked through funceval,
// we could end up hooking up a callback to push forward the async method's state machine,
// the debugger would then abort the funceval after it takes too long, and then continuing
// execution could result in another callback being hooked up. At that point we have
// multiple callbacks registered to push the state machine, which could result in bad behavior.
Debugger.NotifyOfCrossThreadDependency();

// At this point, taskField should really be null, in which case we want to create the box.
// However, in a variety of debugger-related (erroneous) situations, it might be non-null,
// e.g. if the Task property is examined in a Watch window, forcing it to be lazily-initialized
// as a Task<TResult> rather than as an AsyncStateMachineBox. The worst that happens in such
// cases is we lose the ability to properly step in the debugger, as the debugger uses that
// object's identity to track this specific builder/state machine. As such, we proceed to
// overwrite whatever's there anyway, even if it's non-null.
#if NATIVEAOT
// DebugFinalizableAsyncStateMachineBox looks like a small type, but it actually is not because
// it will have a copy of all the slots from its parent. It will add another hundred(s) bytes
// per each async method in NativeAOT binaries without adding much value. Avoid
// generating this extra code until a better solution is implemented.
var box = new AsyncStateMachineBox<TStateMachine>();
// DebugFinalizableAsyncStateMachineBox looks like a small type, but it actually is not because
// it will have a copy of all the slots from its parent. It will add another hundred(s) bytes
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be out of scope for this PR, but how about using an unspecialized DebugFinalizableAsyncStateMachineBox<IAsyncStateMachine> for NativeAOT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't know much about this - if it's only used by managed debuggers, it would not be useful for native AOT anyway. It's ifdeffed out so I'm not concerned about it.

// per each async method in NativeAOT binaries without adding much value. Avoid
// generating this extra code until a better solution is implemented.
var box = new AsyncStateMachineBox<TStateMachine>();
#else
AsyncStateMachineBox<TStateMachine> box = AsyncMethodBuilderCore.TrackAsyncMethodCompletion ?
CreateDebugFinalizableAsyncStateMachineBox<TStateMachine>() :
new AsyncStateMachineBox<TStateMachine>();
AsyncStateMachineBox<TStateMachine> box = AsyncMethodBuilderCore.TrackAsyncMethodCompletion ?
CreateDebugFinalizableAsyncStateMachineBox<TStateMachine>() :
new AsyncStateMachineBox<TStateMachine>();
#endif
taskField = box; // important: this must be done before storing stateMachine into box.StateMachine!
box.StateMachine = stateMachine;
box.Context = currentContext;
taskField = box; // important: this must be done before storing stateMachine into box.StateMachine!
box.StateMachine = stateMachine;
box.Context = currentContext;

// Log the creation of the state machine box object / task for this async method.
if (TplEventSource.Log.IsEnabled())
{
TplEventSource.Log.TraceOperationBegin(box.Id, "Async: " + stateMachine.GetType().Name, 0);
}
// Log the creation of the state machine box object / task for this async method.
if (TplEventSource.Log.IsEnabled())
{
AsyncMethodBuilderCore.LogTraceOperationBegin(box, stateMachine.GetType());
}

// And if async debugging is enabled, track the task.
if (Threading.Tasks.Task.s_asyncDebuggingEnabled)
{
Threading.Tasks.Task.AddToActiveTasks(box);
// And if async debugging is enabled, track the task.
if (Threading.Tasks.Task.s_asyncDebuggingEnabled)
{
Threading.Tasks.Task.AddToActiveTasks(box);
}
result = box;
}

return box;
return result;
}

#if !NATIVEAOT
Expand Down
Loading