Tiered compilation fails to elide value type null check

[sharwell]

AsyncMethodBuilderCore is implemented with the assumption that value type state machines will not be boxed for a runtime comparison with null:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncMethodBuilderCore.cs

Lines 23 to 26 in f05dee0

	if (stateMachine == null) // TStateMachines are generally non-nullable value types, so this check will be elided
	{
	ThrowHelper.ThrowArgumentNullException(ExceptionArgument.stateMachine);
	}

This assumption does not hold for Tier 0 compilation. For a release build of Roslyn, execution of analyzers on Roslyn.sln produces 20% (70GB) of its total allocations boxing state machines for this null check.

[stephentoub]

Related:
https://github.com/dotnet/coreclr/issues/14474
dotnet/coreclr#22984
dotnet/coreclr@master...AndyAyersMS:EnableBoxingOptsTier0

cc: @AndyAyersMS, @kouvel

[AndyAyersMS]

Thanks -- is this with latest builds only or do you see it in 3.x too?

The fix for unexpected boxing at Tier0 in the 3.0 cycle was:

• selectively use AggressiveOptimization on some key methods (Avoid boxing allocations for async in Tier0 coreclr#22984).
• enable some simple IL pattern based opts (Match BOX idioms early in the importer coreclr#23550).

I would have expected the IL pattern based opts to kick in here.

[sharwell]

I was running the tool using the executable it generated during the build. Is there a way to see which version of dotnet it executes under the covers?

[sharwell]

I re-ran this with the dotnet tool to get a specific version where the problematic behavior is observed:

PS C:\dev\roslyn> dotnet --version
5.0.100-alpha1-015536

[AndyAyersMS]

Yes, the boxing remains in Tier0, but gets optimized away in Tier1.

The unboxing opt at Tier0 gets blocked by the fact that we're boxing a byref arg and the byref might be null. At Tier1 we're able to handle this by leaving a residual null check behind.

Let me see how hard it is to extend the Tier0 version to cover this case.

[AndyAyersMS]

I'm going to label this as codegen.

We've seen Tier0 code do a lot of allocation in a short time window -- but would be good to verify that the tiering logic is working as expected here. So @kouvel it is probably interesting to look at when/if tiering kicks in on this case.

[sharwell]

💭 One option would be extracting this type of argument null check to a method where aggressive optimizations can be applied. This would allow the code to use the fast null check in Tier 0 without impacting the normal Tier0→Tier1 path.

[AndyAyersMS]

This looks like it can be fairly easily fixed in the jit -- I'm evaluating a prospective fix now.

[AndyAyersMS]

@sharwell is this something you think we should try and fix in 3.1? I don't know if it would make the cut, but we could propose it and see.

[sharwell]

@AndyAyersMS it really depends on whether users are expected to use tiered compilation. The numbers at the top are accurate for our scenario, but the issue is resolved by disabling the feature.

[AndyAyersMS]

It's on by default, so yes we expect most users to use it. If I get this fixed in 3.x will you re-enable?

We do not want users to see regressions when moving from 2.x to 3.x, or decide the feature as a whole can't be trusted and decide to just turn it off forever.