Rejit support for R2R - WIP #9298

noahfalk · 2017-02-03T03:11:46Z

Attempting to use Rejit with R2R rapidly hits two issues:
a) The prestub never communicates to the RejitManager that new code from an R2R image is about to be used. This causes pending requests to rejit a method never
to be honored. Fixed in prestub.cpp

b) ICorProfilerInfo6::EnumNgenMethodsInliningThisMethod didn't work with R2R images because R2R images did not carry the inline tracking table. The majority of the changes address this part. Currently only intra-module inlining is supported. Cross module inlining via NonVersionableAttribute won't be tracked by design.

This change isn't ready to be checked in yet, but it does demonstrate the proposed file format, the layout of the code, and works well enough to pass really trivial functional tests on windows. Most of my planned work at this point is going to be subjecting it to broader testing (still on windows), fixing issues, and responding to PR feedback. Linux is on the roadmap and may work as-is, but verifying that is not part of this PR. I'm going to look into putting a better guard in place so that anyone adding cross module inlining in the future will realize the implications for the profiler. I wasn't planning to do perf investigation for size on disk as this format should be strictly smaller than NGEN's and we already accepted the perf overhead in that case.

noahfalk · 2017-02-03T03:13:48Z

@jkotas @fadimounir - PTAL

@gkhanna79 - Gaurav are there additional people on your team that should be included on this PR?

Thanks!

fadimounir · 2017-02-03T18:36:08Z

The prestub never communicates to the RejitManager that new code from an R2R image is about to be used. This causes pending requests to rejit a method never
to be honored

@noahfalk could you please give me some context on what this rejit feature is? What do we rejit and why? Is it because we can't use AOT code from the R2R image for some reason? etc...

gkhanna79 · 2017-02-03T18:43:58Z

CC @rahku @jkotas

fadimounir · 2017-02-03T18:48:04Z

src/inc/readytorun.h

@@ -57,6 +57,7 @@ enum ReadyToRunSectionType
    // 107 used by an older format of READYTORUN_SECTION_AVAILABLE_TYPES
    READYTORUN_SECTION_AVAILABLE_TYPES              = 108,
    READYTORUN_SECTION_INSTANCE_METHOD_ENTRYPOINTS  = 109,
+    READYTORUN_SECTION_INLINING_INFO                = 110


READYTORUN_SECTION_INLINING_INFO [](start = 4, length = 32)

@jkotas Does this need a change the the R2R minor version?

Yes, I think it would be a good idea to bump the R2R minor version for this.

fadimounir · 2017-02-03T18:50:07Z

src/vm/ceeload.cpp

 {
    LIMITED_METHOD_DAC_CONTRACT;
-    return m_persistentInlineTrackingMap;
+#ifdef FEATURE_READYTORUN
+    if(IsReadyToRun() && GetReadyToRunInfo()->GetInlineTrackingMap() != NULL) return TRUE;


return TRUE; [](start = 78, length = 12)

Nit: new line :)

fadimounir · 2017-02-03T18:55:43Z

You should have a test (at least a manual one before merging the PR) where you crossgen an assembly to produce a R2R using an old crossgen without any of your changes, and see how this loads with your runtime changes.
And vice versa, you should ensure that new R2R images with your changes either work with an older runtime, or get rejected by it (but not crash it)

fadimounir · 2017-02-03T19:06:50Z

src/vm/inlinetracking.h

+
+// This type know how to serialize and deserialize the inline tracking map format within an R2R image. See
+// above for a description of the format.
+class PersistentInlineTrackingMapR2R


PersistentInlineTrackingMapR2R [](start = 6, length = 30)

Maybe this should be under the FEATURE_READYTORUN?

fadimounir · 2017-02-03T19:12:38Z

src/vm/readytoruninfo.h

@@ -40,6 +41,9 @@ class ReadyToRunInfo
    Crst                            m_Crst;
    PtrHashMap                      m_entryPointToMethodDescMap;

+    PersistentInlineTrackingMapR2R  m_persistentInlineTrackingMap;
+    PTR_PersistentInlineTrackingMapR2R m_pPersistentInlineTrackingMap;


I'm personally not a big fan of this... Maybe you should make the .TryLoad function a static one that returns a pointer to a heap allocated map. Feels a bit like a hack to me

Mostly I just did it because the machinery to make new allocations looks like it requires some additional arguments to be passed around and the map datastructure is only a few pointers in size, however if I am breaking form I can certainly do a seperate allocation.

fadimounir · 2017-02-03T19:25:02Z

src/vm/inlinetracking.cpp


-#ifndef DACCESS_COMPILE
-COUNT_T PersistentInlineTrackingMap::GetInliners(PTR_Module inlineeOwnerMod, mdMethodDef inlineeTkn, COUNT_T inlinersSize, MethodInModule inliners[], BOOL *incompleteData)
+    // Going through last 10 inliners to check if a given inliner has recently been registered.


