JIT: Remove empty try regions #8949

AndyAyersMS · 2017-01-13T20:34:59Z

In runtimes that do not support thread abort, a try-finally with
an empty try can be optimized to simply the content of the finally.

See documentation update for more details.

Closes #8924.

AndyAyersMS · 2017-01-13T20:37:11Z

Will hold off on merge pending @kouvel's fix noted in #8924.

@Brucefo PTAL; cc @dotnet/jit-contrib

Impacts 130 methods in the jit-diff assembly set:

Total bytes of diff: -13430 (-0.10 % of base)
    diff is an improvement.
Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes
Top file improvements by size (bytes):
       -2916 : System.Linq.Parallel.dasm (-0.46 % of base)
       -2796 : System.Private.CoreLib.dasm (-0.08 % of base)
       -1750 : Microsoft.CodeAnalysis.CSharp.dasm (-0.08 % of base)
       -1326 : System.Linq.dasm (-0.92 % of base)
       -1271 : Microsoft.CodeAnalysis.dasm (-0.17 % of base)
16 total files with size differences (16 improved, 0 regressed).
Top method improvements by size (bytes):
        -353 : System.Security.Cryptography.Cng.dasm - Microsoft.Win32.SafeHandles.SafeNCryptHandle:DuplicateOwnerHandle():ref:this
        -276 : System.Security.Principal.Windows.dasm - <get_Claims>d__83:System.IDisposable.Dispose():this
        -244 : System.Private.CoreLib.dasm - ResourceHelper:GetResourceStringCode(ref):this
        -225 : Microsoft.CSharp.dasm - <AllPossibleInterfaces>d__4:System.IDisposable.Dispose():this
        -221 : Microsoft.CodeAnalysis.CSharp.dasm - <Microsoft-Cci-ITypeDefinition-GetFields>d__32:System.IDisposable.Dispose():this
130 total methods with size differences (130 improved, 0 regressed).

mikedn · 2017-01-13T20:41:17Z

You probably wanted @BruceForstall

AndyAyersMS · 2017-01-13T20:47:32Z

Yeah, thanks.

mikedn · 2017-01-13T21:03:48Z

Documentation/design-docs/finally-optimizations.md

+because this try cannot have multple leaves and its handler cannot be
+reached by nested exit paths.
+
+When the empty try is identified, the jit modifies the callaways pair


callaways -> callalways?

AndyAyersMS · 2017-01-13T21:31:05Z

Odd-looking ubuntu failure:

13:11:50 Directory specified by --coreNativeFxBinDir does not exist: /mnt/j/workspace/dotnet_coreclr/master/checked_ubuntu_tst_prtest/bin/Linux.x64.Release/Native

Hopefully not an indicator that ongoing CoreFX changes are leaking through.... Will retry.

@dotnet-bot retest Ubuntu x64 Checked Build and Test

AndyAyersMS · 2017-01-13T22:17:39Z

Retesting ubuntu now that #8938 is in...

@dotnet-bot retest Ubuntu x64 Checked Build and Test

AndyAyersMS · 2017-01-13T22:37:20Z

OSX seems similarly afflicted; will let it fail before I retry.

AndyAyersMS · 2017-01-13T23:19:49Z

@dotnet-bot retest OSX x64 Checked Build and Test

BruceForstall · 2017-01-18T00:12:18Z

Documentation/design-docs/finally-optimizations.md

+
+Empty trys with non-empty finallys often exist in code that must run
+under both thread-abort aware and non-thread-abort aware runtimes. In
+the former case the placement of cleanup code in thea finally ensures


thea [](start = 49, length = 4)

typo: "thea"

BruceForstall · 2017-01-18T00:16:33Z

Documentation/design-docs/finally-optimizations.md

+
+* callfinally thunks (all but x86): the try must be a single empty
+basic block that always jumps to a callfinally that is the first
+half of a callalways pair;


callalways [](start = 10, length = 10)

can you write out "callallways pair" as "callfinally/always pair"? it's easier to see it's actually a "pair" in that case, and it matches the JIT BBJ_* names more closely.

Yep, will fix.

BruceForstall · 2017-01-18T00:20:14Z

Documentation/design-docs/finally-optimizations.md

+branch directly to the continuation (the branch target of the second
+half of the callalways pair), updates various status flags on the
+blocks, and then removes the try-finally region.
+


In the "callfinally thunks" case, you'll have always->callfinally, converted to always->always. Can you just get rid of the thunk entirely and redirect this thunk always?

Yes, you certainly could do that.

I've been on the fence with these optimizations deciding how much cleanup they should do. On the one hand it seems entirely reasonable to let the subsequent flow opts clean things up, and leaving redundant and/or unreachable blocks around doesn't seem to cause trouble or inhibit other opts. On the other it does not take to much extra work to fix things here.

