Enable Mono for using faster invoke stubs by steveharter · Pull Request #89108 · dotnet/runtime

steveharter · 2023-07-18T17:02:29Z

Add support for Mono to emit faster invoke stubs for both the new invoke APIs added in #88415 as well as standard invoke.

Replacement for closed issue #88952

ghost · 2023-07-18T17:02:41Z

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

Issue Details

[Verifying Tests]

Author:	steveharter
Assignees:	steveharter
Labels:	`area-System.Reflection`
Milestone:	-

EgorBo · 2023-07-18T19:45:28Z

just curious - is it possible to avoid boxed Nullable<> in the first place? I guess it's not possible to create ones with plain C# (where boxed nullables are boxed as just plain values)

steveharter · 2023-07-19T15:10:05Z

just curious - is it possible to avoid boxed Nullable<> in the first place? I guess it's not possible to create ones with plain C# (where boxed nullables are boxed as just plain values)

It's done this way primarily for the interpreted path, which doesn't use emit and is located in the native runtime -- it either needs to be passed a pointer to a nullable or have special logic that creates a nullable from the null \ <T> input. The design we went with keeps the native code (and also emit) to be minimal, so we went with first approach where we pass in a pointer to a nullable which is triggered from the library reflection invoke code. The most straightforward way of doing that is to box a nullable.

Also, for both the interpreted path and the various emit paths we also wanted to normalize the object arg input and have the same validation\fix-up code so we can select the strategy without having to do special fix-ups and validation per strategy type.

vargaz · 2023-07-24T18:06:28Z

- if (vtype == typeof(IntPtr) || vtype == typeof(UIntPtr))
+ if (vtype == typeof(IntPtr))
 return CheckValueStatus.Success;
+


Why is this needed ?

Thanks for this. With the last commit, it is no longer necessary. Also removed from core CLR.
It was necessary previously since the Opcodes.UnboxAny would throw a InvalidCastException for UIntPtr arguments.

steveharter · 2023-07-18T17:02:29Z

Add support for Mono to emit faster invoke stubs for both the new invoke APIs added in #88415 as well as standard invoke.

Replacement for closed issue #88952

ghost · 2023-07-18T17:02:41Z

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

Issue Details

[Verifying Tests]

Author:	steveharter
Assignees:	steveharter
Labels:	`area-System.Reflection`
Milestone:	-

EgorBo · 2023-07-18T19:45:28Z

just curious - is it possible to avoid boxed Nullable<> in the first place? I guess it's not possible to create ones with plain C# (where boxed nullables are boxed as just plain values)

steveharter · 2023-07-19T15:10:05Z

just curious - is it possible to avoid boxed Nullable<> in the first place? I guess it's not possible to create ones with plain C# (where boxed nullables are boxed as just plain values)

It's done this way primarily for the interpreted path, which doesn't use emit and is located in the native runtime -- it either needs to be passed a pointer to a nullable or have special logic that creates a nullable from the null \ <T> input. The design we went with keeps the native code (and also emit) to be minimal, so we went with first approach where we pass in a pointer to a nullable which is triggered from the library reflection invoke code. The most straightforward way of doing that is to box a nullable.

Also, for both the interpreted path and the various emit paths we also wanted to normalize the object arg input and have the same validation\fix-up code so we can select the strategy without having to do special fix-ups and validation per strategy type.

vargaz · 2023-07-24T18:06:28Z

- if (vtype == typeof(IntPtr) || vtype == typeof(UIntPtr))
+ if (vtype == typeof(IntPtr))
 return CheckValueStatus.Success;
+


Why is this needed ?

Thanks for this. With the last commit, it is no longer necessary. Also removed from core CLR.
It was necessary previously since the Opcodes.UnboxAny would throw a InvalidCastException for UIntPtr arguments.

steveharter · 2023-07-18T17:02:29Z

Add support for Mono to emit faster invoke stubs for both the new invoke APIs added in #88415 as well as standard invoke.

Replacement for closed issue #88952

ghost · 2023-07-18T17:02:41Z

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

Issue Details

[Verifying Tests]

Author:	steveharter
Assignees:	steveharter
Labels:	`area-System.Reflection`
Milestone:	-