10 [](start = 26, length = 2)

What is this number based on? just curious :). Any data?

This number didn't actually come from me, I was simply rearranging the file a bit so that methods on different types wouldn't be intermingled. The original author was Eugene and I think he came up with this number and several others empirically based on some ad-hoc experiements/guessing reasonable values.

fadimounir · 2017-02-03T19:28:06Z

src/vm/inlinetracking.cpp

+    int SizeOfInlineeIndex;
+};
+
+#ifdef FEATURE_NATIVE_IMAGE_GENERATION


FEATURE_NATIVE_IMAGE_GENERATION [](start = 7, length = 31)

shouldn't we also have FEATURE_READYTORUN here?

fadimounir · 2017-02-03T19:28:39Z

src/vm/inlinetracking.cpp

+    entry->SortAndDeduplicate();
+    MethodInModule inlinee = entry->m_inlinee;
+    InlineSArray<MethodInModule, 3> &inliners = entry->m_inliners;
+    COUNT_T tatalInlinersCount = inliners.GetCount();


tatalInlinersCount [](start = 12, length = 18)

typo

fadimounir · 2017-02-03T19:30:28Z

InlineTrackingEntry **inlinees = new InlineTrackingEntry *[runtimeMapCount];

We should probably check for null allocations (out of memory cases)

Refers to: src/vm/inlinetracking.cpp:382 in c761c11. [](commit_id = c761c11, deletion_comment = False)

fadimounir · 2017-02-03T19:33:33Z

src/vm/inlinetracking.cpp

+    // Sort records from runtimeMap, because we need to make sure 
+    // we save everything in deterministic order. Hashtable iteration is not deterministic.
+    COUNT_T runtimeMapCount = runtimeMap->GetCount();
+    InlineTrackingEntry **inlinees = new InlineTrackingEntry *[runtimeMapCount];


new InlineTrackingEntry *[runtimeMapCount]; [](start = 37, length = 43)

check for null allocation (OOM)

Will do (though if it alarms anyone this is a pre-existing bug in the NGEN variant of the code that got replicated here)

Looking around at numerous other allocation sites in NGEN/R2R paths I am not seeing any that do NULL checks. It appears the common pattern is "new (image->GetHeap()) InlineTrackingEntry *[runtimeMapCount]" that will throw an out of memory exception automatically on failure to allocate.

fadimounir · 2017-02-03T19:37:42Z

src/vm/inlinetracking.cpp

+
+    InliningHeader header;
+    header.MajorVersion = 1;
+    header.MinorVersion = 0;


It would be nice to have these versions declared in #define directives so that updating them in the future would be easier (including reading them in TryLoad)

fadimounir · 2017-02-03T19:39:57Z

src/vm/inlinetracking.cpp

+    inlineeIndex->Append(record);
+}
+
+void PersistentInlineTrackingMapR2R::Save(SBuffer* pSaveTarget, InlineTrackingMap* runtimeMap)


Save [](start = 37, length = 4)

Nit: Ideally, we should share the pre-saving logic between this R2R and the NGEN persistent inline tracking map, and just have the Save method do the acutal writing (the pre-saving logic is the same in both case)

Sure, I'll confirm that I can merge a bit more than we've got so far.

fadimounir · 2017-02-03T19:44:50Z

src/vm/inlinetracking.cpp

