[2.2] Port some changes relevant to tiered compilation to 2.2 #20265

kouvel · 2018-10-04T19:13:00Z

Port of the following PRs to 2.2:

Replace assert in Precode::SetTargetInterlocked #20007 - Changes only affect debug builds
- Replace incorrect assert in Precode::SetTargetInterlocked
Fix perf scenarios in the min_opt config #20010 - Changes only affect perf test infrastructure
- Fix perf scenarios in the min_opt config
Add MethodImplOptions.AggressiveOptimization and use it for tiering #20009
- Add MethodImplOptions.AggressiveOptimization and use it for tiering
- Workaround for https://github.com/dotnet/coreclr/issues/19751
- API review: https://github.com/dotnet/corefx/issues/32235
Add MethodImplAttributes.AggressiveOptimization #20274
- Add MethodImplAttributes.AggressiveOptimization for reflection access
- API review: https://github.com/dotnet/corefx/issues/32628

Description

Ports of #20007 and #20010 don't affect shipping code.

#20009 - Add MethodImplOptions.AggressiveOptimization and use it for tiering

PR [2.2] Expose MethodImplOptions.AggressiveOptimization and MethodImplAttributes.AggressiveOptimization corefx#32622 is also included in the set
It's likely that people would run into issue https://github.com/dotnet/coreclr/issues/19751 in their benchmarks when tiering is enabled by default, and it was determined that we should have this workaround for 2.2 RTM when tiered compilation is enabled
PR [2.2] Disable tiered compilation by default for 2.2 RTM #20525 disables tiered compilation by default in 2.2. Despite that, we would like to encourage customers to use tiered compilation in 2.2 RTM and these changes would provide them a useful workaround for the issue above.
Added MethodImplOptions.AggressiveOptimization to be used in a MethodImplAttribute to indicate that the method contains hot code
For a method flagged with AggressiveOptimization, tiering uses a foreground tier 1 JIT on first call to the method, skipping the tier 0 JIT and call counting
Crossgen does not generate code for a method flagged with AggressiveOptimization, such that it will be jitted at run-time, whether tiering is enabled or not
Refactored some tiered compilation logic to simplify and to limit the frequency of metadata lookups necessary to check for the AggressiveOptimization flag

Adds a new keyword aggressiveoptimization to IL ASM:

.method private hidebysig static void  Main() cil managed aggressiveoptimization

Customer impact

No impact for methods that are not flagged with AggressiveOptimization
Methods flagged with AggressiveOptimization will be jitted with optimizations at run-time
Enables working around issue https://github.com/dotnet/coreclr/issues/19751

Regression?

The port of Replace assert in Precode::SetTargetInterlocked #20007 is a fix for a regression in Enable tiered compilation #19691
Other ports are not fixes for regressions

Packaging reviewed?

Yes, no impact (coreclr only)

Risk

Relatively low, as there are currently no methods in .NET Core that are flagged with AggressiveOptimization
Metadata lookups add some time to the prestub path (first invocation of a method, and each invocation while counting calls), but there was no measurable difference in perf (the prestub path is already very heavy)
Binary compat
- Older runtimes run newer binaries with the AggressiveOptimization flag without error, but would ignore the flag
- Older ildasm disassembles newer binaries with the AggressiveOptimization flag without error, but strips the flag
- Newer runtimes run older binaries without issue
- Older ilasm would fail to compile newer ildasm output if it includes the new aggressiveoptimization keyword, this is by design

AndyAyersMS · 2018-10-05T15:58:17Z

src/vm/codeversion.cpp

-    m_optTier(NativeCodeVersion::OptimizationTier0),
+#ifdef FEATURE_TIERED_COMPILATION
+    m_optTier(optimizationTier),
+#endif


Nit: I don't think this will compile if FEATURE_TIERED_COMPILATION is not defined. Not sure if it is worth fixing though.

It looked like m_optTier is defined under FEATURE_TIERED_COMPILATION so I thought that it would not compile without the define before the change

Sorry, should have been more specific -- OptimizationTier is only defined under FEATURE_TIERED_COMPILATION, so it is the argument list where the problem arises, not the initializer list.

Oh ok, I had moved that out of the define here: https://github.com/dotnet/coreclr/pull/20265/files#diff-3fc5234394621d5eb9524166a67ddd82R67 due to some paths that are outside the define needing to refer to it, though it won't actually be stored or used without the define. There's probably a better way to do it but I had settled on that at the time.

Ah I see. I think it's fine how you have it ... varying signatures based on defines is always annoying.

AndyAyersMS

