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

Use helpers in Guid's ROS<byte> constructor #78446

Merged
merged 5 commits into from
Nov 21, 2022

Conversation

DaZombieKiller
Copy link
Contributor

Closes #78440. (SharpLab demo)
Moves the big endian path and exception throwing to a helper, resulting in improved codegen when the constructor is inlined.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 16, 2022
@ghost
Copy link

ghost commented Nov 16, 2022

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

Issue Details

Closes #78440. (SharpLab demo)
Moves the big endian path and exception throwing to a helper, resulting in improved codegen when the constructor is inlined.

Author: DaZombieKiller
Assignees: -
Labels:

area-System.Runtime, community-contribution

Milestone: -

@EgorBo
Copy link
Member

EgorBo commented Nov 16, 2022

Technically, the BigEndian path doesn't need a helper, it's trimmed out anyway. It's the throw helper that messes inlining up:

*************** In fgFindBasicBlocks() for System.Guid:.ctor(System.ReadOnlySpan`1[ubyte]):this
weight= 77 : state  16 [ ldarga.s ]
weight= 79 : state  40 [ call ]
weight= 41 : state  32 [ ldc.i4.s ]
weight=  6 : state  46 [ beq.s ]
weight= 79 : state  40 [ call ]
weight= 66 : state 102 [ ldstr ]
weight= 79 : state  40 [ call ]
weight= 66 : state 102 [ ldstr ]
weight=227 : state 103 [ newobj ]
weight=210 : state 108 [ throw ]
weight=159 : state 112 [ ldsfld ]
weight=-24 : state  39 [ pop ]
weight= 10 : state   3 [ ldarg.0 ]
weight= 16 : state   4 [ ldarg.1 ]
weight= 79 : state  40 [ call ]
weight= 36 : state 115 [ stobj ]
weight= 19 : state  42 [ ret ]

multiplier in instance constructors increased to 1.5.
multiplier in methods of struct increased to 4.5.
1 arguments are structs passed by value.  Multiplier increased to 6.5.
Inline candidate has an arg that feeds a constant test.  Multiplier increased to 7.5.
Inline candidate callsite is boring.  Multiplier increased to 8.8.
calleeNativeSizeEstimate=1225
callsiteNativeSizeEstimate=135
benefit multiplier=8.8
threshold=1188
Native estimate for function size exceeds threshold for inlining 122.5 > 118.8 (multiplier = 8.8)

@tannergooding
Copy link
Member

Technically, the BigEndian path doesn't need a helper, it's trimmed out anyway.

Is it properly trimmed out? The linker has a bad habit of leaving comparisons and branches that do nothing and that likely negatively impacts some analysis and opts we can do

@EgorBo
Copy link
Member

EgorBo commented Nov 16, 2022

Technically, the BigEndian path doesn't need a helper, it's trimmed out anyway.

Is it properly trimmed out? The linker has a bad habit of leaving comparisons and branches that do nothing and that likely negatively impacts some analysis and opts we can do

I've attached the IL ^, looks pretty normal to me 🤷
In fact, it's almost inlined as is

@stephentoub
Copy link
Member

Thanks. I just read @EgorBo's comment. Given that, I think we should just outline the throw and keep the rest the same. Otherwise, we're possibly regressing Guid's ctor on big endian for little reason.

@DaZombieKiller
Copy link
Contributor Author

That makes sense. When I added the ReadGuidLittleEndian helper I was in the "portable assembly" mindset and forgot that the runtime is always trimmed, rendering the helper redundant. I've removed it in 65763ac.

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks.

@stephentoub stephentoub merged commit 9129083 into dotnet:main Nov 21, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Guid(ReadOnlySpan<Byte>) could be optimized
8 participants