Restart async workload after GlobalCleanup re-entry (MemoryRandomization)#3103
Conversation
|
This is output from copilot, so not sure if this is the correct approach. |
|
It would be good to add a test for this case.
This will affect memory allocation measurements. I'm not sure the best way to handle it. My initial thought was just implement a custom |
|
Maybe a simpler way would be to create the workload continuer in [Edit] Or more simply, we can create it in |
Two issues with async benchmarks under MemoryRandomization=true: 1. The engine re-invokes __GlobalCleanup/__GlobalSetup between iterations to randomize the managed heap. The async workload's WorkloadValueTaskSource was lazily created in WorkloadActionNoUnroll on first call and never recreated, so the second iteration would call Continue() on a source already in a terminal state and crash with System.InvalidOperationException from ManualResetValueTaskSourceCore.SignalCompletion. 2. If the workload itself threw, __WorkloadCore would catch and SetException on the source. A subsequent __GlobalCleanup that unconditionally called Complete() would double-signal the source and mask the original exception with InvalidOperationException. Fix: allocate the WorkloadValueTaskSource and start the async workload loop in __GlobalSetup (not lazily in the measured WorkloadActionNoUnroll hot path), and null it out in __GlobalCleanup. Because the engine calls __GlobalSetup again between iterations under MemoryRandomization, the next iteration always gets a fresh source allocated *outside* the MemoryDiagnoser-measured window. WorkloadActionNoUnroll becomes branchless and allocation-free. Add IsContinuerPending to WorkloadValueTaskSource and use it to guard the Complete() call in __GlobalCleanup, so we skip the signal when the workload has already SetException'd.
DrewScoggins
force-pushed
the
fix-async-workload-restart-with-memory-randomization
branch
from
4805ed6 to
a8e2310
Compare
|
Decided to go with the second suggestion. |
timcassell
left a comment
timcassell
left a comment
There was a problem hiding this comment.
Also need to update both InProcess toolchains.
| "public long invokeCount;" | ||
| ]; | ||
|
|
||
| protected override string PrependExtraGlobalSetupImpl(string impl) |
There was a problem hiding this comment.
It should be Append to setup, Prepend to cleanup.
| {impl} | ||
| => $$""" | ||
| if (this.__fieldsContainer.workloadContinuerAndValueTaskSource != null | ||
| && this.__fieldsContainer.workloadContinuerAndValueTaskSource.IsContinuerPending) |
There was a problem hiding this comment.
The guard is unnecessary.
| // is awaiting the next iteration. When the workload threw or the loop has already exited, | ||
| // calling SetResult(true) again on an already-signaled source would throw. | ||
| public bool IsContinuerPending | ||
| => continuerSource.GetStatus(continuerSource.Version) == ValueTaskSourceStatus.Pending; |
2 participants
When
MemoryRandomization=true,Engine.RunCorecallsRandomizeManagedHeapMemorybetween iterations, which invokesGlobalCleanupActionfollowed byGlobalSetupAction. The generated__GlobalCleanuppreviously calledworkloadContinuerAndValueTaskSource?.Complete()to signal the async workload loop to exit, but left the field non-null in a terminal state. The next iteration'sWorkloadActionNoUnrollthen calledContinue()on the same source, which callsSetResult(false)on aManualResetValueTaskSourceCorethat had already been signaled, throwingInvalidOperationExceptionfromSignalCompletion. This broke every async benchmark that opted into memory randomization.Fix:
WorkloadValueTaskSource.IsContinuerPendingso the generated cleanup can detect whether the workload loop is still awaiting the next iteration. CallingComplete()when not pending would mask the original exception (e.g. when the workload itself threw and the loop has already exited via the catch block) with a double-signalInvalidOperationException.PrependExtraGlobalCleanupImplto:Complete()when the continuer source is pending.workloadContinuerAndValueTaskSource = nullafter signalling.The existing
WorkloadActionNoUnrollcodegen already handles the null case by allocating a freshWorkloadValueTaskSourceand restarting__StartWorkload(), so iterations after a between-iterationGlobalCleanupnow run against a fresh, pending source.