Skip to content

Commit

Permalink
Address feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@mdh1418
mdh1418 committed Jul 9, 2024
1 parent a4cc7b0 commit 39716d3
Show file tree
Hide file tree
Showing 6 changed files with 135 additions and 109 deletions.
57 changes: 47 additions & 10 deletions src/coreclr/vm/proftoeeinterfaceimpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6855,6 +6855,13 @@ HRESULT ProfToEEInterfaceImpl::SuspendRuntime()
(LF_CORPROF,
LL_INFO1000,
"**PROF: SuspendRuntime\n"));
if (!IsCalledAsynchronously() && !AreCallbackStateFlagsSet(COR_PRF_CALLBACKSTATE_IN_TRIGGERS_SCOPE))
{
LOG((LF_CORPROF,
LL_ERROR,
"**PROF: ERROR: Returning CORPROF_E_UNSUPPORTED_CALL_SEQUENCE due to illegal asynchronous profiler call\n"));
return CORPROF_E_UNSUPPORTED_CALL_SEQUENCE;
}

if (!g_fEEStarted)
{
Expand Down Expand Up @@ -6887,6 +6894,13 @@ HRESULT ProfToEEInterfaceImpl::ResumeRuntime()
(LF_CORPROF,
LL_INFO1000,
"**PROF: ResumeRuntime\n"));
if (!IsCalledAsynchronously() && !AreCallbackStateFlagsSet(COR_PRF_CALLBACKSTATE_IN_TRIGGERS_SCOPE))
{
LOG((LF_CORPROF,
LL_ERROR,
"**PROF: ERROR: Returning CORPROF_E_UNSUPPORTED_CALL_SEQUENCE due to illegal asynchronous profiler call\n"));
return CORPROF_E_UNSUPPORTED_CALL_SEQUENCE;
}

if (!g_fEEStarted)
{
Expand Down Expand Up @@ -7644,6 +7658,13 @@ HRESULT ProfToEEInterfaceImpl::EnumerateGCHeapObjects(ObjectCallback callback, v
(LF_CORPROF,
LL_INFO1000,
"**PROF: EnumerateGCHeapObjects.\n"));
if (!IsCalledAsynchronously() && !AreCallbackStateFlagsSet(COR_PRF_CALLBACKSTATE_IN_TRIGGERS_SCOPE))
{
LOG((LF_CORPROF,
LL_ERROR,
"**PROF: ERROR: Returning CORPROF_E_UNSUPPORTED_CALL_SEQUENCE due to illegal asynchronous profiler call\n"));
return CORPROF_E_UNSUPPORTED_CALL_SEQUENCE;
}

if (callback == nullptr)
{
Expand All @@ -7655,22 +7676,38 @@ HRESULT ProfToEEInterfaceImpl::EnumerateGCHeapObjects(ObjectCallback callback, v
return CORPROF_E_RUNTIME_UNINITIALIZED;
}

bool ownEESuspension = FALSE;
if (!ThreadSuspend::SysIsSuspendInProgress() && (ThreadSuspend::GetSuspensionThread() == 0))
bool ownEESuspension = false;
bool suspendedByThisThread = (ThreadSuspend::GetSuspensionThread() == GetThreadNULLOk());
if (suspendedByThisThread && !g_profControlBlock.fProfilerRequestedRuntimeSuspend)
{
g_profControlBlock.fProfilerRequestedRuntimeSuspend = TRUE;
ThreadSuspend::SuspendEE(ThreadSuspend::SUSPEND_REASON::SUSPEND_FOR_PROFILER);
ownEESuspension = TRUE;
// This thread is responsible for suspending the runtime so we can't block
// waiting for the runtime to resume. However it wasn't the profiler call that did
// the suspend so we don't know what state the GC heap is in. Other threads also might
// be modifying it concurrently. In the future more analysis or coordination with other
// suspenders might let us narrow the scope of this error condition, but we have no need
// to do this now.
return CORPROF_E_SUSPENSION_IN_PROGRESS;
}
else if (!g_profControlBlock.fProfilerRequestedRuntimeSuspend)
else if (suspendedByThisThread && g_profControlBlock.fProfilerRequestedRuntimeSuspend)
{
return CORPROF_E_SUSPENSION_IN_PROGRESS;
// This thread previously invoked ICorProfiler::SuspendRuntime(). Our caller
// has the responsibility to resume the runtime no earlier than when this API returns
// and to preserve the GC heap in a consistent state. We should avoid invoking
// SuspendEE/ResumeEE again in this function because those APIs do not support
// re-entrant suspends.
}
else
{
// The profiler requested a runtime suspension before invoking this API,
// so the responsibility of resuming the runtime is outside the scope of this API.
// Given that the profiler owns the suspension, walking the GC Heap is safe.
_ASSERTE(!suspendedByThisThread);
// Its possible some background threads are suspending and resuming the runtime
// concurrently. We need to suspend the runtime on this thread to be certain the heap
// stays in a walkable state for the duration that we need it to. Our call to
// SuspendEE() may race with other threads by design and this thread may block
// arbitrarily long inside SuspendEE() for other threads to complete their own
// suspensions.
g_profControlBlock.fProfilerRequestedRuntimeSuspend = TRUE;
ThreadSuspend::SuspendEE(ThreadSuspend::SUSPEND_REASON::SUSPEND_FOR_PROFILER);
ownEESuspension = TRUE;
}

// Suspending EE ensures safe object inspection. We permit the GC Heap walk callback to
Expand Down
55 changes: 10 additions & 45 deletions src/tests/profiler/gcheapenumeration/gcheapenumeration.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,15 @@ class GCHeapEnumerationTests
static List<object> _rootObjects = new List<object>();

[DllImport("Profiler")]
private static extern void EnumerateGCHeapObjects();

[DllImport("Profiler")]
private static extern void EnumerateHeapObjectsInBackgroundThread();
private static extern void EnumerateGCHeapObjectsWithoutProfilerRequestedRuntimeSuspension();

[DllImport("Profiler")]
private static extern void EnumerateGCHeapObjectsWithinProfilerRequestedRuntimeSuspension();

public static int EnumerateGCHeapObjectsSingleThreadNoPriorSuspension()
{
_rootObjects.Add(new CustomGCHeapObject());
EnumerateGCHeapObjects();
EnumerateGCHeapObjectsWithoutProfilerRequestedRuntimeSuspension();
return 100;
}

Expand All @@ -37,39 +34,15 @@ public static int EnumerateGCHeapObjectsSingleThreadWithinProfilerRequestedRunti
return 100;
}

public static int EnumerateGCHeapObjectsInBackgroundThreadWithRuntimeSuspension()
// Test invoking ProfToEEInterfaceImpl::EnumerateGCHeapObjects during non-profiler requested runtime suspension, e.g. during GC
// ProfToEEInterfaceImpl::EnumerateGCHeapObjects should be invoked by the GarbageCollectionStarted callback
public static int EnumerateGCHeapObjectsMultiThreadWithCompetingRuntimeSuspension()
{
_rootObjects.Add(new CustomGCHeapObject());
EnumerateHeapObjectsInBackgroundThread();
GC.Collect();
return 100;
}

public static int EnumerateGCHeapObjectsMultiThreadWithCompetingRuntimeSuspension()
{
ManualResetEvent startEvent = new ManualResetEvent(false);
Thread gcThread = new Thread(() =>
{
startEvent.WaitOne();
GC.Collect();
});
gcThread.Name = "GC.Collect";
gcThread.Start();

Thread enumerateThread = new Thread(() =>
{
startEvent.WaitOne();
Thread.Sleep(1000);
_rootObjects.Add(new CustomGCHeapObject());
EnumerateGCHeapObjects();
});
enumerateThread.Name = "EnumerateGCHeapObjects";
enumerateThread.Start();

startEvent.Set();
return 100;
}

public static int Main(string[] args)
{
if (args.Length > 0 && args[0].Equals("RunTest", StringComparison.OrdinalIgnoreCase))
Expand All @@ -82,38 +55,30 @@ public static int Main(string[] args)
case nameof(EnumerateGCHeapObjectsSingleThreadWithinProfilerRequestedRuntimeSuspension):
return EnumerateGCHeapObjectsSingleThreadWithinProfilerRequestedRuntimeSuspension();

case nameof(EnumerateGCHeapObjectsInBackgroundThreadWithRuntimeSuspension):
return EnumerateGCHeapObjectsInBackgroundThreadWithRuntimeSuspension();

case nameof(EnumerateGCHeapObjectsMultiThreadWithCompetingRuntimeSuspension):
return EnumerateGCHeapObjectsMultiThreadWithCompetingRuntimeSuspension();
}
}

