Lazy sync of Thread::m_LastThrownObjectHandle from ExInfo::m_exception#127649
Lazy sync of Thread::m_LastThrownObjectHandle from ExInfo::m_exception#127649max-charlamb wants to merge 4 commits intodotnet:mainfrom
Conversation
…eagerly Stop eagerly synchronizing m_LastThrownObjectHandle with ExInfo::m_exception during active exception dispatch. Instead, set LTO lazily when the ExInfo is destroyed in PopExInfos. This simplifies the code and makes m_exception the sole source of truth during dispatch. Removed: SafeSetThrowables, SafeUpdateLastThrownObject, SyncManagedExceptionState, and the InternalUnhandledExceptionFilter re-sync block. Made SetLastThrownObject private; all external callers now use SafeSetLastThrownObject which handles OOM gracefully. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@EgorBot -intel -amd using BenchmarkDotNet.Attributes;
using System.Runtime.CompilerServices;
[MemoryDiagnoser]
public class ExceptionBench
{
[Benchmark]
public int SimpleThrowCatch()
{
try { throw new InvalidOperationException("x"); }
catch (Exception e) { return e.Message.Length; }
}
[Benchmark]
public int Rethrow()
{
try
{
try { throw new InvalidOperationException("x"); }
catch { throw; }
}
catch (Exception e) { return e.Message.Length; }
}
[Benchmark]
public int CatchAndThrowNew()
{
try
{
try { throw new InvalidOperationException("inner"); }
catch { throw new ApplicationException("outer"); }
}
catch (Exception e) { return e.Message.Length; }
}
[Benchmark]
public int NestedThrowCatch()
{
int r = 0;
try { Outer(); }
catch (Exception e) { r = e.Message.Length; }
return r;
}
[MethodImpl(MethodImplOptions.NoInlining)]
static void Outer()
{
try { Inner(); }
catch (InvalidOperationException) { throw new ApplicationException("rewrapped"); }
}
[MethodImpl(MethodImplOptions.NoInlining)]
static void Inner() => throw new InvalidOperationException("x");
[Benchmark]
public int DeepThrowCatch()
{
try { Recurse(20); return 0; }
catch (Exception e) { return e.Message.Length; }
}
[MethodImpl(MethodImplOptions.NoInlining)]
static void Recurse(int n)
{
if (n == 0) throw new InvalidOperationException("deep");
Recurse(n - 1);
}
}The interesting cases here:
Expecting the rethrow/catch-and-throw-new paths to show the largest delta since those are the dispatch transitions where |
There was a problem hiding this comment.
Pull request overview
This PR refactors CoreCLR exception state tracking to avoid eagerly synchronizing Thread::m_LastThrownObjectHandle (LTO) with ExInfo::m_exception during active exception dispatch, instead updating LTO lazily when ExInfo entries are popped. The intent is to reduce per-throw overhead after ExInfo::m_exception became the primary GC-tracked exception object during dispatch (per #127300).
Changes:
- Update LTO lazily in
ExInfo::PopExInfosfromExInfo::m_exceptionas each tracker is popped. - Remove eager-sync helpers/paths (
SafeSetThrowables,SafeUpdateLastThrownObject,SyncManagedExceptionState) and switch call sites toSafeSetLastThrownObject. - Restrict direct
Thread::SetLastThrownObjectusage by making it private and routing external callers throughSafeSetLastThrownObject(now optionally marks unhandled).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/threads.h | Updates LTO documentation, removes eager-sync APIs, makes SetLastThrownObject private, extends SafeSetLastThrownObject signature. |
| src/coreclr/vm/threads.cpp | Removes SafeSetThrowables/SafeUpdateLastThrownObject implementations; updates thread teardown to clear LTO via SafeSetLastThrownObject. |
| src/coreclr/vm/exinfo.cpp | Adds lazy LTO update when popping ExInfo entries. |
| src/coreclr/vm/exceptionhandling.cpp | Removes eager LTO sync at first-chance notification and removes SyncManagedExceptionState call after catch funclets. |
| src/coreclr/vm/excep.cpp | Removes unhandled-exception filter resync of LTO against the active tracker. |
| src/coreclr/vm/eepolicy.cpp | Routes fatal error/SO paths through SafeSetLastThrownObject(..., TRUE) instead of SetLastThrownObject / SafeSetThrowables. |
| src/coreclr/vm/comutilnative.cpp | Uses SafeSetLastThrownObject in FailFast path. |
The reviewer noted that PopExInfos now reads m_exception (an OBJECTREF) and calls Thread::SafeSetLastThrownObject, which requires MODE_COOPERATIVE for non-NULL throwables, without an explicit GC mode contract. All six callers were already cooperative: - exceptionhandling.cpp:601 - explicit GCX_COOP() three lines above - CleanUpForSecondPass callers (lines 2084/2139/2148) - the asm stub puts the thread in COOP before entering, and the SO path uses GCX_COOP_NO_DTOR - CallCatchFunclet (line 3185) - has MODE_COOPERATIVE CONTRACTL itself - ResumeAtInterceptionLocation (line 3300) - catch-dispatch path also calls PopExplicitFrames, which already requires cooperative mode Add the contract explicitly so that future callers cannot violate it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cleans up dead code identified by @jkotas in PR dotnet#127649 review: - Delete Thread::SetLastThrownObjectHandle: zero callers post-PR. - Delete SetManagedUnhandledExceptionBit forward declaration: had no definition anywhere in the codebase. - Remove the always-NULL pThrowableIn parameter from NotifyAppDomainsOfUnhandledException. - Replace UpdateCurrentThrowable (whose name was misleading: it never updated anything, only returned a boolean) with an inline check at the single call site using the GC-mode-agnostic IsThrowableNull / IsLastThrownObjectNull helpers, removing the need for a GCX_COOP inside the surrounding PAL_TRY block. - Merge SafeSetLastThrownObject into SetLastThrownObject: a single public method whose contract reflects the safe NOTHROW behavior. The EX_TRY/EX_CATCH wraps only the throwing CreateHandle call; observable behavior on the OOM fallback path is unchanged. - Drop a stale `similar to UpdateCurrentThrowable()` comment in eedbginterfaceimpl.cpp. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two changes responding to PR review feedback: 1. Remove dead CEEInfo::HandleException. The function has been unreachable since 2016 (commit 4d9f4b8 `Remove SEH interactions between the JIT and the EE'') which replaced the old ICorJitInfo::FilterException/HandleException pair with runWithErrorTrap. The function is private, non-virtual, not part of the ICorJitInfo interface, and has zero callers in coreclr, the JIT, the AOT thunks, or SuperPMI (the SuperPMI Packet_HandleException slot is commented out). Removing it also retires the long-stale comment about `sync between the LTO and the exception tracker'' that pre-dates the ExInfo redesign and the lazy-LTO model from dotnet#127300/dotnet#127649. 2. Reorder declarations in threads.h so SetLastThrownObject precedes SetSOForLastThrownObject, matching the order of the definitions in threads.cpp. Also update an unrelated stale comment in ExInfo::PopExInfos: the `unmanaged thread'' rationale is incorrect because both UMThunkUnwindFrameChainHandler and CallDescrWorkerUnwindFrameChainHandler short-circuit unmanaged threads before reaching PopExInfos, and the function carries a MODE_COOPERATIVE contract. No behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
09de907 to
70188bb
Compare
| { | ||
| LOG((LF_EH, LL_INFO100, "InternalUnhandledExceptionFilter_Worker: Not collecting bucket information as thread object does not exist\n")); | ||
| } | ||
|
|
| @@ -1470,8 +1470,6 @@ class Thread | |||
| } | |||
|
|
|||
|
|
|||
| // LTO may be stale during active dispatch. Readers that need the current | ||
| // exception should call GetThrowable() first and fall back to LastThrownObject() | ||
| // only when GetThrowable() returns NULL. | ||
| OBJECTHANDLE m_LastThrownObjectHandle; // Unsafe to use directly. Use accessors instead. |
There was a problem hiding this comment.
| OBJECTHANDLE m_LastThrownObjectHandle; // Unsafe to use directly. Use accessors instead. | |
| OBJECTHANDLE m_LastThrownObjectHandle; |
| } | ||
| #endif // DEBUGGING_SUPPORTED | ||
|
|
||
| // Set LTO from the exception being destroyed so that post-ExInfo consumers |
There was a problem hiding this comment.
We should do this only when it is actually going to be needed. For example, in a simple try { throw new Exception(); } catch { }, PopExInfos is going to be called from CallCatchFunclets, we create the LTO handle and the very next line in CallCatchFunclets is going to delete the LTO handle.
| pThread->SetLastThrownObject(NULL); | ||
| } | ||
|
|
||
| // Sync managed exception state, for the managed thread, based upon any active exception tracker |
There was a problem hiding this comment.
Is this going to cause a behavior change in things like the windbg !pe command ?
Before this change, !pe issued while the thread is executing a catch block is going to print the exception that's being caught. After this change, I think it is going to print nothing.
There was a problem hiding this comment.
(It would be nice if we can fix this without short-lived GCHandle allocation.)
Note
This PR was authored with the assistance of GitHub Copilot.
Summary
Follow-up to #127300. Stop eagerly synchronizing
Thread::m_LastThrownObjectHandlewithExInfo::m_exceptionduring active exception dispatch. Instead, set LTO lazily when theExInfois destroyed inPopExInfos.After #127300 made
m_exceptionthe GC-tracked exception object during dispatch, the eager LTO sync became redundant cost — every throw and dispatch transition was creating/updating a GCHandle that nothing on the dispatch path actually consumes (consumers readm_exceptiondirectly, or only need LTO once dispatch unwinds).Key Changes
ExInfo::~ExInfo(viaPopExInfos) writesm_exceptionintom_LastThrownObjectHandleexactly once, when the exception leaves the dispatch chain.m_exceptionremains the sole source of truth during dispatch.SafeSetThrowables— the dual-write helper is no longer needed.SafeUpdateLastThrownObject— eager sync sites are gone.SyncManagedExceptionStateand theInternalUnhandledExceptionFilterre-sync block.Cleanup of dead exception-handling helpers
While reviewing this PR, @jkotas pointed out additional dead code that the lazy-LTO change exposed, plus other latent dead code in the area:
SafeSetLastThrownObjectintoSetLastThrownObject— single public method, NOTHROW contract. TheEX_TRY/EX_CATCHnow wraps only the throwingCreateHandlecall; observable behavior on the OOM fallback path is unchanged.Thread::SetLastThrownObjectHandle— zero callers post-PR.SetManagedUnhandledExceptionBitforward declaration — no definition existed anywhere in the codebase.pThrowableInparameter ofNotifyAppDomainsOfUnhandledException.UpdateCurrentThrowable(whose name was misleading: it never updated anything, only returned a boolean) with an inline check at the single call site using the GC-mode-agnosticIsThrowableNull/IsLastThrownObjectNullhelpers.CEEInfo::HandleException— unreachable since 2016 (4d9f4b8"Remove SEH interactions between the JIT and the EE" replaced the oldICorJitInfo::FilterException/HandleExceptionpair withrunWithErrorTrap). The function is private, non-virtual, not part of theICorJitInfointerface, and has zero callers in coreclr, the JIT, the AOT thunks, or SuperPMI. Removing it also retires a long-stale comment about "sync between the LTO and the exception tracker" that pre-dates theExInforedesign.What stays unchanged
m_LastThrownObjectHandleitself remains anOBJECTHANDLE— required by the ICorDebug managed debugging protocol, which reads it cross-process viaBuildFromGCHandle.GetCurrentException/GetThreadExceptionstill prefer the liveExInfo::m_exception(via pseudo-handle) and only fall back to LTO when noExInfois active — same precedence as Remove ExInfo::m_hThrowable - use direct pointer for exception objects #127300.m_ltoIsUnhandledflag is kept — it is still set TRUE fromEEPolicy::HandleFatalError/HandleFatalStackOverflowand consumed by the managed debugger via DAC and cDAC.Testing