In the callfinally thunk case the only reference to the callfinally will be in the try, and we already know that the try is a single empty jump-always block. So it would be easy enough to retarget that instead.

That still leaves dead blocks around, and flow opts will almost certainly remove the try block itself later on...

BruceForstall · 2017-01-18T00:20:29Z

Documentation/design-docs/finally-optimizations.md

+EH implementation models, so the try screening has two cases:
+
+* callfinally thunks (all but x86): the try must be a single empty
+basic block that always jumps to a callfinally that is the first


ARM32 doesn't use thunks.

Will update to note x64/arm64 use callfinally thunks, x86/arm32 do not.

BruceForstall · 2017-01-18T00:21:48Z

src/jit/compphases.h

@@ -100,6 +100,7 @@ CompPhaseNameMacro(PHASE_CLR_API,                "CLR API calls",
 #endif

 CompPhaseNameMacro(PHASE_EMPTY_FINALLY,          "Remove empty finally",           "EMPTYFIN",  false, -1)
+CompPhaseNameMacro(PHASE_EMPTY_TRY,              "Remove empty try",               "EMPTYTRY",  false, -1)
 CompPhaseNameMacro(PHASE_CLONE_FINALLY,          "Clone finally",                  "CLONEFIN",  false, -1)


nit: since you made empty try removal execute first, maybe you can list it first here, too. (and in the function declarations)

BruceForstall · 2017-01-18T00:23:05Z

src/jit/flowgraph.cpp

+    // Code in a finally gets special treatment in the presence of
+    // thread abort.
+    bool enableRemoveEmptyTry = false;
+#endif // FEATURE_CORECLR


Maybe you could add a debug way to force enable this using a COMPLUS variable, even on desktop?

Sure, will add the same sort of override as we have for finally cloning.

BruceForstall · 2017-01-18T00:24:56Z

src/jit/flowgraph.cpp

+        if (verbose)
+        {
+            printf("\n*************** After fgRemoveEmptyFinally()\n");
+            fgDispBasicBlocks();


copy-paste error: rename func

BruceForstall · 2017-01-18T00:25:28Z

src/jit/flowgraph.cpp

+
+        // (6) Remove the try-finally EH region. This will compact the
+        // EH table so XTnum now points at the next entry and will update
+        // the EH regon indices of any nested EH in the (former) handler.


regon [](start = 18, length = 5)

typo: regon

BruceForstall · 2017-01-18T00:27:31Z

src/jit/flowgraph.cpp

+        if (!HBtab->HasFinallyHandler())
+        {
+            JITDUMP("EH#%u is not a try-finally; skipping.\n", XTnum);
+            XTnum++;


Seems like this output is going to be a bit verbose in the normal case. Well, not like LSRA verbose...

There aren't that many EH regions... but I can tone it down if you prefer.

BruceForstall · 2017-01-18T00:34:12Z

src/jit/flowgraph.cpp

+            assert(verifiedSingleCallfinally);
+            continue;
+        }
+


Should this whole section be under #ifdef DEBUG? Or are you worried there might actually be such cases that you haven't considered, so this is just defense-in-depth, and you'll wait to see if any asserts come in?

It's not easy for me to test all the EH model variants locally, so this was just extra paranoia. Will probably leave it since it is pretty cheap.

BruceForstall · 2017-01-18T00:41:47Z

@AndyAyersMS Mostly minor comments, overall looks very good.

AndyAyersMS · 2017-01-18T03:11:57Z

@BruceForstall think I got to most of your feedback with the updates.

Rebased to pick up the fix in #8952 and pulled that bit out as a helper method since it is now needed by 3 finally opts.

BruceForstall · 2017-01-18T20:01:56Z

Documentation/design-docs/finally-optimizations.md

+
+The screening then verifies that the callfinally identified above is
+the only callfinally for the try. No other callfinallys are expected
+because this try cannot have multple leaves and its handler cannot be


multple [](start = 29, length = 7)

typo: multple

BruceForstall · 2017-01-18T20:02:12Z

Documentation/design-docs/finally-optimizations.md

+reached by nested exit paths.
+
+When the empty try is identified, the jit modifies the
+callfinally/aways pair to branch to the handler, modifies the


aways [](start = 12, length = 5)

typo: aways

BruceForstall · 2017-01-18T20:03:00Z

Documentation/design-docs/finally-optimizations.md

+
+When the empty try is identified, the jit modifies the
+callfinally/aways pair to branch to the handler, modifies the
+handler's return points to branch directly to the continuation (the


Is the word "points" unnecessary? Seems confusing.

AndyAyersMS · 2017-01-24T04:31:31Z

Will need to be reconciled with #9065.

davghouse · 2017-01-24T18:42:08Z

This PR is creating a big buzz at the office. We're all just now learning about the concept of .NET Core and runtimes and so on, so we're extremely ignorant. Could someone explain the circumstances under which 'FEATURE_CORECLR' (referenced in src/jit/flowgraph.cpp) is false, and why code in the CoreCLR repo needs to worry about the 'FEATURE_CORECLR' being false? It really confused us when we saw that line because it seems contradictory. And also when we saw 'If the JIT is used on runtimes that do support thread abort, I guess it shouldn't do it there...'. So there are more runtimes at play than just CoreCLR? Can someone explain what they are? Thanks 😄

mikedn · 2017-01-24T18:54:18Z

@davghouse There's CoreCLR (aka .NET Core) and there's .NET Framework, the Windows only version of .NET from which .NET Core was created. The runtime code is more or less shared between .NET Core and .NET Framework so in some cases care needs to be taken to avoid breaking .NET Framework.

AndyAyersMS · 2017-01-24T23:13:12Z

@davghouse We try to ensure that JIT changes in particular can flow back to the JIT used in desktop CLR, where thread abort is supported.

AndyAyersMS · 2017-01-24T23:45:51Z

Rebased and updated past merge conflict in compphases.h, and squashed to one commit.

AndyAyersMS · 2017-01-25T01:40:42Z

Looks like test runner script errors on OSX & Ubuntu. Maybe #9092 has fixed this, will retry.

17:07:24 Unknown switch: --coreFxNativeBinDir=/Users/dotnet-bot/j/workspace/dotnet_coreclr/master/checked_osx_tst_prtest/bin/OSX.x64.Release

Logs (here) and (here)

@dotnet-bot retest OSX x64 Checked Build and Test
@dotnet-bot retest Ubuntu x64 Checked Build and Test

AndyAyersMS · 2017-01-25T05:45:12Z

OSX and Ubuntu still broken, seems to be hitting all recent runs. The runtime can't initialize itself:

18:29:57                coreclr_initialize failed - status: 0x80131534

AndyAyersMS · 2017-01-25T20:45:03Z

Retrying these now that recent CI issues were cleared up.

@dotnet-bot retest OSX x64 Checked Build and Test
@dotnet-bot retest Ubuntu x64 Checked Build and Test

In runtimes that do not support thread abort, a try-finally with an empty try can be optimized to simply the content of the finally. See documentation update for more details. Closes #8924.

AndyAyersMS · 2017-01-25T22:12:21Z

Rebasing to try get past the latest CI issues.

AndyAyersMS · 2017-01-25T23:13:40Z

@wtgodbe Ubuntu still not working after rebase over #9113 -- coreclr fails to initialize.

AndyAyersMS · 2017-01-26T01:09:12Z

One more try with the last round of CI updates.

@dotnet-bot retest OSX x64 Checked Build and Test
@dotnet-bot retest Ubuntu x64 Checked Build and Test

AndyAyersMS · 2017-01-26T03:13:29Z

OSX failed in an unusual way (log), will retry.

17:10:34 Building remotely on dci-macpro-06 (mac-roslyn mac-unpacker mac) in workspace /Users/dotnet-bot/j/workspace/dotnet_coreclr/master/checked_osx_flow_prtest@5
17:10:34 [WS-CLEANUP] Deleting project workspace...
17:10:40 
ERROR: [WS-CLEANUP] Cannot delete workspace: remote file operation failed: /Users/dotnet-bot/j/workspace/dotnet_coreclr/master/checked_osx_flow_prtest@5 at hudson.remoting.Channel@7d8e20d9:dci-macpro-06: hudson.remoting.ChannelClosedException: channel is already closed
17:10:40 ERROR: Cannot delete workspace: remote file operation failed: /Users/dotnet-bot/j/workspace/dotnet_coreclr/master/checked_osx_flow_prtest@5 at hudson.remoting.Channel@7d8e20d9:dci-macpro-06: hudson.remoting.ChannelClosedException: channel is already closed

@dotnet-bot retest OSX x64 Checked Build and Test

AndyAyersMS · 2017-01-26T05:20:07Z

@BruceForstall I think this is ready....

BruceForstall · 2017-01-26T18:19:52Z

src/jit/flowgraph.cpp

+//    In runtimes where thread abort is not possible, `try {} finally {S}`
+//    can be optimized to simply `S`. This method looks for such
+//    cases and removes the try-finally from the EH table, making
+//    suitable flow, block flag, statement, and reigon updates.


reigon [](start = 48, length = 6)

typo: reigon

BruceForstall · 2017-01-26T18:22:38Z

src/jit/flowgraph.cpp

+    JITDUMP("\n*************** In fgRemoveEmptyTry()\n");
+
+#if FEATURE_CORECLR
+    bool enableRemoveEmptyTry = true;


This needs to be #ifdef. Same for the case in fgCloneFinally().

BruceForstall · 2017-01-26T18:30:41Z

src/jit/flowgraph.cpp

+            XTnum++;
+            continue;
+        }
+


It's interesting that all 3 of your try/finally optimizations loop the EH table looking for finally. Not a big deal. And keeps the optimization functions self-contained and independent. But you could presumably set a global "this method has a finally" bool set during EH import. (Assuming nobody adds finallys afterwards, which I think is true (although fgAddSyncMethodEnterExit() for synchronized methods on non-x86 injects a try/fault)).

We're downstream of fgAddInternal so we see the injected EH. But since it's a manually-cloned finally we don't do anything with it.

Will leave the structure as is for now...

BruceForstall · 2017-01-26T18:39:07Z

Found a couple nits. Otherwise, LGTM. You should turn it on for desktop (using your complus var) and run desktop SPMI asm diffs to check diffs and ensure no amd64 asserts across more code.

AndyAyersMS · 2017-01-27T17:35:19Z

No asserts in desktop SPMI. Lots of diffs. Around 50K methods with size improvements (often substantial) and 10 or so with small regressions.

AndyAyersMS · 2017-01-27T17:41:00Z

Number of empty trys is much less than 50K though, there is a bit of version skew with the baseline and so I see frequent small unrelated diffs.

In runtimes that do not support thread abort, a try-finally with an empty try can be optimized to simply the content of the finally. See documentation update for more details. Closes dotnet/coreclr#8924. Commit migrated from dotnet/coreclr@119b04c

AndyAyersMS added * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) area-CodeGen optimization labels Jan 13, 2017

