Jumpstub fixes #15296

jkotas · 2017-11-30T06:36:15Z

This set of changes is fixing #14995 and #14996, and improves reliability of jump stub allocation. The key changes are:

Retry JITing on failure to allocate jump stub. Failure to allocate jump during JITing is not fatal anymore. There is extra memory reserved for jump stubs on retry to ensure that the retry succeeds allocating the jump stubs that it needs with high probability.
Reserve space for jump stubs for precodes and other code fragments at the end of each code heap segment. This is trying to ensure that eventual allocation of jump stubs for precodes and other code fragments succeeds. Accounting is done conservatively - reserves more than strictly required. It wastes a bit of address space, but no actual memory. Also, this reserve is not used to allocate jump stubs for JITed code since the JITing can recover from failure to allocate the jump stub now.

My plan is to port these changes to .NET Framework as well since these issues affect important workloads there.

jkotas · 2017-11-30T06:43:08Z

@dotnet-bot test Windows_NT x64 corefx_baseline
@dotnet-bot test Ubuntu x64 corefx_baseline

BruceForstall · 2017-11-30T20:33:01Z

Does it make sense to also update https://github.com/dotnet/coreclr/blob/master/Documentation/design-docs/jump-stubs.md?

kouvel · 2017-11-30T19:03:30Z

src/vm/amd64/cgenamd64.cpp

