Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

JIT: minopts codegen strategy in impRuntimeLookupToTree#13305

Closed
AndyAyersMS wants to merge 1 commit into
dotnet:masterfrom
AndyAyersMS:SmallerRuntimeLookup
Closed

JIT: minopts codegen strategy in impRuntimeLookupToTree#13305
AndyAyersMS wants to merge 1 commit into
dotnet:masterfrom
AndyAyersMS:SmallerRuntimeLookup

Conversation

@AndyAyersMS
Copy link
Copy Markdown
Member

In minopts/debug/rare-block modes, try and minimize code size and
jit time by always chosing the helper call for complex runtime lookups.

In minopts/debug/rare-block modes, try and minimize code size and
jit time by always chosing the helper call for complex runtime lookups.
@AndyAyersMS
Copy link
Copy Markdown
Member Author

Similar to what was done in #13188, use the helper call for runtime lookups if:

  • the jit can use a helper call
  • the jit is trying to minimize jit time or the tree lookup is in a rarely run block
  • it is likely that the helper call is less code.

This one is hard to evaluate via jit-diff, since R2R always uses the helper today. So we can only see diffs in corelib where we are still using fragile prejitting.

No diffs in corelib with full opt.

With minopts forced on, we see:

Total bytes of diff: -156793 (-0.93 % 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):
     -156793 : System.Private.CoreLib.dasm (-3.19 % of base)
1 total files with size differences (1 improved, 0 regressed), 78 unchanged.
Top method improvements by size (bytes):
      -14722 : System.Private.CoreLib.dasm - Dictionary`2:.ctor(ref,ref):this (58 methods)
       -6919 : System.Private.CoreLib.dasm - Dictionary`2:OnDeserialization(ref):this (29 methods)
       -4415 : System.Private.CoreLib.dasm - ValueTuple`8:CompareTo(struct):int:this (15 methods)
       -3815 : System.Private.CoreLib.dasm - Dictionary`2:System.Collections.ICollection.CopyTo(ref,int):this (29 methods)
       -3634 : System.Private.CoreLib.dasm - Enumerator:System.Collections.IEnumerator.get_Current():ref:this (117 methods)
883 total methods with size differences (883 improved, 0 regressed), 65342 unchanged.

Still evaluating on desktop now since SPMI should give us a better take on the size impact.

Throughput improvement for minopts/debug also tough to get a handle on, because handle expansion also varies between jitting and prejitting. It is likely in the 2-3% range. Will see if I can get plausible measurements from jitting and report back.

@dotnet-bot test Windows_NT x64 Throughput
@dotnet-bot test Windows_NT minopts

@JosephTremoulet PTAL
cc @dotnet/jit-contrib

Copy link
Copy Markdown

@JosephTremoulet JosephTremoulet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, w/ one comment comment

Comment thread src/jit/importer.cpp
// Also used if possible when optimizing jit code size or jit time, and
// the inline sequences below will result in larger code or slower jitting.
//
// 2. pLookup->indirections != CORINFO_USEHELPER :
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cases in the comments here describe the old logic. Maybe just mention under pLookup->indirections != CORINFO_USEHELPER that using a helper call is a valid option at the jit's discretion.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did add something to the "1" entry that was supposed to cover the subsequent cases. Let me call it out better.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

@dotnet/dnceng any idea why the arm testing is backing up like this?

@mmitche
Copy link
Copy Markdown
Member

mmitche commented Aug 10, 2017

@AndyAyersMS Yeah, Azure updated a custom script extension and broke some VM images. I'm fixing now.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

TP job results are not what I expected.

Here are the results plotted as diff/base ratio vs base time (so higher that 1.0 is worse for this change):
image
Net change is negative at 1.006, geoman however is positive at 0.98.

I would expect most of the larger assemblies (the points further to the right) to be below 1.0 as this change should clearly speed up minopts jit time.

Am going to rerun just to see if the second run gives similar results, since this job is new and we don't know how consistent it is just yet.

@dotnet-bot test Windows_NT x64 Throughput

@AndyAyersMS
Copy link
Copy Markdown
Member Author

Perhaps odder still: the TP job just runs the default crossgen, and so is generating R2R code for all the assemblies (it skips corelib). So there should be no impact from this change on the TP job as the logic for R2R hasn't changed any.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

If I pick some jit-intensive scenario (say running the checked build of CscBench, which is 75% jit time) and force-enable minopts, and look at instructions retired, the jit contribution to instructions retired drops about 4%.

Overall process instructions retired however says flat, so evidently the reduction in jit time is matched by a similar increase in the time to execute the jitted code.

So one might question whether this is the right approach or the right tuning -- it seems reasonable for minopts (less risk) and for the eventual Tier0 (where Tier0 jitted code should not run often) but maybe not as reasonable for debug codegen?

@karelz
Copy link
Copy Markdown
Member

karelz commented Aug 25, 2017

@AndyAyersMS any update on this PR? Last update was from 2 weeks ago ...

@AndyAyersMS
Copy link
Copy Markdown
Member Author

I'm still looking for the right way to assess CQ/TP tradeoffs in debug mode. May need to do some testing on desktop, for instance F5 scenarios in VS.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

Still looking for the best way to evaluate this. Since nothing's imminent, I'm going to close it for now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants