KEMBAR78
Use more spans in `System.Reflection.Metadata` et. al. by teo-tsirpanis · Pull Request #76574 · dotnet/runtime · GitHub
Skip to content

Use more spans in System.Reflection.Metadata et. al. #76574

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
merged 16 commits into from
Nov 3, 2022
Merged

Use more spans in System.Reflection.Metadata et. al. #76574

merged 16 commits into from
Nov 3, 2022

Conversation

@teo-tsirpanis
Copy link
Contributor

This PR among other things changes the buffer code of System.Reflection.Metadata so that it uses spans and framework methods that are more optimized, and reduces pinning and unsafely converting between immutable and mutable arrays.

@ghost ghost added area-System.Reflection.Metadata community-contribution Indicates that the PR has been added by a community member labels Oct 3, 2022
@ghost
Copy link

ghost commented Oct 3, 2022

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

Issue Details

This PR among other things changes the buffer code of System.Reflection.Metadata so that it uses spans and framework methods that are more optimized, and reduces pinning and unsafely converting between immutable and mutable arrays.

Author: teo-tsirpanis
Assignees: -
Labels:

area-System.Reflection.Metadata
Milestone: -

@build-analysis build-analysis bot mentioned this pull request Oct 11, 2022
Closed
stephentoub
stephentoub reviewed Oct 25, 2022
View reviewed changes
...braries/System.Reflection.Metadata/src/System/Reflection/Internal/Utilities/BlobUtilities.cs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Nit: You could tidy this (and the others like it) up to just be:

public static void WriteUInt16(this byte[] buffer, int start, ushort value) =>
    Unsafe.WriteUnaligned(ref buffer[start], BitConverter.IsLittleEndian ? BinaryPrimitives.ReverseEndianness(value) : value);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@steveharter
Copy link
Contributor

Are there any remaining CI issues? There were some Mono build failures, but I suspect infrastructure and just re-ran those.

@steveharter
Copy link
Contributor

@teo-tsirpanis can you rebase to the latest? There are these build errors that have already recently been fixed (see this PR):

D:\a\_work\1\s\src\mono\dlls\mscordbi\cordb-value.cpp(351): error C2259: 'CordbArrayValue': cannot instantiate abstract class
2022-10-26T18:05:27.8469947Z   D:\a\_work\1\s\src\mono\dlls\mscordbi\cordb-value.h(170): note: see declaration of 'CordbArrayValue'
2022-10-26T18:05:27.8905158Z   D:\a\_work\1\s\src\mono\dlls\mscordbi\cordb-value.cpp(351): note: due to following members:

@teo-tsirpanis
Use Unsafe.As in ImmutableByteArrayInterop.
f6b854b 
Code quality increases.
@teo-tsirpanis
Optimize methods in the BlobUtilities class.
417f538
@teo-tsirpanis
Include more information in exceptions thrown from catch blocks.
ce584cd
@teo-tsirpanis
Use spans in BlobContentId, avoiding ImmutableByteArrayInterop.
44e38dc
@teo-tsirpanis
Use spans in BlobWriter, reducing pinning and ImmutableArrayInterop.
a2acd50
@teo-tsirpanis
Use spans in BlobBuilder, reducing pinning and ImmutableArrayInterop.
daed179
@teo-tsirpanis
Remove NoInlining from the throw helpers.
47350a7 
It prevents the JIT from looking into the method's body, realizing it's a throw helper, and doing what's best (such as considering it cold).
@teo-tsirpanis
Inline BlobUtilities.Read(Immutable)?Bytes.
9a56ab1
@teo-tsirpanis
Avoid a length check introduced by the use of BinaryPrimitives.
8ebbbfa
@teo-tsirpanis
Remove DisallowNull from `ImmutableArrayInterop.DangerousCreateFrom…
85bc2d6 
…UnderlyingArray`.
@teo-tsirpanis
Remove an early bail-out if the buffer was empty.
de0c6f2
@teo-tsirpanis
Stop using value tuples.
5001e7f
@teo-tsirpanis
Fix tests and expose writing a span to BlobWriter.
94b72c1
@teo-tsirpanis
Copy link
Contributor Author

Done @steveharter.

@build-analysis build-analysis bot mentioned this pull request Oct 27, 2022
Closed
2 tasks
stephentoub
stephentoub approved these changes Nov 3, 2022
View reviewed changes
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 b8a0906 into dotnet:main Nov 3, 2022
@teo-tsirpanis teo-tsirpanis deleted the srm-opt branch November 3, 2022 15:49
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoub stephentoub stephentoub approved these changes

@GrabYourPitchforks GrabYourPitchforks Awaiting requested review from GrabYourPitchforks

+1 more reviewer

@steveharter steveharter steveharter left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Reflection.Metadata 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.

3 participants

@teo-tsirpanis @steveharter @stephentoub