Skip to content

Use more ReadOnlySpan<byte>s in System.Reflection.MetadataLoadContext #56207

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
jeffhandley merged 4 commits into from
Jul 26, 2021
Merged

Use more ReadOnlySpan<byte>s in System.Reflection.MetadataLoadContext #56207

jeffhandley merged 4 commits into from
Jul 26, 2021

Conversation

@teo-tsirpanis
Copy link
Contributor

@teo-tsirpanis teo-tsirpanis commented Jul 23, 2021

This PR replaces the properties of System.Reflection.TypeLoading.Utf8Constants from byte arrays to ReadOnlySpans of bytes. Thankfully the users of this class can easily adapt to spans.

Furthermore, Helpers.ToUtf16(ReadOnlySpan<byte>) was optimized to not allocate an extra unnecessary array, and the overload that accepts a byte array was remove because it is unused.

@teo-tsirpanis
Use spans instead of byte arrays in MetadataLoadContext.Utf8Constants.
68fda07
@teo-tsirpanis
Optimize Helpers.ToUtf16(ReadOnlySpan<byte>).
5dd31cb 
It avoids a byte array allocation on all frameworks.
And remove an unused overload of it.
@ghost ghost added community-contribution Indicates that the PR has been added by a community member area-System.Reflection and removed community-contribution Indicates that the PR has been added by a community member labels Jul 23, 2021
stephentoub
stephentoub approved these changes Jul 23, 2021
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.

@jeffhandley
Copy link
Member

jeffhandley commented Jul 24, 2021

The System.IO and Quic test failures are unrelated, but there's a failure in System.Reflection.MetadataLoadContext.Tests that looks related to this PR.

https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-56207-merge-eb22edcc4e8d4580af/System.Reflection.MetadataLoadContext.Tests/console.76daa926.log?sv=2019-07-07&se=2021-08-12T19%3A19%3A50Z&sr=c&sp=rl&sig=a6B3xl1DKfDD4we1ObwY5CWbTYvFtz1givkx%2BPkzVVQ%3D

System.Reflection.Tests.CustomAttributeTests.TestMarshalAsPseudoCustomAttribute(fieldName: "F19", expected: MarshalAsAttribute { ArraySubType = 0, IidParameterIndex = 0, MarshalCookie = "", MarshalType = "Blah", MarshalTypeRef = null, ... }) [FAIL]
  System.ArgumentNullException : Array cannot be null.
  Parameter name: bytes
  Stack Trace:
       at System.Text.Encoding.GetString(Byte* bytes, Int32 byteCount)
    /_/src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/General/Helpers.cs(377,0): at System.Reflection.TypeLoading.Helpers.ToUtf16(ReadOnlySpan`1 utf8)
    /_/src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Modules/Ecma/EcmaModule.GetTypeCore.cs(56,0): at System.Reflection.TypeLoading.Ecma.EcmaModule.GetTypeCoreNoCache(ReadOnlySpan`1 ns, ReadOnlySpan`1 name, Exception& e)
    /_/src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Modules/RoModule.cs(112,0): at System.Reflection.TypeLoading.RoModule.GetTypeCore(ReadOnlySpan`1 ns, ReadOnlySpan`1 name, Boolean ignoreCase, Exception& e)
    /_/src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Assemblies/RoAssembly.cs(121,0): at System.Reflection.TypeLoading.RoAssembly.GetTypeCore(ReadOnlySpan`1 ns, ReadOnlySpan`1 name, Boolean ignoreCase, Exception& e)
    /_/src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Assemblies/RoAssembly.cs(118,0): at System.Reflection.TypeLoading.RoAssembly.GetTypeCore(String ns, String name, Boolean ignoreCase, Exception& e)
    /_/src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/General/Helpers.cs(267,0): at System.Reflection.TypeLoading.Helpers.LoadTypeFromAssemblyQualifiedName(String name, RoAssembly defaultAssembly, Boolean ignoreCase, Boolean throwOnError)
    /_/src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/CustomAttributes/Ecma/EcmaCustomAttributeHelpers.cs(241,0): at System.Reflection.TypeLoading.Ecma.EcmaCustomAttributeHelpers.ToMarshalAsAttribute(BlobHandle blobHandle, EcmaModule module)
    /_/src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/CustomAttributes/CustomAttributeHelpers.cs(108,0): at System.Reflection.TypeLoading.CustomAttributeHelpers.<>c__DisplayClass5_0.<TryComputeMarshalAsCustomAttributeData>b__0()
    /_/src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/CustomAttributes/RoPseudoCustomAttributeData.cs(43,0): at System.Reflection.TypeLoading.RoPseudoCustomAttributeData.LazilyComputeArguments()
    /_/src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/CustomAttributes/RoPseudoCustomAttributeData.cs(32,0): at System.Reflection.TypeLoading.RoPseudoCustomAttributeData.get_ConstructorArguments()
    /_/src/libraries/System.Reflection.MetadataLoadContext/tests/src/TestUtils/TestUtils.cs(61,0): at System.Reflection.Tests.TestUtils.ProjectBackToRuntime(CustomAttributeData cad)
    /_/src/libraries/System.Reflection.MetadataLoadContext/tests/src/Tests/CustomAttributes/DllImportTests.cs(58,0): at System.Reflection.Tests.CustomAttributeTests.TestMarshalAsPseudoCustomAttribute(String fieldName, MarshalAsAttribute expected)
System.Reflection.Tests.CustomAttributeTests.TestMarshalAsPseudoCustomAttribute(fieldName: "F20", expected: MarshalAsAttribute { ArraySubType = 0, IidParameterIndex = 0, MarshalCookie = "YumYum", MarshalType = "Blah", MarshalTypeRef = null, ... }) [FAIL]
  System.ArgumentNullException : Array cannot be null.

Can you investigate this, @teo-tsirpanis?

@jeffhandley
Copy link
Member

jeffhandley commented Jul 24, 2021

@buyaa-n This PR is assigned to you for follow-up/decision before the RC1 snap.

@teo-tsirpanis
Copy link
Contributor Author

teo-tsirpanis commented Jul 24, 2021

@jeffhandley fixed it. Looks like Encoding.GetString throws when the pointer is null even when the length is zero.

@jeffhandley jeffhandley merged commit b00a64a into dotnet:main Jul 26, 2021
@jeffhandley
Copy link
Member

jeffhandley commented Jul 26, 2021

Thanks, @teo-tsirpanis!!

@teo-tsirpanis teo-tsirpanis deleted the mlc-readonlyspan branch July 29, 2021 20:03
@ghost ghost locked as resolved and limited conversation to collaborators Aug 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jeffhandley jeffhandley jeffhandley approved these changes

@stephentoub stephentoub stephentoub approved these changes

Assignees

@buyaa-n buyaa-n

Labels

area-System.Reflection

Projects

No open projects
Triage POD for Reflection, META, etc.
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@teo-tsirpanis @jeffhandley @stephentoub @buyaa-n