[NativeAOT] Reflection Invoke refactoring by jkotas · Pull Request #73131 · dotnet/runtime

jkotas · 2022-07-31T20:18:08Z

Refactored reflection Invoke in NativeAOT to be similar to how it is done in CoreCLR.
All argument validation and coercion is done via static code now. This makes the reflection invoke stubs much smaller and the AOT compiler side a lot simpler.
The invoke stub is minimal now and just takes the arguments for the methods to invoke as "Span of byrefs". The two notable differences with CoreCLR are:
- The invoke stub takes the function pointer to call as an argument to allow sharing of the stubs between methods with the same signature. CoreCLR has the thunks non-sharable currently. We have discussed sharing them among methods with the same signature like it is done here.
- The return value is returned as byref. CoreCLR thunk does boxing of the return value as part of the stub. Again, we have discussed to do it this way in CoreCLR too, we just did not have time to do it yet.

- Refactored reflection Invoke in NativeAOT to be similar to how it is done in CoreCLR. - All argument validation and coercion is done via static code now. This makes the reflection invoke stubs much smaller and the AOT compiler side a lot simpler. - The invoke stub is minimal now and just takes the arguments for the methods to invoke as "Span of byrefs". The two notable differences with CoreCLR are: - The invoke stub takes the function pointer to call as an argument to allow sharing of the stubs between methods with the same signature. CoreCLR has the thunks non-sharable currently. We have discussed sharing them among methods with the same signature like it is done here. - The return value is returned as byref. CoreCLR thunk does boxing of the return value as part of the stub. Again, we have discussed to do it this way in CoreCLR too, we just did not have time to do it yet. Fixesdotnet#72548

jkotas · 2022-07-31T22:32:56Z

Some perf numbers

	NativeAOT - current	NativeAOT - this PR	CoreCLR - net6.0	CoreCLR - net7.0
Invoke static method with no arguments	22ns	11ns	34ns	8.2ns
Invoke instance method with int and string arguments	67ns	34ns	96ns	38ns

The extra fixed overhead that is observable in invoke static method with no arguments is due extra "Invoker" layer in native AOT. I am going to delete that layer in subsequent PRs.

jkotas · 2022-08-01T17:26:07Z

cc @steveharter @dotnet/area-system-reflection

jkotas · 2022-08-01T18:05:01Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-08-01T18:05:25Z

Azure Pipelines successfully started running 1 pipeline(s).

jkotas · 2022-08-01T18:36:53Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-08-01T18:37:14Z

Azure Pipelines successfully started running 1 pipeline(s).

jkotas · 2022-08-01T19:46:11Z

@MichalStrehovsky @dotnet/ilc-contrib This is ready for review.

jkotas · 2022-08-01T22:19:46Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-08-01T22:19:59Z

Azure Pipelines successfully started running 1 pipeline(s).

jkotas · 2022-08-02T02:20:18Z

/azp run runtime-extra-platforms

azure-pipelines · 2022-08-02T02:20:47Z

Azure Pipelines successfully started running 1 pipeline(s).

MichalStrehovsky

Very nice!

Co-authored-by: Michal Strehovský <MichalStrehovsky@users.noreply.github.com>

AaronRobinsonMSFT · 2022-08-02T16:30:40Z

@jkotas I think this broke the API compat for AOT.

 LibraryImportGenerator -> ...\runtime\artifacts\bin\LibraryImportGenerator\Release\netstandard2.0\Microsoft.Interop.LibraryImportGenerator.dll
System.Private.CoreLib -> ...\runtime\artifacts\bin\System.Private.CoreLib\ref\Release\net7.0\System.Private.CoreLib.dll
...\.nuget\packages\microsoft.dotnet.apicompat\7.0.0-beta.22372.1\build\Microsoft.DotNet.ApiCompat.targets(95,5): error : Compat issues with assembly System.Private
.CoreLib: [...\runtime\src\coreclr\nativeaot\System.Private.CoreLib\src\System.Private.CoreLib.csproj]
...\.nuget\packages\microsoft.dotnet.apicompat\7.0.0-beta.22372.1\build\Microsoft.DotNet.ApiCompat.targets(95,5): error : TypesMustExist : Type 'System.Reflection.D ynamicInvokeInfo' does not exist in the reference but it does exist in the implementation. [...\runtime\src\coreclr\nativeaot\System.Private.CoreLib\src\System.Private.CoreLib.cs proj]
...\.nuget\packages\microsoft.dotnet.apicompat\7.0.0-beta.22372.1\build\Microsoft.DotNet.ApiCompat.targets(95,5): error : [...\runtime\src\coreclr\nativeaot\System. Private.CoreLib\src\System.Private.CoreLib.csproj]
...\.nuget\packages\microsoft.dotnet.apicompat\7.0.0-beta.22372.1\build\Microsoft.DotNet.ApiCompat.targets(95,5): error : *** Invalid/Unused baseline differences ** * [...\runtime\src\coreclr\nativeaot\System.Private.CoreLib\src\System.Private.CoreLib.csproj]
...\.nuget\packages\microsoft.dotnet.apicompat\7.0.0-beta.22372.1\build\Microsoft.DotNet.ApiCompat.targets(95,5): error : TypesMustExist : Type 'System.InvokeUtils' does not exist in the reference but it does exist in the implementation. [...\runtime\src\coreclr\nativeaot\System.Private.CoreLib\src\System.Private.CoreLib.csproj]
...\.nuget\packages\microsoft.dotnet.apicompat\7.0.0-beta.22372.1\build\Microsoft.DotNet.ApiCompat.targets(95,5): error : TypesMustExist : Type 'System.Reflection.D elegateDynamicInvokeInfo' does not exist in the reference but it does exist in the implementation. [...\runtime\src\coreclr\nativeaot\System.Private.CoreLib\src\System.Private.Co reLib.csproj]
...\.nuget\packages\microsoft.dotnet.apicompat\7.0.0-beta.22372.1\build\Microsoft.DotNet.ApiCompat.targets(112,5): error : MatchingRefApiCompat failed - The referen ce assembly doesn't match all the APIs in the implementation for '...\runtime\artifacts\bin\coreclr\windows.x64.Release\aotsdk\System.Private.CoreLib.dll'. To address either fix errors in the reference assembly (referenced as implementation in compat errors for this reverse compat check), add the issues to the baseline file '...\runtime\src\coreclr\nativ eaot\System.Private.CoreLib\src\MatchingRefApiCompatBaseline.txt' or disable this check by setting RunMatchingRefApiCompat=false in this project. [...\runtime\src\coreclr\nativea ot\System.Private.CoreLib\src\System.Private.CoreLib.csproj]

jkotas · 2022-08-02T16:34:00Z

@akoeplinger It looks like your #72143 was not updated to account for my changes. Could you please fix it up?

akoeplinger · 2022-08-02T17:00:52Z

Yep, I did merge main into my PR branch but looks like it was a few minutes before your PR went it.

Fixed build break caused by merging dotnet#73131 and dotnet#73131 around the same time.

akoeplinger · 2022-08-02T17:10:57Z

Opened a PR with the fix: #73248

Fixed build break caused by merging #73131 and #73131 around the same time.

ghost added the area-NativeAOT-coreclr label Jul 31, 2022

ghost assigned jkotas Jul 31, 2022

jkotas force-pushed the byreflike-reflection branch from 0bef061 to b08f4eeCompare July 31, 2022 20:30

Fix calls on instance methods
5fd6f79

jkotas added 12 commits July 31, 2022 16:53

Fix test failures
65a21f6

Fixed value type instance methods
cf8f6e6

Fix copy back
f6ab25a

Add function pointers issue link
d361e3f

Cleanup
81058c4

Fix allocation of byref boxes
19efbb6

Throw NotSupportedException for byref-like types in invoke signatures
98a5492

Delete comment that is no longer relevant
5a87165

Fix comment
608528c

Byrefs do not support conversions
d897b8e

Copy back on exceptions
da873b1

Fix typo
c48584b

jkotas marked this pull request as ready for review August 1, 2022 17:25

jkotas requested a review from MichalStrehovsky as a code owner August 1, 2022 17:25

Move the call logic to DynamicInvokeInfo
f380100

Workaround to make tests happy
4ca659b

jkotas added 2 commits August 1, 2022 19:14

Fix test
294b34a

Merge branch 'main' into byreflike-reflection
c75cb6d

MichalStrehovsky approved these changes Aug 2, 2022
View reviewed changes

jkotasand others added 4 commits August 1, 2022 22:54

Apply suggestions from code review
4f5035f
Co-authored-by: Michal Strehovský <MichalStrehovsky@users.noreply.github.com>

Fix build
1615da7

Fix formatting
2fff235

Nit
753cc07

jkotas merged commit f348338 into dotnet:mainAug 2, 2022

jkotas deleted the byreflike-reflection branch August 2, 2022 12:54

This was referenced Aug 2, 2022
system.io.compression.tests.zipfile_extract.extracttodirectoryunicode #72537
Closed
CI failure: Assertion `<disabled>' not met in normal-system.xml.xmlserializer.tests #73213
Closed

akoeplinger mentioned this pull request Aug 2, 2022
Update ApiCompat baselines for NativeAOT System.Private.CoreLib #73248
Merged

akoeplinger added a commit to akoeplinger/runtime that referenced this pull request Aug 2, 2022
Update ApiCompat baselines for NativeAOT System.Private.CoreLib
d555a3d
Fixed build break caused by merging dotnet#73131 and dotnet#73131 around the same time.

akoeplinger added a commit that referenced this pull request Aug 2, 2022
Update ApiCompat baselines for NativeAOT System.Private.CoreLib (#73248)
67b23d2
Fixed build break caused by merging #73131 and #73131 around the same time.

MichalStrehovsky mentioned this pull request Aug 3, 2022
Intermittent crash in DynamicGenerics test on OSX x64 #73299
Closed

ghost locked as resolved and limited conversation to collaborators Sep 1, 2022

Uh oh!

Conversation

jkotas commented Jul 31, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas commented Jul 31, 2022

Uh oh!

jkotas commented Aug 1, 2022

Uh oh!

jkotas commented Aug 1, 2022

Uh oh!

azure-pipelinesBot commented Aug 1, 2022

Uh oh!

jkotas commented Aug 1, 2022

Uh oh!

azure-pipelinesBot commented Aug 1, 2022

Uh oh!

jkotas commented Aug 1, 2022

Uh oh!

jkotas commented Aug 1, 2022

Uh oh!

azure-pipelinesBot commented Aug 1, 2022

Uh oh!

jkotas commented Aug 2, 2022

Uh oh!

azure-pipelinesBot commented Aug 2, 2022

Uh oh!

MichalStrehovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AaronRobinsonMSFT commented Aug 2, 2022

Uh oh!

jkotas commented Aug 2, 2022

Uh oh!

akoeplinger commented Aug 2, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akoeplinger commented Aug 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jkotas commented Jul 31, 2022•
edited
Loading

akoeplinger commented Aug 2, 2022•
edited
Loading