dnfclas added the cla-already-signed label Jan 13, 2017

mikedn reviewed Jan 13, 2017

View reviewed changes

AndyAyersMS mentioned this pull request Jan 14, 2017

JIT: fix bad assumption in non-funclet EH models #8952

Merged

AndyAyersMS removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jan 14, 2017

BruceForstall reviewed Jan 18, 2017

View reviewed changes

AndyAyersMS force-pushed the EmptyTryRemoval branch from 00e325a to 75f75d7 Compare January 18, 2017 03:09

BruceForstall reviewed Jan 18, 2017

View reviewed changes

AndyAyersMS force-pushed the EmptyTryRemoval branch from a3bd368 to 8c3b7f1 Compare January 24, 2017 23:45

JIT: Remove empty try regions

f2fda54

In runtimes that do not support thread abort, a try-finally with an empty try can be optimized to simply the content of the finally. See documentation update for more details. Closes #8924.

AndyAyersMS force-pushed the EmptyTryRemoval branch from 7444b39 to f2fda54 Compare January 25, 2017 22:11

BruceForstall reviewed Jan 26, 2017

View reviewed changes

incorporate review feedback

862562e

AndyAyersMS merged commit 119b04c into dotnet:master Jan 27, 2017

AndyAyersMS deleted the EmptyTryRemoval branch January 27, 2017 17:36