+BOOL PersistentInlineTrackingMapR2R::TryLoad(Module* pModule, const BYTE* pBuffer, DWORD cbBuffer)
+{
+    InliningHeader* pHeader = (InliningHeader*)pBuffer;
+    if(pHeader->MajorVersion != 1)


MajorVersion [](start = 16, length = 12)

Have you considered using the R2R major and minor versions instead of declaring new ones? (also seems like MinorVersion is not used)

Is there a document or comments somewhere that describe the r2r versioning model? I'm not at all attached to these version checks if I can piggy back on a broader versioning boundary.

@gkhanna79 or @jkotas could you please point Noah to the r2r design doc? I can't remember where it was

I made some changes for versioning already guessing that we are using the standard major = breaking, minor = non-breaking and added comments to that effect. If that all looks good then I don't need the doc, but if I guessed incorrectly then yes the doc could be very useful.

fadimounir · 2017-02-03T19:49:13Z

The general direction looks good to me.

Do you think it would be better to have the 2 implementations for NGEN and R2R separated by #ifdefs and avoid having the NGEN and R2R suffixes to class names?

noahfalk · 2017-02-03T22:38:02Z

Historical background on rejit:

Rejit is a profiler feature that we introduced in .Net 4.0 (or 4.5 I can't recall?). The intention is to allow a profiler writer to change the implementation of a method at any time during the execution of a process. The profiler does this by indicating which Modules/MethodTokens need to be updated in ICorProfilerInfo4::RequestRejit, then providing a new IL body in IProfilerControl::SetILFunctionBody. The IL is usually a close derivative of the previous IL, but it doesn't have to be. The runtime takes this IL, uses the JIT to compile it and places a jump instruction on the beginning of the old method body to send execution to the new method body. A simple scenario where this might be useful - a developer is running an app in production and they want to log the value of a local variable in the middle of a function without rebuilding and redeploying a new application. Using rejit the developer could ask the profiler to change the implementation so the IL includes a call to a logging statement that was not there before.

In 4.5.1 (I think?) we added support for rejit in NGEN images, previously it was only supported for runtime jitted code. Part of that additional work was exposing the inlining information. This was required because if you wanted to change the implementation of method A, and the IL of method A has already been inlined inside method B, then you need to recompile B in order to get the new behavior.

When R2R was created nobody hooked it up to the rejit mechanism in the same way that NGEN was, so scenario-wise profilers lost the ability to change a bunch of framework code when moving from desktop to .Net Core. This feature aims to restore that ability.

noahfalk · 2017-02-03T22:40:16Z

Re: testing comment above - yes I plan to do both the forward and back-compat tests (new image/old runtime) + (old image/new runtime)

noahfalk · 2017-02-03T22:59:28Z

Do you think it would be better to have the 2 implementations for NGEN and R2R separated by #ifdefs and avoid having the NGEN and R2R suffixes to class names?

I don't think we can. We need to support both the mscorlib image which is NGEN'ed and the other framework images which are R2R using the same build of coreclr.

noahfalk · 2017-02-07T05:05:30Z

@fadimounir - I've taken a stab at addressing all the feedback, but keep it coming as needed. I still have a few small changes of my own planned and testing is continuing. Thanks!

mjsabby · 2017-02-10T07:36:39Z

src/inc/corprof.idl

    *
    * It can be used to lift limitation on inlining for ReJIT.
+    *
+    * NOTE: If the inlinee method is decorated with the System.Runtime.Versioning.NonVersionable attribute then


I would more precisely call out that either NonVersionable or the profiler author explicitly disables all native images.

hmm, not sure I understand your suggestion? Are you suggesting adding a comment like this: "Inlining results for methods marked NonVersionable may not be complete. If you need to get an full accounting you may avoid the issue by disabling the use of all native images"

Correct, since I'm assuming this doc will go on MSDN.

mattwarren · 2017-02-10T10:02:27Z

Rejit is a profiler feature that we introduced in .Net 4.0 (or 4.5 I can't recall?).

According to David Broman's blog ReJIT was added in .NET 4.5

noahfalk · 2017-02-11T02:10:03Z

Unless I hear otherwise by Monday afternoon I'm going to assume this PR is in good shape. I'll squash and merge at that time. Thanks all!

Two changes: a) R2R code wasn't being reported to the Rejit Manager when it was used, this is a simple fix in prestub.cpp. This makes the ReJit API work. b) The bulk of the changes handle adding support for an inlining table to R2R so that ICorProfilerInfo6::EnumNgenMethodsInliningThisMethod can supply that information to profilers. This was only tested on Windows thus far, but there is no apparent reason this change would be OS specific.

fadimounir · 2017-02-14T20:28:39Z

LGTM

Rejit support for R2R Commit migrated from dotnet/coreclr@3cd73fb

dnfclas added the cla-already-signed label Feb 3, 2017

fadimounir reviewed Feb 3, 2017

View reviewed changes

mjsabby reviewed Feb 10, 2017

View reviewed changes

noahfalk force-pushed the r2r branch from ca52450 to 0921ee6 Compare February 13, 2017 22:33

noahfalk force-pushed the r2r branch from 0921ee6 to ac40eab Compare February 13, 2017 22:42

noahfalk merged commit 3cd73fb into dotnet:master Feb 14, 2017

karelz modified the milestone: 2.0.0 Aug 28, 2017

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/coreclr#9298 from noahfalk/r2r

bb1cd0f

Rejit support for R2R Commit migrated from dotnet/coreclr@3cd73fb

Rejit support for R2R - WIP #9298

Rejit support for R2R - WIP #9298

Conversation

noahfalk commented Feb 3, 2017

noahfalk commented Feb 3, 2017

fadimounir commented Feb 3, 2017

gkhanna79 commented Feb 3, 2017

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

fadimounir commented Feb 3, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fadimounir commented Feb 3, 2017

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

Choose a reason for hiding this comment

noahfalk Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fadimounir commented Feb 3, 2017

noahfalk commented Feb 3, 2017

noahfalk commented Feb 3, 2017

noahfalk commented Feb 3, 2017

noahfalk commented Feb 7, 2017

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

mattwarren commented Feb 10, 2017

noahfalk commented Feb 11, 2017

fadimounir commented Feb 14, 2017

noahfalk Feb 3, 2017 •

edited

Loading