Remove helper method frame from Profiler hooks #107152

davidwrighton · 2024-08-29T18:12:48Z

Remove helper method frame and update the CONTRACTs to suit the new behavior. This WILL change the profiler behavior in that a set of apis in the Profiler api surface will not fail immediately on any attempt to use them. However, the set of apis which change are I believe not safe to call within a ProfilerEnter/Leave hook as an actual triggered GC will cause potential memory corruption if the arguments/return value of the function have any GC pointers.

This be done without the tweaks to remove the CodeVersionManager lock, but in general, I'd like to get rid of that lock from most of the entire runtime. I welcome opinions as to whether I should try to hold the CodeVersionManager lock into another change focussed on that.

- Now, you can iterate the lists, and read the various rejit id's safely This enables a few of the apis on the profiler to be safe to call while in a funciton enter/leave hook. From my analysis these functions were previously safe to call and did not assert, but only because of the presence of a HELPER_METHOD_FRAME. Now, they should simply be safe to call and the erroneous failure to assert has been addressed

…hodFrameFromProfilerHooks

…elper method frame - Rework AppDomain::GetFriendlyName logic so that it is always safe for use here.

davidwrighton · 2024-09-09T17:20:39Z

This be done without the tweaks to remove the CodeVersionManager lock, but in general, I'd like to get rid of that lock from most of the entire runtime. I welcome opinions as to whether I should try to hold the CodeVersionManager lock into another change focussed on that.

dotnet-policy-service · 2024-09-09T17:24:18Z

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

dotnet-policy-service · 2024-09-09T17:24:19Z

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

AaronRobinsonMSFT · 2024-09-09T17:46:11Z