if (!RunProfilerTest(nameof(EnumerateGCHeapObjectsSingleThreadNoPriorSuspension), "FALSE"))
if (!RunProfilerTest(nameof(EnumerateGCHeapObjectsSingleThreadNoPriorSuspension), false))
{
return 101;
}

if (!RunProfilerTest(nameof(EnumerateGCHeapObjectsSingleThreadWithinProfilerRequestedRuntimeSuspension), "FALSE"))
if (!RunProfilerTest(nameof(EnumerateGCHeapObjectsSingleThreadWithinProfilerRequestedRuntimeSuspension), false))
{
return 102;
}

if (!RunProfilerTest(nameof(EnumerateGCHeapObjectsInBackgroundThreadWithRuntimeSuspension), "TRUE"))
if (!RunProfilerTest(nameof(EnumerateGCHeapObjectsMultiThreadWithCompetingRuntimeSuspension), true))
{
return 103;
}

if (!RunProfilerTest(nameof(EnumerateGCHeapObjectsMultiThreadWithCompetingRuntimeSuspension), "FALSE"))
{
return 104;
}

return 100;
}

private static bool RunProfilerTest(string testName, string shouldSetMonitorGCEventMask)
private static bool RunProfilerTest(string testName, bool shouldSetMonitorGCEventMask)
{
try
{
Expand All @@ -123,7 +88,7 @@ private static bool RunProfilerTest(string testName, string shouldSetMonitorGCEv
profileeArguments: testName,
envVars: new Dictionary<string, string>
{
{ShouldSetMonitorGCEventMaskEnvVar, shouldSetMonitorGCEventMask},
{ShouldSetMonitorGCEventMaskEnvVar, shouldSetMonitorGCEventMask ? "TRUE" : "FALSE"},
}
) == 100;
}
Expand Down
Loading

0 comments on commit 39716d3

Please sign in to comment.