Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JIT: Remove empty try regions #8949

Merged
merged 2 commits into from Jan 27, 2017

Conversation

Projects
None yet
6 participants
@AndyAyersMS
Copy link
Member

commented Jan 13, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2017

You probably wanted @BruceForstall

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2017

Yeah, thanks.

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

This comment has been minimized.

Copy link
@mikedn

mikedn Jan 13, 2017

Contributor

callaways -> callalways?

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2017

Retesting ubuntu now that #8938 is in...

@dotnet-bot retest Ubuntu x64 Checked Build and Test

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2017

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

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2017

@dotnet-bot retest OSX x64 Checked Build and Test


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

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 18, 2017

Member

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

typo: "thea"

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Jan 18, 2017

Author Member

Fixed


* 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;

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 18, 2017

Member

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.

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Jan 18, 2017

Author Member

Yep, will fix.

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.

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 18, 2017

Member

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?

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Jan 18, 2017

Author Member

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...

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

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 18, 2017

Member

ARM32 doesn't use thunks.

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Jan 18, 2017

Author Member

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

@@ -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)

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 18, 2017

Member

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

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Jan 18, 2017

Author Member

Sure

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

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 18, 2017

Member

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

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Jan 18, 2017

Author Member

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

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

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 18, 2017

Member

copy-paste error: rename func

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Jan 18, 2017

Author Member

Fixed.


// (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.

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 18, 2017

Member

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

typo: regon

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

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 18, 2017

Member

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

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Jan 18, 2017

Author Member

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

assert(verifiedSingleCallfinally);
continue;
}

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 18, 2017

Member

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?

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Jan 18, 2017

Author Member

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

This comment has been minimized.

Copy link
Member

commented Jan 18, 2017

@AndyAyersMS Mostly minor comments, overall looks very good.

@AndyAyersMS AndyAyersMS force-pushed the AndyAyersMS:EmptyTryRemoval branch from 00e325a to 75f75d7 Jan 18, 2017

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2017

@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.


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

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 18, 2017

Member

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

typo: multple

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

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 18, 2017

Member

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

typo: aways


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

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 18, 2017

Member

Is the word "points" unnecessary? Seems confusing.

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2017

Updated with fixes for comments plus the locally introduced instances of #if DEBUG. Rebased to get past merge conflict in morph.cpp; as part of that, this optimization is now disabled for arm like the other two finally optimizations, pending resolution of #9015.

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2017

Will need to be reconciled with #9065.

@davghouse

This comment has been minimized.

Copy link

commented Jan 24, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2017

@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

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2017

@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 AndyAyersMS force-pushed the AndyAyersMS:EmptyTryRemoval branch from a3bd368 to 8c3b7f1 Jan 24, 2017

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2017

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

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2017

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

JIT: Remove empty try regions
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 AndyAyersMS force-pushed the AndyAyersMS:EmptyTryRemoval branch from 7444b39 to f2fda54 Jan 25, 2017

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2017

Rebasing to try get past the latest CI issues.

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2017

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

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

commented Jan 26, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Jan 26, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Jan 26, 2017

@BruceForstall I think this is ready....

// 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.

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 26, 2017

Member

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

typo: reigon

JITDUMP("\n*************** In fgRemoveEmptyTry()\n");

#if FEATURE_CORECLR
bool enableRemoveEmptyTry = true;

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 26, 2017

Member

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

XTnum++;
continue;
}

This comment has been minimized.

Copy link
@BruceForstall

BruceForstall Jan 26, 2017

Member

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)).

This comment has been minimized.

Copy link
@AndyAyersMS

AndyAyersMS Jan 26, 2017

Author Member

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

This comment has been minimized.

Copy link
Member

commented Jan 26, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Jan 27, 2017

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

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

13 checks passed

CentOS7.1 x64 Debug Build and Test Build finished.
Details
FreeBSD x64 Checked Build Build finished.
Details
Linux ARM Emulator Cross Debug Build Build finished.
Details
Linux ARM Emulator Cross Release Build Build finished.
Details
OSX x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Formatting Build finished.
Details
Windows_NT arm Cross Debug Build Build finished.
Details
Windows_NT arm Cross Release Build Build finished.
Details
Windows_NT x64 Debug Build and Test Build finished.
Details
Windows_NT x64 Formatting Build finished.
Details
Windows_NT x64 Release Priority 1 Build and Test Build finished.
Details
Windows_NT x86 Checked Build and Test Build finished.
Details

@AndyAyersMS AndyAyersMS deleted the AndyAyersMS:EmptyTryRemoval branch Jan 27, 2017

@AndyAyersMS

This comment has been minimized.

Copy link
Member Author

commented Jan 27, 2017

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.

@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.