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

Add MemoryMarshal.GetArrayDataReference(Array) #53562

Merged
merged 3 commits into from
Jun 3, 2021

Conversation

GrabYourPitchforks
Copy link
Member

@GrabYourPitchforks GrabYourPitchforks commented Jun 1, 2021

Resolves #35528.

This PR has a few distinct components:

  • Introduce the overload Array.GetArrayDataReference(Array array). I opted to name the parameter array instead of arr for local consistency with the existing overload.
  • Remove the existing GetRawSzArrayData and GetRawArrayData internal methods and migrate their callers to the new public APIs.
  • In coreclr, define the metasigs for both overloads so that we can intrinsicify the new API in the future without creating ambiguity within the existing jitinterface.cpp helpers.
  • In mono, intrinsicify and add interp support for both overloads. (Unlike coreclr, mono uses a consistent array layout across both szarray and mdarray, which made this possible.)
  • Shore up the unit tests to cover both overloads.

/cc @buyaa-n, I thought this might interest you because it touches metasig.h, which you and I discussed a few weeks back. In this case, I'm introducing two signatures to metasig.h: one which takes a T[] and returns a ref T, and one which takes an Array and returns a ref byte. Note the use of DEFINE_METASIG_T instead of DEFINE_METASIG for the Array-consuming method. This is because we use the syntax C(ARRAY), which specifically refers to the System.Array class, and per comments at the very top of file, you need to use the _T version of the macro if you want to refer to specific classes. These new definitions are then used in corelib.h.

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Jun 1, 2021

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

Issue Details

Resolves #35528.

This PR has a few distinct components:

  • Introduce the overload Array.GetArrayDataReference(Array array). I opted to name the parameter array instead of arr for local consistency with the existing overload.
  • Remove the existing GetRawSzArrayData and GetRawArrayData internal methods and migrate their callers to the new public APIs.
  • In coreclr, define the metasigs for both overloads so that we can intrinsicify the new API in the future without creating ambiguity within the existing jitinterface.cpp helpers.
  • In mono, intrinsicify and add interp support for both overloads. (Unlike coreclr, mono uses a consistent array layout across both szarray and mdarray, which made this possible.)
  • Shore up the unit tests to cover both overloads.

/cc @buyaa-n, I thought this might interest you because it touches metasig.h, which you and I discussed a few weeks back. In this case, I'm introducing two signatures to metasig.h: one which takes a T[] and returns a ref T, and one which takes an Array and returns a ref byte. Note the use of DEFINE_METASIG_T instead of DEFINE_METASIG for the Array-consuming method. This is because we use the syntax C(ARRAY), which specifically refers to the System.Array class, and per comments at the very top of file, you need to use the __T_ macro if you want to refer to specific classes. These new macros are then used in corelib.h.

Author: GrabYourPitchforks
Assignees: -
Labels:

area-System.Runtime

Milestone: 6.0.0

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.

CoreCLR part LGTM. Thanks!

@vargaz
Copy link
Contributor

vargaz commented Jun 2, 2021

The mono changes look ok.

@GrabYourPitchforks
Copy link
Member Author

@jkotas I thought I saw feedback from you earlier that I should add [NonVersionable] annotations, but looks like the feedback was deleted? Do you still want those annotations added, or did I imagine this?

@jkotas
Copy link
Member

jkotas commented Jun 2, 2021

@jkotas I thought I saw feedback from you earlier that I should add [NonVersionable] annotations, but looks like the feedback was deleted?

Sorry for the confusion. I have deleted that feedback. GetArrayDataReference(Array array) cannot not be marked as [NonVersionable] since it it depends on MethodTable fields that are not guaranteed to be stable.

src/mono/mono/mini/intrinsics.c Outdated Show resolved Hide resolved
src/mono/mono/mini/intrinsics.c Outdated Show resolved Hide resolved
@GrabYourPitchforks
Copy link
Member Author

Latest iteration:

  • Address Marek's PR feedback
  • Improve unit test coverage by exercising tests for arrays of varying rank

@GrabYourPitchforks GrabYourPitchforks merged commit 3e1ddb0 into dotnet:main Jun 3, 2021
@GrabYourPitchforks GrabYourPitchforks deleted the arraydataref branch June 3, 2021 07:46
@ghost ghost locked as resolved and limited conversation to collaborators Jul 3, 2021
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.

Proposal: MemoryMarshal.GetArrayDataReference<T>(T[,]) overload
5 participants