karelz modified the milestone: 2.0.0 Aug 28, 2017

danmoseley mentioned this pull request Jan 31, 2020

Cleanup: remove empty try's dotnet/runtime#19995

Closed

kouvel mentioned this pull request Jun 24, 2020

Migrate coreclr's worker thread pool to be able to use the portable thread pool in opt-in fashion dotnet/runtime#38225

Merged

JIT: Remove empty try regions #8949

JIT: Remove empty try regions #8949

Uh oh!

Conversation

AndyAyersMS commented Jan 13, 2017

Uh oh!

AndyAyersMS commented Jan 13, 2017

Uh oh!

mikedn commented Jan 13, 2017

Uh oh!

AndyAyersMS commented Jan 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Jan 13, 2017

Uh oh!

AndyAyersMS commented Jan 13, 2017

Uh oh!

AndyAyersMS commented Jan 13, 2017

Uh oh!

AndyAyersMS commented Jan 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BruceForstall Jan 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BruceForstall commented Jan 18, 2017

Uh oh!

AndyAyersMS commented Jan 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BruceForstall Jan 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Jan 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davghouse commented Jan 24, 2017

Uh oh!

mikedn commented Jan 24, 2017

BruceForstall Jan 18, 2017 •

edited

Loading

BruceForstall Jan 18, 2017 •

edited

Loading

AndyAyersMS commented Jan 24, 2017 •

edited

Loading