Skip to content
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.

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

Fix regression from RuntimeType.AllocateValueType rewrite #101137

Merged
merged 4 commits into from
Jun 5, 2024

Conversation

jkoritzinsky
Copy link
Member

Testing out fixes for #101134 and #104777

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 16, 2024
@jkoritzinsky
Copy link
Member Author

/benchmark help

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

@jkoritzinsky
Copy link
Member Author

/benchmark micro aspnet-perf-win runtime --variable filter=System.Reflection.Invoke.StaticMethod

Copy link

pr-benchmarks bot commented Apr 16, 2024

Benchmark started for micro on aspnet-perf-win with runtime and arguments --variable filter=*System.Reflection.Invoke.StaticMethod*. Logs: link

Copy link

pr-benchmarks bot commented Apr 16, 2024

micro - aspnet-perf-win

| benchmark                                                 | mean (micro.base) | mean (micro.pr) | ratio | allocated (micro.base) | allocated (micro.pr) | ratio |
| --------------------------------------------------------- | ----------------- | --------------- | ----- | ---------------------- | -------------------- | ----- |
| StaticMethod4_arrayNotCached_int_string_struct_class      |          31.80 ns |        30.08 ns |  0.95 |                  104 B |                104 B |  1.00 |
| StaticMethod4_ByRefParams_int_string_struct_class         |          138.6 ns |        137.7 ns |  0.99 |                   48 B |                 48 B |  1.00 |
| StaticMethod4_int_string_struct_class                     |          19.52 ns |        19.80 ns |  1.01 |                    0 B |                  0 B |       |
| StaticMethod5_arrayNotCached_int_string_struct_class_bool |          45.05 ns |        43.55 ns |  0.97 |                  136 B |                136 B |  1.00 |
| StaticMethod5_ByRefParams_int_string_struct_class_bool    |          197.7 ns |        206.7 ns |  1.05 |                   72 B |                 72 B |  1.00 |

@jkoritzinsky
Copy link
Member Author

Looks like this helps, but not enough.

@jkoritzinsky
Copy link
Member Author

/benchmark micro aspnet-perf-win runtime --variable filter=System.Reflection.Invoke.StaticMethod

Copy link

pr-benchmarks bot commented Apr 16, 2024

Benchmark started for micro on aspnet-perf-win with runtime and arguments --variable filter=*System.Reflection.Invoke.StaticMethod*. Logs: link

Copy link

pr-benchmarks bot commented Apr 16, 2024

micro - aspnet-perf-win

| benchmark                                                 | mean (micro.base) | mean (micro.pr) | ratio | allocated (micro.base) | allocated (micro.pr) | ratio |
| --------------------------------------------------------- | ----------------- | --------------- | ----- | ---------------------- | -------------------- | ----- |
| StaticMethod4_arrayNotCached_int_string_struct_class      |          29.41 ns |        30.07 ns |  1.02 |                  104 B |                104 B |  1.00 |
| StaticMethod4_ByRefParams_int_string_struct_class         |         142.15 ns |        87.21 ns |  0.61 |                   48 B |                 48 B |  1.00 |
| StaticMethod4_int_string_struct_class                     |          20.62 ns |        19.83 ns |  0.96 |                    0 B |                  0 B |       |
| StaticMethod5_arrayNotCached_int_string_struct_class_bool |          42.85 ns |        43.83 ns |  1.02 |                  136 B |                136 B |  1.00 |
| StaticMethod5_ByRefParams_int_string_struct_class_bool    |          192.8 ns |        120.2 ns |  0.62 |                   72 B |                 72 B |  1.00 |

Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

@@ -104,6 +105,7 @@ void InitJITHelpers1()
SetJitHelperFunction(CORINFO_HELP_NEWARR_1_OBJ, JIT_NewArr1OBJ_UP);

ECall::DynamicallyAssignFCallImpl(GetEEFuncEntryPoint(AllocateStringFastUP), ECall::FastAllocateString);
ECall::DynamicallyAssignFCallImpl(GetEEFuncEntryPoint(JIT_BoxFastUP), ECall::NonNullableBox);
Copy link
Member

Choose a reason for hiding this comment

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

This block is Windows specific, so this change only helps on Windows. We need an equivalent fix for non-Windows platforms too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I'll probably revert this particular change and make an equivalent one that's more xplat-friendly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've inlined the trail allocation logic into JIT_Box and factored out the rest of the logic to be in a separate framed helper instead of using the dynamic FCALL impl logic.

Let me know if this looks good to you.

@jkoritzinsky
Copy link
Member Author

/benchmark micro aspnet-perf-win runtime --variable filter=System.Reflection.Invoke.StaticMethod

@jkoritzinsky jkoritzinsky marked this pull request as ready for review June 4, 2024 18:34
@jkoritzinsky jkoritzinsky requested a review from jkotas June 4, 2024 18:34
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks!

@jkoritzinsky
Copy link
Member Author

/ba-g timeouts unrelated

@jkoritzinsky jkoritzinsky merged commit 1753956 into dotnet:main Jun 5, 2024
134 of 152 checks passed
@jkoritzinsky jkoritzinsky deleted the allocate-vt-regression branch June 5, 2024 01:00
@github-actions github-actions bot locked and limited conversation to collaborators Jul 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants