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

Conversation

kouvel
Copy link

@kouvel kouvel commented Oct 22, 2018

Description

A fix for https://github.com/dotnet/coreclr/issues/19752 appears to be desired before enabling tiered compilation by default for 2.2. Some small steady-state performance regressions (~5%) have been seen in TechEmpower benchmarks running without the R2R (TechEmpower runs benchmarks on aspnetcore with COMPlus_ReadyToRun=0). In default mode, where R2R is enabled and the aspnetcore shared framework is used, tiered compilation typically improves steady-state performance. The fix is not too small and involves some risk that does not appear to be appropriate for 2.2, so this change disables tiered compilation by default for 2.2 RTM.

Customer impact

  • Eliminates regressions from tiered compilation
  • Improvements from tiered compilation being enabled would be lost by default. Customers may still choose to enable tiered compilation.

Regression?

No

Packaging reviewed?

Yes, no impact (coreclr only)

Risk

Low, CI has also been testing with tiered compilation disabled

@kouvel kouvel added Servicing-consider Issue for next servicing release review area-CodeVersioning labels Oct 22, 2018
@kouvel kouvel added this to the 2.2 milestone Oct 22, 2018
@kouvel kouvel self-assigned this Oct 22, 2018
@kouvel kouvel requested a review from noahfalk October 22, 2018 19:05
@kouvel kouvel added Servicing-consider Issue for next servicing release review and removed Servicing-consider Issue for next servicing release review labels Oct 22, 2018
@noahfalk
Copy link
Member

I imagine this commit text could get some publicity from people worried that they shouldn't turn on tiered compilation, so I think we should be very explicit about what conditions are being compared to generate this 5% difference, and whether or not any customer is likely to find themselves in this state via a normal development/deployment scenario.

You described it above as 'without R2R'ed ASP.NET shared framework', but I wanted to double check as I had a different understanding (and I could be dead wrong ; ). My assumption is that the perf lab was measuring with all R2R and fragile NGEN disabled - in particular, that assemblies such as System.Private.CoreLib.dll and System.Net.dll were being jitted. The only normal dev scenario I am aware of that approaches this level of jitting is packaging the app as a self-contained app but even that may not disable enough pre-compiled code to show the 5% delta?

Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

The code change looked good to me, my only concern is to confirm the commit text accurately describes the perf measuring conditions.

@benaadams
Copy link
Member

benaadams commented Oct 22, 2018

My assumption is that the perf lab was measuring with all R2R and fragile NGEN disabled - in particular, that assemblies such as System.Private.CoreLib.dll and System.Net.dll were being jitted.

It runs with the environment flag

ENV COMPlus_ReadyToRun 0

TechEmpower/FrameworkBenchmarks/.../aspcore.dockerfile

@kouvel
Copy link
Author

kouvel commented Oct 22, 2018

Clarified inline above

@noahfalk
Copy link
Member

thanks for timely info @benaadams 👍

@AndyAyersMS
Copy link
Member

Did we get any other feedback from enabling tiering in the 2.2 previews?

I know of #20421 but that came from somebody looking at 3.0....

@kouvel
Copy link
Author

kouvel commented Oct 22, 2018

I have not seen much concrete feedback from 2.2 previews so far. A couple of mentions of known perf issues, and generic posts here and there is all I saw.

@kouvel
Copy link
Author

kouvel commented Oct 23, 2018

@dotnet-bot test Ubuntu arm64 Cross Debug Innerloop Build

@kouvel kouvel changed the title Disable tiered compilation by default for 2.2 RTM [2.2] Disable tiered compilation by default for 2.2 RTM Oct 23, 2018
@kouvel
Copy link
Author

kouvel commented Oct 23, 2018

@dotnet-bot test this please

@vivmishra vivmishra added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Oct 23, 2018
@vivmishra
Copy link

Approved for 2.2 RTM

@kouvel
Copy link
Author

kouvel commented Oct 23, 2018

@dotnet-bot test Windows_NT x64 Formatting

@kouvel
Copy link
Author

kouvel commented Oct 24, 2018

@dotnet-bot test this please

@kouvel
Copy link
Author

kouvel commented Oct 24, 2018

@dotnet-bot test Windows_NT x64 perf scenarios
test Windows_NT x64 min_opts perf scenarios
test Windows_NT x86 perf scenarios
test Windows_NT x86 min_opts perf scenarios

@kouvel
Copy link
Author

kouvel commented Oct 24, 2018

@dotnet-bot test Windows_NT x64 Formatting

The following jobs seem to be broken, as seen in the test job #20570, the failures are not caused by this change:

  • Ubuntu arm64 Cross Debug Innerloop Build
  • Windows_NT x64 full_opt ryujit Performance Scenarios Tests
  • Windows_NT x86 full_opt ryujit Performance Scenarios Tests

@kouvel
Copy link
Author

kouvel commented Oct 24, 2018

@dotnet-bot test Windows_NT x64 Formatting

@kouvel kouvel merged commit 70846ce into dotnet:release/2.2 Oct 25, 2018
@kouvel kouvel deleted the DisableTiering branch October 25, 2018 19:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Servicing-approved Approved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants