Skip to content

For perf, remove Invoker pattern for fields #74614

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
steveharter merged 3 commits into from
Aug 27, 2022
Merged

For perf, remove Invoker pattern for fields #74614

steveharter merged 3 commits into from
Aug 27, 2022

Conversation

@steveharter
Copy link
Contributor

@steveharter steveharter commented Aug 25, 2022 •
edited
Loading

Fixes #74550 by removing overhead added by the Invoker pattern which is unnecessary until IL emit support is added for fields.

Brings field getter to even with 6.0, and setter about 10% faster.

Suggest porting this to 7.0

For 8.0, this change will eventually be reverted and the corresponding IL emitted for field access.

Before:

|                                               Method |        Job |                                                                                                   Toolchain |       Mean |     Error |     StdDev |     Median |        Min |        Max | Ratio | RatioSD |  Gen 0 | Allocated | Alloc Ratio |
|----------------------------------------------------- |----------- |------------------------------------------------------------------------------------------------------------ |-----------:|----------:|-----------:|-----------:|-----------:|-----------:|------:|--------:|-------:|----------:|------------:|
|                                        Field_Get_int | Job-BQDNCA | \runtime60\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.9\corerun.exe |  30.993 ns | 0.1622 ns |  0.1438 ns |  30.972 ns |  30.768 ns |  31.259 ns |  0.87 |    0.01 | 0.0022 |      24 B |        1.00 |
|                                        Field_Get_int | Job-RQCXYM |   \runtime\artifacts\bin\testhost\net7.0-windows-Release-x64\shared\Microsoft.NETCore.App\7.0.0\corerun.exe |  35.749 ns | 0.2806 ns |  0.2487 ns |  35.752 ns |  35.461 ns |  36.344 ns |  1.00 |    0.00 | 0.0021 |      24 B |        1.00 |
|                                                      |            |                                                                                                             |            |           |            |            |            |            |       |         |        |           |             |
|                                        Field_Set_int | Job-BQDNCA | \runtime60\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.9\corerun.exe |  36.037 ns | 0.2146 ns |  0.1903 ns |  36.018 ns |  35.744 ns |  36.418 ns |  0.98 |    0.01 | 0.0023 |      24 B |        1.00 |
|                                        Field_Set_int | Job-RQCXYM |   \runtime\artifacts\bin\testhost\net7.0-windows-Release-x64\shared\Microsoft.NETCore.App\7.0.0\corerun.exe |  36.766 ns | 0.2736 ns |  0.2425 ns |  36.666 ns |  36.378 ns |  37.268 ns |  1.00 |    0.00 | 0.0023 |      24 B |        1.00 |

After

|                                               Method |        Job |                                                                                                   Toolchain |       Mean |     Error |    StdDev |     Median |        Min |        Max | Ratio | RatioSD |  Gen 0 | Allocated | Alloc Ratio |
|----------------------------------------------------- |----------- |------------------------------------------------------------------------------------------------------------ |-----------:|----------:|----------:|-----------:|-----------:|-----------:|------:|--------:|-------:|----------:|------------:|
|                                        Field_Get_int | Job-ELBTUN | \runtime60\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.9\corerun.exe |  34.137 ns | 0.5458 ns | 0.4839 ns |  34.074 ns |  33.391 ns |  35.035 ns |  1.00 |    0.01 | 0.0022 |      24 B |        1.00 |
|                                        Field_Get_int | Job-XYBTNI |   \runtime\artifacts\bin\testhost\net7.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\corerun.exe |  34.164 ns | 0.0963 ns | 0.0854 ns |  34.169 ns |  33.972 ns |  34.275 ns |  1.00 |    0.00 | 0.0022 |      24 B |        1.00 |
|                                                      |            |                                                                                                             |            |           |           |            |            |            |       |         |        |           |             |
|                                        Field_Set_int | Job-ELBTUN | \runtime60\artifacts\bin\testhost\net6.0-windows-Release-x64\shared\Microsoft.NETCore.App\6.0.9\corerun.exe |  37.470 ns | 0.9993 ns | 1.1107 ns |  36.990 ns |  36.026 ns |  40.156 ns |  1.13 |    0.04 | 0.0022 |      24 B |        1.00 |
|                                        Field_Set_int | Job-XYBTNI |   \runtime\artifacts\bin\testhost\net7.0-windows-Release-x64\shared\Microsoft.NETCore.App\8.0.0\corerun.exe |  33.512 ns | 0.1265 ns | 0.1056 ns |  33.506 ns |  33.369 ns |  33.780 ns |  1.00 |    0.00 | 0.0022 |      24 B |        1.00 |

@steveharter steveharter self-assigned this Aug 25, 2022
@ghost
Copy link

ghost commented Aug 25, 2022

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

Issue Details

null

Author: steveharter
Assignees: steveharter
Labels:

area-System.Reflection
Milestone: -

@steveharter steveharter requested a review from buyaa-n August 26, 2022 18:40
@steveharter steveharter marked this pull request as ready for review August 26, 2022 18:40
buyaa-n
buyaa-n approved these changes Aug 26, 2022
View reviewed changes
Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

LGTM

@steveharter steveharter merged commit 13d8d2e into dotnet:main Aug 27, 2022
@steveharter steveharter deleted the FieldPerf branch August 27, 2022 00:59
@steveharter
Copy link
Contributor Author

steveharter commented Aug 27, 2022

/backport to release/7.0

@github-actions
Copy link
Contributor

github-actions bot commented Aug 27, 2022

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/2937474963

@github-actions github-actions bot mentioned this pull request Aug 27, 2022
Merged
@danmoseley
Copy link
Member

danmoseley commented Aug 28, 2022

@steveharter Do we have appropriate coverage in the perf repo (I saw this was customer reported)

@steveharter
Copy link
Contributor Author

steveharter commented Aug 29, 2022

@steveharter Do we have appropriate coverage in the perf repo (I saw this was customer reported)

Yes the existing (but new) benchmarks in the perf repo cover this. One item to consider however is that they are currently only based on an int field and thus could be expanded to reference types.

@AndyAyersMS AndyAyersMS mentioned this pull request Sep 1, 2022
Closed
@steveharter steveharter mentioned this pull request Sep 7, 2022
Merged
steveharter added a commit to steveharter/runtime that referenced this pull request Sep 19, 2022
@steveharter
Revert "For perf, remove Invoker pattern for fields (dotnet#74614)"
771c3f0 
This reverts commit 13d8d2e.
@ghost ghost locked as resolved and limited conversation to collaborators Sep 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotas jkotas jkotas approved these changes

@marek-safar marek-safar Awaiting requested review from marek-safar

+1 more reviewer

@buyaa-n buyaa-n buyaa-n approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@steveharter steveharter

Labels

area-System.Reflection

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

.NET 6 vs .NET 7 reflection performance regression

4 participants

@steveharter @danmoseley @jkotas @buyaa-n