src/coreclr/vm/appdomain.cpp

 {
    CONTRACT (LPCWSTR)
    {
-        THROWS;
-        if (GetThreadNULLOk()) {GC_TRIGGERS;} else {DISABLED(GC_NOTRIGGER);}
+        NOTHROW;


This isn't correct. If m_friendlyName isn't in UTF16 format, then GetUnicode() will throw.

SString shouldn't be used as a field type. I would change that to avoid this issue.

- Get rid of GetFriendlyNameForLogging, as its difference from GetFriendlyName is immaterial at the only time its ever called

davmason · 2024-09-09T23:51:30Z

This WILL change the profiler behavior in that a set of apis in the Profiler api surface will not fail immediately on any attempt to use them. However, the set of apis which change are I believe not safe to call within a ProfilerEnter/Leave hook as an actual triggered GC will cause potential memory corruption if the arguments/return value of the function have any GC pointers.

Is "not fail immediately" a typo for "now fail immediately"? As written this worries me, the whole profiler ELT hook concept is not well understood by the community at large, and very poorly documented by us. We shouldn't be introducing new ways for profilers to silenty fail in weird ways.

…hodFrameFromProfilerHooks

noahfalk

A few small suggestions inline, looks like a nice improvement overall. I'm happy to see that lock diminished 👍

noahfalk · 2024-09-09T20:31:10Z

src/coreclr/debug/ee/functioninfo.cpp

+        }
+        else
+        {
+            _ASSERTE(!"Unexpected rejit state, should be active as there exists a native code version for this IL code version");


I suspect there are corner cases where this isn't true. For example a profiler could request to rejit a method, the JIT starts compiling it, and then before the JIT finishes the profiler requests to rejit it again. By the time the JIT invokes this method the IL version is no longer active.

ILCodeVersion goes from state kStateRequested to kStateGettingReJITParameters to kStateActive. We cannot actually run the JIT without the function being active, as we don't actually have any IL. There is no pathway for an ILCodeVersion to become inactive.

It however can be not THE active IL Code version, which is a different concept (which confusingly doesn't require the ILCodeVersion to have reached kStateActive. Honestly, there are 4 concepts of "active" in the code versioning codebase, all of which mean fairly different things, which makes everything here really confusing to read.

The DebuggerJitInfo can only be created after jitting completes, as the constructor for a DebuggerJitInfo requires a pointer to the start of the jitted code.

Ah yes, I forgot that RejitState() == kStateActive was distinct from CodeVersionManager::GetActiveILCodeVersion(). And agreed, the naming collision doesn't make that convenient to discuss. Probably not in this PR, but what you do think about renaming kStateActive to something like kILInitialized or kReJITParametersInitialized? I think that would better capture its meaning and reduce overload on the 'active' term.

noahfalk · 2024-09-09T23:35:35Z

src/coreclr/vm/rejit.cpp

-        CAN_TAKE_LOCK;
-        GC_TRIGGERS;
+        CANNOT_TAKE_LOCK;
+        GC_NOTRIGGER;


The code comment below at 1133-1135 is out of sync with the code now that the lock is removed.

noahfalk · 2024-09-09T23:36:58Z

src/coreclr/vm/rejit.cpp

-
-    // Caller must ensure this lock is taken!
-    _ASSERTE(CodeVersionManager::IsLockOwnedByCurrentThread());
-
    NativeCodeVersion nativeCodeVersion = pMD->GetCodeVersionManager()->GetNativeCodeVersion(pMD, pCodeStart);


Suggested change

NativeCodeVersion nativeCodeVersion = pMD->GetCodeVersionManager()->GetNativeCodeVersion(pMD, pCodeStart);

NativeCodeVersion nativeCodeVersion = pCodeVersionManager->GetNativeCodeVersion(pMD, pCodeStart);

Now that the methods are merged we've already got the manager available.

noahfalk · 2024-09-10T00:04:29Z

src/coreclr/vm/codeversion.h

    //};

-    ReJITID m_parentId;
+    DAC_IGNORE(const) ReJITID m_parentId;
    PTR_NativeCodeVersionNode m_pNextMethodDescSibling;


There is a comment below saying m_pNextILVersionNode is never modified after being added to the list. I'm guessing the same thing holds here and we could add a similar comment?

noahfalk · 2024-09-10T00:08:08Z

I welcome opinions as to whether I should try to hold the CodeVersionManager lock into another change focussed on that.

This change seemed like a good scope as-is. I wouldn't add more lock refactoring to this PR if that is what you were asking about.

davidwrighton · 2024-09-10T00:12:34Z

@davmason, we have logic which attempts to replicate the behavior of our contracts system, but with runtime failures. This is the logic which calls PROFILER_TO_CLR_ENTRYPOINT_SYNC_EX with kP2EETriggers as one of the flags. The behavior will be to return CORPROF_E_UNSUPPORTED_CALL_SEQUENCE. This change makes calls to the following functions throw if used from an profile enter/leave event.

GetAppDomainsContainingModule,
GetILFunctionBodyAllocator,
SetILFunctionBody,
SetILInstrumentedCodeMap,
ForceGC
GetClassFromToken,
GetClassFromTokenAndTypeArgs,
GetFunctionFromTokenAndTypeArgs,
GetAppDomainInfo
RequestReJITWithInliners,
GetReJITIDs,
RequestReJIT,
RequestRevert,
EnumJITedFunctions2,
EnumModules,
EnumNgenModuleMethodsInliningThisMethod,
ApplyMetaData,

This is unfortunate, so I went through the work to make some of the more likely used apis reliably succeed.

However, I can't fix all of them (although I could fix some of them). We also have the problem that we already had an unpredictable failure case, where if we actually hit a GC suspend point during an Enter/Leave hook, we will corrupt memory. This change makes more apis fail, but the ones that work should reliably work unlike the previous model where some stuff worked but only if the customer didn't get happen to get unlucky.

tommcdon

Thank you!

davmason

LGTM, I am a little worried about the implications for the profiler but doing this at the beginning of 10 should help with that

…hodFrameFromProfilerHooks

Remove helper method frame and update the CONTRACTs to suit the new behavior. This WILL change the profiler behavior in that a set of apis in the Profiler api surface will not fail immediately on any attempt to use them. However, the set of apis which change are I believe not safe to call within a ProfilerEnter/Leave hook as an actual triggered GC will cause potential memory corruption if the arguments/return value of the function have any GC pointers.

Remove helper method frame from Profiler hooks

b8bcc52

dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 29, 2024

dotnet-policy-service bot assigned davidwrighton Aug 29, 2024

This was referenced Aug 29, 2024

System.IO.Net5Compat.Tests and System.IO.Tests suddenly exiting with error 137 #100558

Open

SIGKILL (OOM?) while running LibraryImportGenerator.Tests w/o actionable log messages or artifacts dotnet/dnceng#2496

Open

teo-tsirpanis added area-VM-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Sep 1, 2024

davidwrighton added 5 commits September 4, 2024 16:07

Merge branch 'main' of github.com:dotnet/runtime into RemoveHelperMet…

5895693

…hodFrameFromProfilerHooks

Fix compile issues on Unix platforms

25efc4a

Make ProfToEEInterfaceImpl::GetAppDomainInfo safe to call without a h…

8fcd2c1

…elper method frame - Rework AppDomain::GetFriendlyName logic so that it is always safe for use here.

Remove another place where we have problematic contracts

829b1c3

davidwrighton marked this pull request as ready for review September 9, 2024 17:19

davidwrighton requested a review from davmason September 9, 2024 17:23

davidwrighton added the area-Diagnostics-coreclr label Sep 9, 2024

davidwrighton requested review from tommcdon and AaronRobinsonMSFT September 9, 2024 17:23

AaronRobinsonMSFT requested a review from noahfalk September 9, 2024 17:39

AaronRobinsonMSFT reviewed Sep 9, 2024

View reviewed changes

davmason requested a review from mdh1418 September 9, 2024 21:08

Get rid of SString as a field on the AppDomain type for friendly name

15f8da6

- Get rid of GetFriendlyNameForLogging, as its difference from GetFriendlyName is immaterial at the only time its ever called

Merge branch 'main' of github.com:dotnet/runtime into RemoveHelperMet…

95ac809

…hodFrameFromProfilerHooks

noahfalk approved these changes Sep 10, 2024

View reviewed changes

tommcdon approved these changes Sep 10, 2024

View reviewed changes

davmason approved these changes Sep 10, 2024

View reviewed changes

davidwrighton added 2 commits September 10, 2024 14:00

Code review feedback

480f310

Merge branch 'main' of github.com:dotnet/runtime into RemoveHelperMet…

e839096

…hodFrameFromProfilerHooks

davidwrighton merged commit 5e560d9 into dotnet:main Sep 11, 2024
90 checks passed

build-analysis bot mentioned this pull request Sep 11, 2024

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Open

3 tasks

noahfalk mentioned this pull request Sep 12, 2024

Profiler Interest Group - Announcements #9305

Open

github-actions bot locked and limited conversation to collaborators Oct 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove helper method frame from Profiler hooks #107152

Remove helper method frame from Profiler hooks #107152

davidwrighton commented Aug 29, 2024 •

edited

Loading

davidwrighton commented Sep 9, 2024

dotnet-policy-service bot commented Sep 9, 2024

dotnet-policy-service bot commented Sep 9, 2024

AaronRobinsonMSFT Sep 9, 2024

AaronRobinsonMSFT Sep 9, 2024

davidwrighton Sep 9, 2024

davmason commented Sep 9, 2024

noahfalk left a comment

noahfalk Sep 9, 2024

davidwrighton Sep 10, 2024

noahfalk Sep 10, 2024

noahfalk Sep 9, 2024

noahfalk Sep 9, 2024

noahfalk Sep 10, 2024

noahfalk commented Sep 10, 2024

davidwrighton commented Sep 10, 2024

tommcdon left a comment

davmason left a comment

	NativeCodeVersion nativeCodeVersion = pMD->GetCodeVersionManager()->GetNativeCodeVersion(pMD, pCodeStart);
	NativeCodeVersion nativeCodeVersion = pCodeVersionManager->GetNativeCodeVersion(pMD, pCodeStart);

Remove helper method frame from Profiler hooks #107152

Remove helper method frame from Profiler hooks #107152

Conversation

davidwrighton commented Aug 29, 2024 • edited Loading

davidwrighton commented Sep 9, 2024

dotnet-policy-service bot commented Sep 9, 2024

dotnet-policy-service bot commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davmason commented Sep 9, 2024

noahfalk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noahfalk commented Sep 10, 2024

davidwrighton commented Sep 10, 2024

tommcdon left a comment

Choose a reason for hiding this comment

davmason left a comment

Choose a reason for hiding this comment

davidwrighton commented Aug 29, 2024 •

edited

Loading