EgorBo · 2023-07-18T19:45:28Z

just curious - is it possible to avoid boxed Nullable<> in the first place? I guess it's not possible to create ones with plain C# (where boxed nullables are boxed as just plain values)

steveharter · 2023-07-19T15:10:05Z

just curious - is it possible to avoid boxed Nullable<> in the first place? I guess it's not possible to create ones with plain C# (where boxed nullables are boxed as just plain values)

It's done this way primarily for the interpreted path, which doesn't use emit and is located in the native runtime -- it either needs to be passed a pointer to a nullable or have special logic that creates a nullable from the null \ <T> input. The design we went with keeps the native code (and also emit) to be minimal, so we went with first approach where we pass in a pointer to a nullable which is triggered from the library reflection invoke code. The most straightforward way of doing that is to box a nullable.

Also, for both the interpreted path and the various emit paths we also wanted to normalize the object arg input and have the same validation\fix-up code so we can select the strategy without having to do special fix-ups and validation per strategy type.

vargaz · 2023-07-24T18:06:28Z

- if (vtype == typeof(IntPtr) || vtype == typeof(UIntPtr))
+ if (vtype == typeof(IntPtr))
 return CheckValueStatus.Success;
+


Why is this needed ?

Thanks for this. With the last commit, it is no longer necessary. Also removed from core CLR.
It was necessary previously since the Opcodes.UnboxAny would throw a InvalidCastException for UIntPtr arguments.

steveharter · 2023-07-18T17:02:29Z

Add support for Mono to emit faster invoke stubs for both the new invoke APIs added in #88415 as well as standard invoke.

Replacement for closed issue #88952

ghost · 2023-07-18T17:02:41Z

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

Issue Details

[Verifying Tests]

Author:	steveharter
Assignees:	steveharter
Labels:	`area-System.Reflection`
Milestone:	-

EgorBo · 2023-07-18T19:45:28Z

just curious - is it possible to avoid boxed Nullable<> in the first place? I guess it's not possible to create ones with plain C# (where boxed nullables are boxed as just plain values)

steveharter · 2023-07-19T15:10:05Z

just curious - is it possible to avoid boxed Nullable<> in the first place? I guess it's not possible to create ones with plain C# (where boxed nullables are boxed as just plain values)

It's done this way primarily for the interpreted path, which doesn't use emit and is located in the native runtime -- it either needs to be passed a pointer to a nullable or have special logic that creates a nullable from the null \ <T> input. The design we went with keeps the native code (and also emit) to be minimal, so we went with first approach where we pass in a pointer to a nullable which is triggered from the library reflection invoke code. The most straightforward way of doing that is to box a nullable.

Also, for both the interpreted path and the various emit paths we also wanted to normalize the object arg input and have the same validation\fix-up code so we can select the strategy without having to do special fix-ups and validation per strategy type.

vargaz · 2023-07-24T18:06:28Z

- if (vtype == typeof(IntPtr) || vtype == typeof(UIntPtr))
+ if (vtype == typeof(IntPtr))
 return CheckValueStatus.Success;
+


Why is this needed ?

Thanks for this. With the last commit, it is no longer necessary. Also removed from core CLR.
It was necessary previously since the Opcodes.UnboxAny would throw a InvalidCastException for UIntPtr arguments.

steveharter · 2023-07-18T17:02:29Z

Add support for Mono to emit faster invoke stubs for both the new invoke APIs added in #88415 as well as standard invoke.

Replacement for closed issue #88952

ghost · 2023-07-18T17:02:41Z

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

Issue Details

[Verifying Tests]

Author:	steveharter
Assignees:	steveharter
Labels:	`area-System.Reflection`
Milestone:	-

EgorBo · 2023-07-18T19:45:28Z

just curious - is it possible to avoid boxed Nullable<> in the first place? I guess it's not possible to create ones with plain C# (where boxed nullables are boxed as just plain values)

steveharter · 2023-07-19T15:10:05Z

just curious - is it possible to avoid boxed Nullable<> in the first place? I guess it's not possible to create ones with plain C# (where boxed nullables are boxed as just plain values)

