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.

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

Use Unsafe.WriteUnaligned in BitConverter.GetBytes #91639

Closed
wants to merge 7 commits into from
Closed

Use Unsafe.WriteUnaligned in BitConverter.GetBytes #91639

wants to merge 7 commits into from

Conversation

xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Sep 5, 2023 •
edited
Loading

sharplab

Added AggressiveInlining to prevent regressions from from failure to inline.

@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 Sep 5, 2023
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 5, 2023
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Sep 5, 2023

Similar changes should also be made in #91299.

@xtqqczze xtqqczze marked this pull request as ready for review September 5, 2023 22:01
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Sep 5, 2023

@MihuBot

@MihuBot MihuBot mentioned this pull request Sep 5, 2023
Open
@MihaZupan MihaZupan added area-System.Numerics and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Sep 5, 2023
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Sep 6, 2023

jit-diffs show regressions due to failure to inline and no improvements.

@stephentoub
Copy link
Member

I appreciate the effort, but what is the concrete benefit of this change? A total of 20 bytes of IL and 0 impact on the resulting asm? If that's all this buys, I don't think it's worth the churn.

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Sep 6, 2023

I appreciate the effort, but what is the concrete benefit of this change? A total of 20 bytes of IL and 0 impact on the resulting asm? If that's all this buys, I don't think it's worth the churn.

The motivation was to avoid depending an implementation alignment assumption, i.e. the array data is 8-byte aligned.

I was hoping the reduced IL size might bring a benefit from improved inlining, but jit-diffs show the opposite, see the description for an example.

@xtqqczze xtqqczze marked this pull request as draft September 6, 2023 08:43
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Sep 6, 2023

@EgorBo I was surprised that the misses inlining a 19-byte method consisting of an allocation and two intrinsics.

@huoyaoyuan
Copy link
Member

i.e. the array data is 8-byte aligned.

#91176 (comment)
There can be some places depending on the fact. Is this clarified in any spec?

@stephentoub stephentoub added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 6, 2023
@MichalPetryka
Copy link
Contributor

Is this clarified in any spec?

AFAIR it's just an implementation detail of CoreCLR.

@jkotas
Copy link
Member

jkotas commented Sep 6, 2023 •
edited
Loading

the array data is 8-byte aligned.

The array data is not 8-byte aligned on 32-bit platforms, and there is a risk of alignment fault on arm. We seem to be lucky that the JIT does not generate code that would hit the alignment fault - #20729 has some discussion about it. It is a fragile assumption. It may be possible to trick the JIT into generating code that would hit the alignment fault on Arm32.

@stephentoub stephentoub removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 6, 2023
@MichalPetryka
Copy link
Contributor

I was hoping the reduced IL size

Does adding a call reduce the size here? ref bytes[0] should be smaller and should perform the same here.

@jkotas
Copy link
Member

jkotas commented Sep 6, 2023 •
edited
Loading

Does adding a call reduce the size here?

It does reduce IL size by one byte. Saving one byte of IL size is not something we care about.

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Sep 6, 2023

@MihuBot

@MihuBot MihuBot mentioned this pull request Sep 6, 2023
Open
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Sep 7, 2023

@MihuBot

@MihuBot MihuBot mentioned this pull request Sep 7, 2023
Open
This was referenced Sep 7, 2023
Closed
Closed
@xtqqczze
Copy link
Contributor Author

@MihuBot

@MihuBot MihuBot mentioned this pull request Sep 10, 2023
Open
@xtqqczze xtqqczze changed the title Reduce IL size for BitConverter.GetBytes Use Unsafe.WriteUnaligned in BitConverter.GetBytes Sep 10, 2023
@build-analysis build-analysis bot mentioned this pull request Sep 10, 2023
Closed
@xtqqczze
Copy link
Contributor Author

The System.Data.Common.ObjectStorage:Set diffs may improve as a result of #91945.

@ghost ghost closed this Oct 12, 2023
@ghost
Copy link

ghost commented Oct 12, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 12, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
area-System.Numerics community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@xtqqczze @stephentoub @huoyaoyuan @MichalPetryka @jkotas @MihaZupan