LGTM

kouvel · 2018-10-17T19:01:57Z

@dotnet-bot test CentOS7.1 x64 Checked Innerloop Build and Test

Fixes https://github.com/dotnet/coreclr/issues/19954 - `SetTargetInterlocked` can be soon followed by `ResetTargetInterlocked`, so the assert at the end of `SetTargetInterlocked` is invalid - Removed the assert and instead just verified that the specified target is not the default prestub target

The appropriate environment variables were not being set due to a name mismatch

…20009) Add MethodImplOptions.AggressiveOptimization and use it for tiering Part of fix for https://github.com/dotnet/corefx/issues/32235 Workaround for https://github.com/dotnet/coreclr/issues/19751 - Added and set CORJIT_FLAG_AGGRESSIVE_OPT to indicate that a method is flagged with AggressiveOptimization - For a method flagged with AggressiveOptimization, tiering uses a foreground tier 1 JIT on first call to the method, skipping the tier 0 JIT and call counting - When tiering is disabled, a method flagged with AggressiveOptimization does not use r2r-pregenerated code - R2r crossgen does not generate code for a method flagged with AggressiveOptimization

API review: https://github.com/dotnet/corefx/issues/32628

…is branch

kouvel · 2018-10-23T04:36:49Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test
test Ubuntu arm64 Cross Debug Innerloop Build

kouvel · 2018-10-23T17:56:09Z

@dotnet-bot test this please

vivmishra · 2018-10-23T18:25:01Z

Rejected for 2.2 but approved for 3.0.

kouvel added the area-CodeVersioning label Oct 4, 2018

kouvel added this to the 2.2 milestone Oct 4, 2018

kouvel self-assigned this Oct 4, 2018

kouvel requested review from AndyAyersMS, jkotas and noahfalk October 4, 2018 19:13

kouvel mentioned this pull request Oct 4, 2018

[2.2] Expose MethodImplOptions.AggressiveOptimization and MethodImplAttributes.AggressiveOptimization dotnet/corefx#32622

Closed

noahfalk approved these changes Oct 5, 2018

View reviewed changes

AndyAyersMS reviewed Oct 5, 2018

View reviewed changes

AndyAyersMS approved these changes Oct 5, 2018

View reviewed changes

kouvel and others added 5 commits October 22, 2018 10:00

Fix perf scenarios in the min_opt config (#20010)

0d414e6

The appropriate environment variables were not being set due to a name mismatch

Add MethodImplAttributes.AggressiveOptimization

15fb053

API review: https://github.com/dotnet/corefx/issues/32628

Bring back MethodDesc::GetInitialCodeVersion as it's still used in th…

2844144

…is branch

kouvel added Servicing-consider Issue for next servicing release review and removed Servicing-consider Issue for next servicing release review labels Oct 22, 2018

vivmishra added Servicing-rejected and removed Servicing-consider Issue for next servicing release review labels Oct 23, 2018

kouvel closed this Oct 23, 2018

kouvel deleted the NoTierMethod22 branch October 23, 2018 18:36

kouvel removed area-CodeVersioning labels Dec 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[2.2] Port some changes relevant to tiered compilation to 2.2 #20265

[2.2] Port some changes relevant to tiered compilation to 2.2 #20265

Uh oh!

kouvel commented Oct 4, 2018 •

edited

Loading

Uh oh!

AndyAyersMS Oct 5, 2018

Uh oh!

kouvel Oct 5, 2018

Uh oh!

AndyAyersMS Oct 5, 2018

Uh oh!

kouvel Oct 5, 2018

Uh oh!

AndyAyersMS Oct 5, 2018

Uh oh!

AndyAyersMS left a comment

Uh oh!

kouvel commented Oct 17, 2018

Uh oh!

kouvel commented Oct 23, 2018

Uh oh!

kouvel commented Oct 23, 2018

Uh oh!

vivmishra commented Oct 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[2.2] Port some changes relevant to tiered compilation to 2.2 #20265

[2.2] Port some changes relevant to tiered compilation to 2.2 #20265

Uh oh!

Conversation

kouvel commented Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Customer impact

Regression?

Packaging reviewed?

Risk

Uh oh!

AndyAyersMS Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

kouvel Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

kouvel Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS left a comment

Choose a reason for hiding this comment

Uh oh!

kouvel commented Oct 17, 2018

Uh oh!

kouvel commented Oct 23, 2018

Uh oh!

kouvel commented Oct 23, 2018

Uh oh!

vivmishra commented Oct 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kouvel commented Oct 4, 2018 •

edited

Loading