It's done this way primarily for the interpreted path, which doesn't use emit and is located in the native runtime -- it either needs to be passed a pointer to a nullable or have special logic that creates a nullable from the null \ <T> input. The design we went with keeps the native code (and also emit) to be minimal, so we went with first approach where we pass in a pointer to a nullable which is triggered from the library reflection invoke code. The most straightforward way of doing that is to box a nullable.

Also, for both the interpreted path and the various emit paths we also wanted to normalize the object arg input and have the same validation\fix-up code so we can select the strategy without having to do special fix-ups and validation per strategy type.

vargaz · 2023-07-24T18:06:28Z

- if (vtype == typeof(IntPtr) || vtype == typeof(UIntPtr))
+ if (vtype == typeof(IntPtr))
 return CheckValueStatus.Success;
+


Why is this needed ?

Thanks for this. With the last commit, it is no longer necessary. Also removed from core CLR.
It was necessary previously since the Opcodes.UnboxAny would throw a InvalidCastException for UIntPtr arguments.

steveharter · 2023-07-18T17:02:29Z

Add support for Mono to emit faster invoke stubs for both the new invoke APIs added in #88415 as well as standard invoke.

Replacement for closed issue #88952

ghost · 2023-07-18T17:02:41Z

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

Issue Details

[Verifying Tests]

Author:	steveharter
Assignees:	steveharter
Labels:	`area-System.Reflection`
Milestone:	-

EgorBo · 2023-07-18T19:45:28Z

just curious - is it possible to avoid boxed Nullable<> in the first place? I guess it's not possible to create ones with plain C# (where boxed nullables are boxed as just plain values)

steveharter · 2023-07-19T15:10:05Z

just curious - is it possible to avoid boxed Nullable<> in the first place? I guess it's not possible to create ones with plain C# (where boxed nullables are boxed as just plain values)

It's done this way primarily for the interpreted path, which doesn't use emit and is located in the native runtime -- it either needs to be passed a pointer to a nullable or have special logic that creates a nullable from the null \ <T> input. The design we went with keeps the native code (and also emit) to be minimal, so we went with first approach where we pass in a pointer to a nullable which is triggered from the library reflection invoke code. The most straightforward way of doing that is to box a nullable.

Also, for both the interpreted path and the various emit paths we also wanted to normalize the object arg input and have the same validation\fix-up code so we can select the strategy without having to do special fix-ups and validation per strategy type.

vargaz · 2023-07-24T18:06:28Z

- if (vtype == typeof(IntPtr) || vtype == typeof(UIntPtr))
+ if (vtype == typeof(IntPtr))
 return CheckValueStatus.Success;
+


Why is this needed ?

Thanks for this. With the last commit, it is no longer necessary. Also removed from core CLR.
It was necessary previously since the Opcodes.UnboxAny would throw a InvalidCastException for UIntPtr arguments.

steveharter · 2023-07-18T17:02:29Z

Add support for Mono to emit faster invoke stubs for both the new invoke APIs added in #88415 as well as standard invoke.

Replacement for closed issue #88952

ghost · 2023-07-18T17:02:41Z

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

Issue Details

[Verifying Tests]

Author:	steveharter
Assignees:	steveharter
Labels:	`area-System.Reflection`
Milestone:	-

EgorBo · 2023-07-18T19:45:28Z

just curious - is it possible to avoid boxed Nullable<> in the first place? I guess it's not possible to create ones with plain C# (where boxed nullables are boxed as just plain values)

steveharter · 2023-07-19T15:10:05Z

just curious - is it possible to avoid boxed Nullable<> in the first place? I guess it's not possible to create ones with plain C# (where boxed nullables are boxed as just plain values)

It's done this way primarily for the interpreted path, which doesn't use emit and is located in the native runtime -- it either needs to be passed a pointer to a nullable or have special logic that creates a nullable from the null \ <T> input. The design we went with keeps the native code (and also emit) to be minimal, so we went with first approach where we pass in a pointer to a nullable which is triggered from the library reflection invoke code. The most straightforward way of doing that is to box a nullable.

Also, for both the interpreted path and the various emit paths we also wanted to normalize the object arg input and have the same validation\fix-up code so we can select the strategy without having to do special fix-ups and validation per strategy type.

vargaz · 2023-07-24T18:06:28Z

- if (vtype == typeof(IntPtr) || vtype == typeof(UIntPtr))
+ if (vtype == typeof(IntPtr))
 return CheckValueStatus.Success;
+


Why is this needed ?

Thanks for this. With the last commit, it is no longer necessary. Also removed from core CLR.
It was necessary previously since the Opcodes.UnboxAny would throw a InvalidCastException for UIntPtr arguments.

steveharter · 2023-07-18T17:02:29Z

Add support for Mono to emit faster invoke stubs for both the new invoke APIs added in #88415 as well as standard invoke.

Replacement for closed issue #88952

ghost · 2023-07-18T17:02:41Z

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

Issue Details

[Verifying Tests]

Author:	steveharter
Assignees:	steveharter
Labels:	`area-System.Reflection`
Milestone:	-

EgorBo · 2023-07-18T19:45:28Z

just curious - is it possible to avoid boxed Nullable<> in the first place? I guess it's not possible to create ones with plain C# (where boxed nullables are boxed as just plain values)

steveharter · 2023-07-19T15:10:05Z

just curious - is it possible to avoid boxed Nullable<> in the first place? I guess it's not possible to create ones with plain C# (where boxed nullables are boxed as just plain values)

It's done this way primarily for the interpreted path, which doesn't use emit and is located in the native runtime -- it either needs to be passed a pointer to a nullable or have special logic that creates a nullable from the null \ <T> input. The design we went with keeps the native code (and also emit) to be minimal, so we went with first approach where we pass in a pointer to a nullable which is triggered from the library reflection invoke code. The most straightforward way of doing that is to box a nullable.

Also, for both the interpreted path and the various emit paths we also wanted to normalize the object arg input and have the same validation\fix-up code so we can select the strategy without having to do special fix-ups and validation per strategy type.

vargaz · 2023-07-24T18:06:28Z

- if (vtype == typeof(IntPtr) || vtype == typeof(UIntPtr))
+ if (vtype == typeof(IntPtr))
 return CheckValueStatus.Success;
+


Why is this needed ?

Thanks for this. With the last commit, it is no longer necessary. Also removed from core CLR.
It was necessary previously since the Opcodes.UnboxAny would throw a InvalidCastException for UIntPtr arguments.

Unbox a true nullable without Opcodes.Unbox
1f001d5

steveharter added the area-System.Reflection label Jul 18, 2023

steveharter self-assigned this Jul 18, 2023

Normalize UIntPtr to IntPtr for Mono
37f55c3

build-analysisBot mentioned this pull request Jul 20, 2023
Roslyn source generator crash on mono/linux/arm64 #81123
Closed

jkotas reviewed Jul 20, 2023
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs

Use faster unbox
24a6cab

steveharter requested a review from buyaa-n July 24, 2023 15:26

steveharter marked this pull request as ready for review July 24, 2023 15:26

steveharter requested a review from marek-safar as a code owner July 24, 2023 15:26

steveharter mentioned this pull request Jul 24, 2023
Add public MethodInvoker and ConstructorInvoker classes #88415
Merged
4 tasks

marek-safar requested review from lambdageek and vargaz July 24, 2023 15:32

vargaz reviewed Jul 24, 2023
View reviewed changes

Remove unnecessary casts since Unbox is not called
41aa0e7

jkotas approved these changes Jul 25, 2023
View reviewed changes

steveharter commented Jul 25, 2023
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.cs

steveharter changed the title ~~Unbox a true nullable without Opcodes.Unbox~~Enable Mono for using faster invoke stubsJul 25, 2023

steveharter merged commit 2427b67 into dotnet:mainJul 25, 2023

steveharter deleted the NullableEmit branch July 25, 2023 15:46

lewing mentioned this pull request Aug 1, 2023
[Perf] Linux/x64: 6 Improvements on 7/26/2023 12:02:55 AM dotnet/perf-autofiling-issues#20296
Open

ghost locked as resolved and limited conversation to collaborators Aug 24, 2023