Support unboxing a true nullable and opt-in to faster emitted IL invoke stubs

[steveharter]

From this PR, the ability to generate faster emit stubs from Mono was disabled since it caused issues in some cases when invoking methods that have a Nullable<T> as an argument. This was disabled by these 2 #if statements (which should be removed with this issue):

• runtime/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvokerCommon.cs

  Line 110 in 7980421

  #if !MONO // Temporary until Mono can unbox a true Nullable<T>

• runtime/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvokerCommon.cs

  Line 139 in 7980421

  #if !MONO // Temporary until Mono can unbox a true Nullable<T>

Various tests fail in System.Reflection.Tests such as Invoke(Type methodDeclaringType, string methodName, object obj, object[] parameters, object result) where it is passing yield return new object[] { typeof(MethodInfoDefaultParameters), "NullableInt", new MethodInfoDefaultParameters(), new object[] { (int?)42 }, (int?)42 } as the test data.

The issue is that the mono runtime is calling the Unbox() and UnboxExact() methods on Nullable<T> and appear to be passing int, for example, instead of Nullable<int> for the object parameter. The reflection invoke code creates temporary "true nullable" boxed objects (the default behavior of boxing is to create T or null from a Nullable<T>) in order to invoke a method that has a nullable parameter, and runtime is not detecting them.

Enabling this support and removing the 2 #if statements should result in a 10%-20% perf gain invoke cases where these paths are hit.

[vargaz]

Which part of the runtime needs to be modified to handle these boxed nullable objects ? The unbox.any implementation ?

[steveharter]

Which part of the runtime needs to be modified to handle these boxed nullable objects ? The unbox.any implementation ?

Yes the Unbox_Any implementation which is used in the current emit code. However, for consistency, Unbox should be supported as well IMO.

[vargaz]

Wouldn't this slow down the unbox implementation, since now it has to check for 2 cases instead of 1 ?

[vargaz]

Was this a change made to support the invoke functionality, or the clr always worked like that ?

[MichalPetryka]

Unbox should be supported as well IMO.

@jkotas was considering adding an ECMA amendment forbidding such usage here since even CoreCLR doesn't support it.

[vargaz]

What should be the semantics of unbox.any in this case ? I.e. if the input object is a 'true' boxed nullable, and
unbox Nullable<int> is executed ?

[steveharter]

Was this a change made to support the invoke functionality, or the clr always worked like that ?

In v7 reflection added emit support that normalized all object-based parameter values for either the new emit code or the current interpreted mode and this required temporary creating a boxed true nullable (see "ReboxFromNullable" and "ReboxToNullable") which Mono also added runtime support for. However, these helper methods were manually called by reflection until the PR linked in the description for this issue which added an unbox_any which was already supported by the clr (but not Mono) - the unbox_any wasn't required previously since a void* to the boxed object was provided to the emit-based code.

It is possible to change reflection to call these helper methods instead of unbox_any, at least for Mono, but that isn't ideal since the work to create the true nullable already occurred and that logic is shared across both clrs, and because as mentioned earlier the input is is "normalized" for either invoking the void* emit stub or the new object-based emit stub.

[EgorBo]

What should be the semantics of unbox.any in this case ? I.e. if the input object is a 'true' boxed nullable, and unbox Nullable<int> is executed ?

Looks like it's expected to throw InvalidCastException in this case if I read the spec correctly:

so whatever we access with unbox.any should not be Nullable<T>, but just T.

[steveharter]

FWIW, see

runtime/src/coreclr/vm/object.cpp

Lines 1681 to 1682 in 9ffdb53

	// For safety's sake, also allow true nullables to be unboxed normally.
	// This should not happen normally, but we want to be robust

[EgorBo]

FWIW, see

runtime/src/coreclr/vm/object.cpp

Lines 1681 to 1682 in 9ffdb53

	// For safety's sake, also allow true nullables to be unboxed normally.
	// This should not happen normally, but we want to be robust

Sounds like we break the spec then? 😄

[vargaz]

What should be the result of an unbox Nullable<int> ? A Nullable<int> structure on the stack ? Or an int on the stack ?

[EgorBo]

What should be the result of an unbox Nullable<int> ? A Nullable<int> structure on the stack ? Or an int on the stack ?

That's another (or the same) case where we brake the spec, the spec says that unbox Nullable<> is expected to allocate a new object on heap, create Nullable<> there and return pointer to its value (because normally, boxed Nullable<T> is always just T in the heap) - but that would be kinda wasteful to allocate on heap on every unbox? 😐

So yeah, we do allocate it on stack

[jkotas]

I do not think it makes sense to try to replicate the current questionable poorly defined RyuJIT behavior in Mono JIT and interpreter.

@steveharter Can we fix the reflection invoke to avoid dependency on this behavior? It should not result into any performance loss - create the local to store the Nullable<T> into and use unbox.any for the unboxing operation.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

---

From this PR, the ability to generate faster emit stubs from Mono was disabled since it caused issues in some cases when invoking methods that have a Nullable<T> as an argument. This was disabled by these 2 #if statements (which should be removed with this issue):

• runtime/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvokerCommon.cs

  Line 110 in 7980421

  #if !MONO // Temporary until Mono can unbox a true Nullable<T>

• runtime/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvokerCommon.cs

  Line 139 in 7980421

  #if !MONO // Temporary until Mono can unbox a true Nullable<T>

Various tests fail in System.Reflection.Tests such as Invoke(Type methodDeclaringType, string methodName, object obj, object[] parameters, object result) where it is passing yield return new object[] { typeof(MethodInfoDefaultParameters), "NullableInt", new MethodInfoDefaultParameters(), new object[] { (int?)42 }, (int?)42 } as the test data.

The issue is that the mono runtime is calling the Unbox() and UnboxExact() methods on Nullable<T> and appear to be passing int, for example, instead of Nullable<int> for the object parameter. The reflection invoke code creates temporary "true nullable" boxed objects (the default behavior of boxing is to create T or null from a Nullable<T>) in order to invoke a method that has a nullable parameter, and runtime is not detecting them.

Enabling this support and removing the 2 #if statements should result in a 10%-20% perf gain invoke cases where these paths are hit.

Author:	steveharter
Assignees:	vargaz
Labels:	area-System.Reflection, tenet-performance, untriaged
Milestone:	-

[steveharter]

Closing - we'll go with a different emit approach.