@@ -721,11 +722,26 @@ INT32 rel32UsingJumpStub(INT32 UNALIGNED * pRel32, PCODE target, MethodDesc *pMe
        TADDR hiAddr = baseAddr + INT32_MAX;
        if (hiAddr < baseAddr) hiAddr = UINT64_MAX; // overflow

+        // Always try to allocate with throwOnOutOfMemoryWithinRange=false first to conserve
+        // reserved space untill when it is really needed


Typo: untill

What is the benefit of trying with 'false' first here? It seems like the next try with true would do the same thing but also trying to use the emergency reserve for jump stubs and if that fails, it would throw anyway as before. I probably missed something.

There was a missing piece of logic to conserve reserved space in CanUseCodeHeap. Fixed and added the needed comments to explain - @vitek-karas was confused by this as well.

I see, makes sense now, thanks!

vitek-karas · 2017-11-30T16:46:56Z

src/inc/clrconfigvalues.h

@@ -599,7 +599,7 @@ RETAIL_CONFIG_STRING_INFO(INTERNAL_WinMDPath, W("WinMDPath"), "Path for Windows
 // Loader heap
 // 
 CONFIG_DWORD_INFO_EX(INTERNAL_LoaderHeapCallTracing, W("LoaderHeapCallTracing"), 0, "Loader heap troubleshooting", CLRConfig::REGUTIL_default)
-RETAIL_CONFIG_DWORD_INFO(INTERNAL_CodeHeapReserveForJumpStubs, W("CodeHeapReserveForJumpStubs"), 2, "Percentage of code heap to reserve for jump stubs")
+RETAIL_CONFIG_DWORD_INFO(INTERNAL_CodeHeapReserveForJumpStubs, W("CodeHeapReserveForJumpStubs"), 1, "Percentage of code heap to reserve for jump stubs")


I assume we reduce this because JIT required jump stubs are not covered by this number anymore... so we don't need so much space anymore... right?

vitek-karas · 2017-11-30T16:55:50Z

src/vm/amd64/cgenamd64.cpp

@@ -721,11 +722,26 @@ INT32 rel32UsingJumpStub(INT32 UNALIGNED * pRel32, PCODE target, MethodDesc *pMe
        TADDR hiAddr = baseAddr + INT32_MAX;
        if (hiAddr < baseAddr) hiAddr = UINT64_MAX; // overflow

+        // Always try to allocate with throwOnOutOfMemoryWithinRange=false first to conserve
+        // reserved space untill when it is really needed


Nit: "until" (single l)

vitek-karas · 2017-11-30T17:13:24Z

src/vm/amd64/cgenamd64.cpp

@@ -721,11 +722,26 @@ INT32 rel32UsingJumpStub(INT32 UNALIGNED * pRel32, PCODE target, MethodDesc *pMe
        TADDR hiAddr = baseAddr + INT32_MAX;
        if (hiAddr < baseAddr) hiAddr = UINT64_MAX; // overflow

+        // Always try to allocate with throwOnOutOfMemoryWithinRange=false first to conserve
+        // reserved space untill when it is really needed


I would expand the comment to make it very explicit that we're talking about conserving JumpStubReserve (not a reserved VM space, which is what it sounds like to me now).

vitek-karas · 2017-11-30T17:21:54Z

src/vm/jitinterface.cpp

+                    delta = rel32UsingJumpStub(fixupLocation, (PCODE)target, m_pMethodBeingCompiled, NULL, false /* throwOnOutOfMemoryWithinRange */);
+                    if (delta == 0)
+                    {
+                        m_fRel32Overflow = TRUE;


I think this is worth a comment of what the effect of this will be. Something like "this forces the JIT to retry the method, which allows us to reserve more space for jump stubs and have a higher chance that we will find space for them". The "unintended" side-effect of this is also that we will recompile the method with "long-pointers" for all data accesses, even though it might not have been necessary otherwise. Worth a note, but I think it's OK to keep it that way - if we're already struggling with jump stubs, then making the method work at all is a victory... small perf gains like rel32 data accesses are fine to ignore.

"unintended" side-effect of this is also that we will recompile the method with "long-pointers" for all data accesses

BTW: By the time we got here we are recompiling the method with "long-pointers" already. (It is the problem 1. described in https://github.com/dotnet/coreclr/blob/master/Documentation/design-docs/jump-stubs.md#jump-stubs-and-the-jit .)

vitek-karas · 2017-11-30T17:51:27Z

src/vm/codeman.cpp

@@ -2101,11 +2123,18 @@ HeapList* LoaderCodeHeap::CreateCodeHeap(CodeHeapRequestInfo *pInfo, LoaderHeap
    {
        if (loAddr != NULL || hiAddr != NULL)
        {
+#ifdef _DEBUG
+            // Always exercise the fallback path with force relocs


Nit: I would word this "Always exercise the fallback path in the caller when forced relocs are turned on. (Took me a couple of minutes to realize what you mean by this).

vitek-karas · 2017-11-30T18:20:04Z

src/vm/codeman.cpp

@@ -2675,49 +2664,6 @@ EEJitManager::DomainCodeHeapList *EEJitManager::GetCodeHeapList(CodeHeapRequestI
    return pList;
 }

-HeapList* EEJitManager::GetCodeHeap(CodeHeapRequestInfo *pInfo)


Please remove mention of this method from the comment in codeman.cpp:565

vitek-karas · 2017-11-30T18:32:03Z

src/vm/codeman.cpp

-                    // pCurrent is the first (and possibly only) heap that would satistfy
-                    pResult = pCurrent;
-                }
-                // We use the initial creation size as a discriminator (i.e largest heap)


If I read it correctly - your change removed this behavior - that is we don't prefer the largest heap anymore, we simply take the first one available...
Is that intentional? There doesn't seem to be any reasoning mentioned in the code as to why we tried to get the largest heap before... so I don't know if removing that functionality is OK or not.

Right. The code to prefer largest heap was added on 2004/07/02 by @briansull . Brian, do you happen to remember why it was done?

No I don't recall why.
Probably just so we start using the new largest heap for the new JITted code, leaving the older mostly used up heaps around in case we needed them for jumpstubs.
Back then we didn't have all of the code to reserve the extra space for jump stubs.

vitek-karas · 2017-11-30T23:42:49Z

src/vm/dynamicmethod.cpp

+#endif
+
+    size_t nibbleMapSize = HEAP2MAPSIZE(ROUND_UP_TO_PAGE(pHp->maxCodeHeapSize));
+    pHp->pHdrMap = new DWORD[nibbleMapSize / sizeof(DWORD)];


Is it OK to just new up the nibble map memory here?
I totally agree that we should not allocate the nibble map from the code heap itself (as that is executable memory, so we should not use it for data), I'm just wondering if we should allocate the nibble map from some of the other heaps (low-frequency heap maybe...)?

I do not see a problem with it.

We need to be able to free this dynamic method heaps independently. Allocating on LoaderHeaps (like low-frequency heap) would not allow us to do that.

vitek-karas · 2017-12-01T00:09:48Z

src/vm/dynamicmethod.cpp

        }
        else
        {
            LOG((LF_BCL, LL_INFO100, "Level2 - CodeHeap [0x%p] - allocation failed:\n\tm_pLastAvailableCommittedAddr: 0x%X\n\tsizeToCommit: 0x%X\n\tm_pBaseAddr: 0x%X\n\tm_TotalBytesAvailable: 0x%X\n", this, m_pLastAvailableCommittedAddr, sizeToCommit, m_pBaseAddr, m_TotalBytesAvailable));
-            return NULL;
+            // Update largest availble block size


availble
Spelling "available"

jkotas · 2017-12-01T00:16:51Z

Does it make sense to also update https://github.com/dotnet/coreclr/blob/master/Documentation/design-docs/jump-stubs.md?

Updated to match the current state.

jkotas · 2017-12-01T04:55:44Z

@vitek-karas Thanks for the review - all feedback addressed.

- Reserve space for jump stubs for precodes and other code fragments at the end of each code heap segment. This is trying to ensure that eventual allocation of jump stubs for precodes and other code fragments succeeds. Accounting is done conservatively - reserves more than strictly required. It wastes a bit of address space, but no actual memory. Also, this reserve is not used to allocate jump stubs for JITed code since the JITing can recover from failure to allocate the jump stub now. Fixes #14996. - Improve algorithm to reuse HostCodeHeap segments: Maintain estimated size of the largest free block in HostCodeHeap. This estimate is updated when allocation request fails, and also when memory is returned to the HostCodeHeap. Fixes #14995. - Retry JITing on failure to allocate jump stub. Failure to allocate jump during JITing is not fatal anymore. There is extra memory reserved for jump stubs on retry to ensure that the retry succeeds allocating the jump stubs that it needs with high probability. - Respect CodeHeapRequestInfo::getRequestSize for HostCodeHeap. CodeHeapRequestInfo::getRequestSize is used to throttle code heap segment size for large workloads. Not respecting it in HostCodeHeap lead to too many too small code heap segments in large workloads. - Switch HostCodeHeap nibble map to be allocated on regular heap as part. It simplied the math required to estimate the nibble map size, and allocating on regular heap is overall goodness since it does not need to be executable.

jkotas requested review from briansull, kouvel and vitek-karas November 30, 2017 06:36

kouvel approved these changes Nov 30, 2017

View reviewed changes

jkotas force-pushed the jumpstub-fixes branch 2 times, most recently from 9abbc29 to 02b9e3c Compare December 1, 2017 00:16

vitek-karas approved these changes Dec 1, 2017

View reviewed changes

jkotas force-pushed the jumpstub-fixes branch from 1e8074e to 5d0a5a8 Compare December 1, 2017 05:03

jkotas merged commit c1e44d9 into dotnet:master Dec 1, 2017

jkotas deleted the jumpstub-fixes branch December 1, 2017 07:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jumpstub fixes #15296

Jumpstub fixes #15296

jkotas commented Nov 30, 2017 •

edited

Loading

jkotas commented Nov 30, 2017

BruceForstall commented Nov 30, 2017

kouvel Nov 30, 2017

jkotas Nov 30, 2017

kouvel Nov 30, 2017

vitek-karas Nov 30, 2017

jkotas Dec 1, 2017

vitek-karas Nov 30, 2017

vitek-karas Nov 30, 2017

vitek-karas Nov 30, 2017

jkotas Dec 1, 2017 •

edited

Loading

vitek-karas Nov 30, 2017

vitek-karas Nov 30, 2017

vitek-karas Nov 30, 2017

jkotas Dec 1, 2017

briansull Dec 1, 2017 •

edited

Loading

vitek-karas Nov 30, 2017

jkotas Dec 1, 2017

vitek-karas Dec 1, 2017

jkotas commented Dec 1, 2017

jkotas commented Dec 1, 2017

Jumpstub fixes #15296

Jumpstub fixes #15296

Conversation

jkotas commented Nov 30, 2017 • edited Loading

jkotas commented Nov 30, 2017

BruceForstall commented Nov 30, 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

jkotas Dec 1, 2017 • edited Loading

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

briansull Dec 1, 2017 • edited Loading

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

jkotas commented Dec 1, 2017

jkotas commented Dec 1, 2017

jkotas commented Nov 30, 2017 •

edited

Loading

jkotas Dec 1, 2017 •

edited

Loading

briansull Dec 1, 2017 •

edited

Loading