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

API proposal: MemoryMarshal.GetRawArrayData #29003

Closed
GrabYourPitchforks opened this issue Mar 18, 2019 · 41 comments
Closed

API proposal: MemoryMarshal.GetRawArrayData #29003

GrabYourPitchforks opened this issue Mar 18, 2019 · 41 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices
Milestone

Comments

@GrabYourPitchforks
Copy link
Member

(Related to https://github.com/dotnet/corefx/issues/33706, but more narrowly scoped.)

Proposal

namespace System.Runtime.InteropServices
{
    public static class MemoryMarshal
    {
        // new method on existing MemoryMarshal type
        public static ref T GetRawArrayRef<T>(T[] array);
    }
}

Motivation

Per the feedback at https://github.com/dotnet/corefx/issues/33706#issuecomment-442145588, third-party libraries may want to perform certain optimizations that today can only be performed within coreclr itself because we know the internal layout of SzArray instances.

Behavior

If array is null, this results in a standard NullReferenceException (see https://github.com/dotnet/corefx/issues/36133#issuecomment-474389127). Otherwise it returns a reference to the element at index 0 in the array, even if the array is empty.

Per ECMA-335 (PDF), § II.14.4.2, it is legal for a managed pointer (type T&) to point just past the end of an array to where the next value would have been stored. Such T& cannot be safely dereferenced, but it is guaranteed to be tracked properly by the GC, and it can be used as an input argument in binary operators like add or clt.

Because it is possible to get a real GC-tracked managed pointer (T&), but because we cannot guarantee that it's safe to dereference, the GetRawArrayData method belongs on MemoryMarshal or some other "unsafe" type. The Unsafe static class is also a candidate for where this API can live, but it does not have any existing API surface for dealing with T[] inputs.

Developers who want a T& that's always safe to dereference can continue to use the standard syntax for getting a reference to the first array element:

ref T myRef = ref theArray[0]; // runtime will bounds check for you
@GrabYourPitchforks GrabYourPitchforks changed the title API proposal: Marshal.GetArrayRawData API proposal: Marshal.GetRawArrayData Mar 18, 2019
@jeffschwMSFT
Copy link
Member

@jkotas
Copy link
Member

jkotas commented Mar 19, 2019

Why not RuntimeHelpers ? This does not have much to do with interop marshaling. Also, it is pretty specialized API.

@GrabYourPitchforks
Copy link
Member Author

@jkotas - RuntimeHelpers isn't something we've historically considered "unsafe" in the sense that its static methods allow you to break type safety / memory safety. The existing RuntimeHelpers method that comes closest is GetUninitializedObject, but that method doesn't feel quite as dangerous as this one.

If we wanted to put this on RuntimeHelpers then I'd advocate prefixing it with Dangerous to make explicit that it provides no safety guarantees.

@GrabYourPitchforks
Copy link
Member Author

(On the other hand, if we had a sister method GetRawStringData(string s) : ref readonly char, I don't really have a strong preference where it goes or what it's named since it's always 100% safely dereferenceable, since even for empty strings it'll just point to the null terminator.)

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Mar 19, 2019

Otherwise it returns a reference to the element at index 0 in the array, even if the array is empty.

What are the next steps to access subsequent elements once I have this ref to the element? In C++ I would use pointers and do something like:

int *p = ...;
int v = *p;
p++;

I don't fully understand the use case here for arrays, especially if users can do ref T myRef = ref theArray[i].

@jkotas
Copy link
Member

jkotas commented Mar 19, 2019

Marshal is very main stream type. If you add a new API there, everybody will see it. I do not think we want this API in a place where everybody will it.

What about MemoryMarshal ? It has AsRef that is about as dangerous as this one.

@GrabYourPitchforks
Copy link
Member Author

@AaronRobinsonMSFT The point is mainly to elide bounds checks in hot code paths. See below:

ref T theRef = ref ThisFunction(theArray); // theRef now points to element 0 (NOT bounds checked)
theRef = ref Unsafe.Add(ref theRef, 1); // theRef now points to element 1
// ...

@jkotas MemoryMarshal is fine by me. I didn't propose it there originally since MemoryMarshal seems geared more toward Span / Memory rather than SzArray, but I wouldn't push back against it living there.

@AaronRobinsonMSFT
Copy link
Member

@GrabYourPitchforks But wouldn't the caller still want to know how many elements exist in order to properly call Add()? Or perhaps you mean the implicit bounds check from the runtime can be avoided? If it is the latter I get this API and agree it probably should be on MemoryMarshal.

@DaZombieKiller
Copy link
Contributor

DaZombieKiller commented Mar 19, 2019

So essentially this would be a JIT intrinsic form of something like this?:

/// <remarks>If <paramref name="array"/> is null, <see cref="ArgumentNullException"/> is thrown.</remarks>
/// <returns>A reference to the element at index 0 in the array, even if the array is empty.</returns>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static ref T GetRawArrayData<T>(T[] array)
{
    if (array == null)
        throw new ArgumentNullException(nameof(array));

    return ref MemoryMarshal.GetReference(new Span<T>(array));
}

I think having the function on Marshal would feel strange. As I understand it, Marshal is primarily intended for, well, marshalling data to and from native code. I agree that MemoryMarshal would make much more sense.

EDIT: Come to think of it, couldn't this function technically be implemented as an overload for MemoryMarshal.GetReference? Passing a T[] to it currently is ambiguous due to the presence of both Span<T> and ReadOnlySpan<T> overloads, so it shouldn't break any existing code.

@jkotas
Copy link
Member

jkotas commented Mar 19, 2019

  if (array == null)
        throw new ArgumentNullException(nameof(array));

This is very low level method and throwing ArgumentNullException from it would make it much more expensive than it should be. This should throw the implicit NullReferenceException. (This would not be the first case in the framework to do this.)

@omariom
Copy link
Contributor

omariom commented Mar 19, 2019

Why not?

public static class MemoryMarshal
{
    public static ref T GetReference<T>(T[] array);
}

There is already such method for span.

@GrabYourPitchforks
Copy link
Member Author

This should throw the implicit NullReferenceException.

I'll update the proposal.

@GrabYourPitchforks GrabYourPitchforks changed the title API proposal: Marshal.GetRawArrayData API proposal: MemoryMarshal.GetRawArrayData Mar 25, 2019
@GrabYourPitchforks
Copy link
Member Author

I updated the description to put the API on MemoryMarshal instead of Marshal since that's where it looked like consensus was heading.

@terrajobst
Copy link
Contributor

Why do we need this API at all? It seems the existing APIs would cover this, no?

namespace System.Runtime.InteropServices
{
    public static class MemoryMarshal
    {
        public static ref T GetReference<T>(Span<T> span);
        public static ref T GetReference<T>(ReadOnlySpan<T> span);
    }
}

Like this:

T[] someArray = GetData();
ref T data = MemoryMarshal.GetReference(someArray);

What would GetRawArrayData() do differently?

@GrabYourPitchforks
Copy link
Member Author

One potential pattern that would fail by bouncing through Span<T>:

object[] arr = new string[] { "Hello", "world!" };
ref object o = ref MemoryMarshal.GetReference(arr);

The implicit conversion from object[] to Span<object> in order to call the existing GetReference<object>(Span<object>) overload would fail at runtime due to the check at https://github.com/dotnet/coreclr/blob/7f1dd83ddb159e2469b85145f228fa60ab3682e2/src/System.Private.CoreLib/shared/System/Span.Fast.cs#L52-L53

Perhaps we'd want this scenario to fail, but wanted to throw it out there.

One other behavioral difference is in null handling. The implicit conversion from T[] to Span<T> converts null arrays to null spans. The new API being considered here fails on null inputs because it's unable to answer the question "What's the address of where the first element is located or would be located?"

@GrabYourPitchforks
Copy link
Member Author

@MichalStrehovsky We were having trouble in API review justifying this new API given the existence of the methods Immo called out. Can you help clarify some of the scenarios?

@jkotas
Copy link
Member

jkotas commented Jul 17, 2019

This is low-level API to enable micro-optimizations in performance sensitive code. You are right that one can get the same functionality by combining existing APIs, but not with the same performance characteristics.

This API is equivalent of MemoryMarshal.GetReference(Span<T> span) for arrays. MemoryMarshal.GetReference was not required API for functionality either (you can get same functionality by combining existing APIs). It was required for performance.

@mjsabby
Copy link
Contributor

mjsabby commented Jul 17, 2019

I think we can expose two APIs instead, which actually will be beneficial for Frozen Objects anyway. RuntimeHelpers.GetObjectSize with something like RuntimeHelpers.EmptyArrayObjectSize, or RuntimeHelpers.GetNumberOfComponents which returns 0 for empty array/string and for objects that aren't either of those.

@jkotas
Copy link
Member

jkotas commented Jul 17, 2019

I do not think we want to make the super low-level APIs like these public.

@mjsabby
Copy link
Contributor

mjsabby commented Jul 17, 2019

Do you consider GetObjectSize super low-level or the NumberOfComponents + EmptyArrayObjectSize?

@jkotas
Copy link
Member

jkotas commented Jul 17, 2019

Anything that talks about managed object size super low-level. It implies what the layout of managed objects and makes changing it hard or impossible.

RuntimeHelpers.OffsetToStringData is one place where similar super low-level detail got exposed, and in turn prevented us from experimenting (easily) with alternative string layouts.

It should be a non-goal for your frozen object experiment to run against public APIs.

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky We were having trouble in API review justifying this new API given the existence of the methods Immo called out. Can you help clarify some of the scenarios?

My scenarios mostly revolved around non-variable-size objects in the version of this API that is not limited to arrays (#33706).

@GrabYourPitchforks
Copy link
Member Author

GrabYourPitchforks commented Jul 30, 2019

Reactivating based on Jan's comment at https://github.com/dotnet/corefx/issues/36133#issuecomment-512053677. Thank you for the context!

Edit: For reference, here are all the locations where the runtime calls into the current (non-public) implementation of this method. It's used primarily as a perf optimization.

@benaadams
Copy link
Member

Where and what would the equivalent string version be? e.g.

ref char Getxxx(string str)

And would they be in same class and overloads of each other

@terrajobst
Copy link
Contributor

terrajobst commented Dec 17, 2019

Video

We considered this:

namespace System.Runtime.CompilerServices
{
    public static class Unsafe
    {
        public static ref T GetRawArrayRef<T>(ref T source);
    }
}

but we'd prefer this:

namespace System.Runtime.InteropServices
{
    public static class MemoryMarshal
    {
        public static ref T GetReference<T>(T[] array);
    }
}

(Please not that's OK because the existing GetReference<T>() method don't bind for arrays due to an ambiguous match, so the caller had to add a cast.)

@GrabYourPitchforks GrabYourPitchforks self-assigned this Dec 17, 2019
@jkotas
Copy link
Member

jkotas commented Dec 17, 2019

Where and what would the equivalent string version be?

There is ref char string.GetPinnableReference() already.

@GrabYourPitchforks
Copy link
Member Author

GrabYourPitchforks commented Dec 18, 2019

@benaadams - Here's a more detailed response on the difference in these APIs, in case you were curious.

As @jkotas points out, string.GetPinnableReference already exists today. You'll notice that there's a bit of a behavioral difference between all of the various GetReference / GetPinnableReference methods, and this stems from the fact that we intended <instance>.GetPinnableReference to always be "safe".

For example, if you have a zero-length string, the method string.GetPinnableReference will return a ref readonly char pointing to the null terminator. It's perfectly safe to dereference such a thing; its behavior is always well-defined.

string theString = string.Empty;
ref readonly char theCharRef = ref theString.GetPinnableReference(); // returns a reference to the terminating null
char c = theCharRef; // c = '\0' once this line runs

Contrast this to ReadOnlySpan<char>, which is not guaranteed to have meaningful data (like a null terminator) after the buffer, since it isn't guaranteed to point to a full string. So the Span<T>/ReadOnlySpan<T>.GetPinnableReference method special-cases empty inputs and returns a null reference. This is "legal" in the sense that the runtime can handle this situation appropriately, and attempting to dereference such a thing will result in a standard, catchable NRE instead of a potential AV.

ReadOnlySpan<char> theSpan = GetString();
ref readonly char theCharRef = ref theSpan.GetPinnableReference(); // might return a null ref, which is ok
char c = theCharRef; // might NRE

In both of the above cases, <instance>.GetPinnableReference is 100% safe.

That said, there's an unsafe equivalent method MemoryMarshal.GetReference, whose behavior is essentially <instance>.GetPinnableReference without the bounds checking.

ReadOnlySpan<char> theSpan = GetString();
ref readonly char theCharRef = ref MemoryMarshal.GetReference(theSpan);
if (!theSpan.IsEmpty)
{
    char c = theCharRef; // OK, will return the first element of 'theSpan'
}
else
{
    char c = theCharRef; // undefined behavior, including AVing the process
}

The references returned by MemoryMarshal.GetReference are still valid for comparison APIs like Unsafe.IsAddressGreaterThan. If you look at the implementation of MemoryExtensions.Reverse, you'll see that we use a combination of MemoryMarshal.GetReference and Unsafe.IsAddressLessThan in order to compare two refs as if they were pointers. Crucially, the implementation of the Reverse method does not attempt to dereference the result of MemoryMarshal.GetReference unless it is known that it points to legitimate data. Otherwise the method could AV.

The reason this all works from a runtime perspective is that per ECMA-335, Sec. II.14.4.2, it's valid for a managed pointer (type &) to reference just past the end of a managed array. That is:

int[] arr = [ 10, 20, 30 ];

'arr' memory layout is
[ OBJ_HEADER | <vtable_ptr> | element_count |  (value 10)  |  (value 20)  |  (value 30)  ]

             ^-- 'arr' object ref           ^-- ref arr[0] ^-- ref arr[1] ^-- ref arr[2] ^-- ref arr[3]
                  points here                   points here    points here    points here    would point here

Within the runtime, the object header that begins every object is 2 native integers in width. In the diagram above I've split into two native-int sized fields: OBJ_HEADER and vtable_ptr. This is just for illustrative purposes so I can demonstrate how things are laid out. Assuming a 64-bit process, OBJ_HEADER, vtable_ptr, and element_count are all 64-bit fields. The array's data immediately follows. In this case, since it's an int[], the data is a contiguous buffer of 32-bit values.

(The above paragraph handwaves a little bit for simplicity, but the overall model is generally accurate as of the time of this writing.)

Say that you have a local of type int[], and say that the address of that object is stored in register rax. To get its Length property and store it in register ebx, the assembly would appear as such:

mov ebx, dword ptr [rax + 8] ; element_count is always 32 bits, so use a 32-bit mov instead of a 64-bit mov

Or, assuming register ecx has the index of the element you want to dereference:

; assume 'ecx' has already been bounds-checked
mov ebx, dword ptr [rax + 16 + 4 * rcx] ; contains the value arr[i]
lea rbx, [rax + 16 + 4 * rcx] ; contains ref arr[i], which is GC-trackable

Importantly, it's valid (per ECMA-335 as mentioned above) to get a reference to just past the end of the array as long as you don't try to dereference it. The .NET garbage collector knows to treat this reference as a live reference to the original array, so you're not at risk of suddenly having objects ripped out from under you. This also means that you can perform reference address comparisons, as previously demonstrated by the MemoryExtensions.Reverse method implementation.

; assume 'rax' is an int[] of length 3
; assume 'ecx' = 3
lea rbx, [rax + 16 + 4 * rcx] ; contains ref arr[3], which is GC-trackable

; don't do this - it's no longer GC-trackable since it's now *PAST*
; the address just past the end of the array.
add rbx, 1 ; approx. equivalent to "ref Unsafe.AddByteOffset(ref arr[3], 1)"

; don't do this either - it has undefined behavior since it attempts to read memory
; past the end of the array.
mov rbx, dword ptr [rax + 16 + 4 * rcx] ; approx. equivalent to int value = ref arr[3]

So... that's where this method comes in. It's essentially a way to allow you to write ref T theRef = ref theArr[0]; which also works for zero-length arrays. It disables bounds checks and array variance checks. But because turning off these checks is dangerous, the API gets put onto the MemoryMarshal class (instead of becoming <instance>.GetPinnableReference), and callers must read the documentation before using this API.

Whew, that was a mouthful. :)

@AaronRobinsonMSFT
Copy link
Member

@GrabYourPitchforks You should add that to the Remarks section of the API; can't imagine a better description of the problem/use case.

@GrabYourPitchforks
Copy link
Member Author

@terrajobst (Please not that's OK because the existing GetReference<T>() method don't bind for arrays due to an ambiguous match, so the caller had to add a cast.)

I ran some tests after our meeting and it turns out we were mistaken. :(

public static void M(byte[] bytes)
{
    Foo<byte>(bytes);
}

// M(...) ends up calling this method
private static void Foo<T>(Span<T> s) { }

private static void Foo<T>(ReadOnlySpan<T> s) { }

@ahsonkhan
Copy link
Contributor

I ran some tests after our meeting and it turns out we were mistaken. :(

Yep, that compiles fine (picks the Span<T> overload):
https://dotnetfiddle.net/lFaLGa

So, shall we change this to what we were initially considering:

public static class Unsafe
{
    public static ref T GetRawArrayRef<T>(T[] array);
}

@jkotas
Copy link
Member

jkotas commented Dec 19, 2019

class Unsafe

Unsafe is meant to be only used for things that are expressible in portable IL, but not available in C#. GetRawArrayData method is not expressible in portable IL.

@GrabYourPitchforks
Copy link
Member Author

Wouldn't the particular IL instruction be no.typecheck|rangecheck ldelema, assuming the compiler could emit it and the runtime honored the no.* prefix?

@jkotas
Copy link
Member

jkotas commented Dec 23, 2019

The no.* prefixes are specified as optimization hints. They do not guarantee that the safety checks are actually skipped.

@GSPP
Copy link

GSPP commented Dec 24, 2019

I am amazed to learn that these no.* IL opcode prefixes exist. In all my time with .NET I had never encountered that concept. There are zero Google hits for "no.typecheck".

It seems that these instructions could be very useful if they actually were a guarantee. The ECMA standard seems to allow the runtime to provide this guarantee:

image

https://www.ecma-international.org/publications/files/ECMA-ST/ECMA-335.pdf page 334.

It would be possible to guarantee and document this as a feature of CoreCLR.

@GrabYourPitchforks
Copy link
Member Author

This was already approved, but the originally approved method name GetReference does not work. Using Ahson's suggestion of falling back to GetRawArrayRef instead. The PR is basically ready for merging once we choose an approved name.

@jkotas
Copy link
Member

jkotas commented Jan 6, 2020

"Ref" is an ambiguous abbreviation. I think it should be either GetRawArrayReference (to match GetReference for Span) or GetRawArrayData (to match what's in the PR right now and the name historically used by the runtime for this).

@ahsonkhan
Copy link
Contributor

Either sounds good to me, with my preference towards GetRawArrayReference to match existing naming pattern.

@GrabYourPitchforks
Copy link
Member Author

Rename to GetArrayDataRef.

@jkotas
Copy link
Member

jkotas commented Jan 7, 2020

Could you please share the rationale for the "Ref" suffix? I do not think it follows the framework naming conventions and it is ambiguous with other uses of "Ref" like DangerousAddRef.

@GrabYourPitchforks
Copy link
Member Author

@jkotas I'm partial to GetRawArrayData, but we had a concern that the API name didn't make clear that you're supposed to pull the value out as a T& instead of a T. That is:

T data = MemoryMarshal.GetRawArrayData(theArray); // WRONG
ref T refToData = ref MemoryMarshal.GetRawArrayData(theArray); // CORRECT

Basically, without the Ref suffix, there's too much temptation to write the first line of code and potentially cause issues in your code base.

There are some counterarguments against this. The first is that MemoryMarshal contains advanced APIs and that consumers really should read the docs before using them. (And perhaps we were being too dismissive of that counterargument - I'm open to reconsidering this.) The second is that things like analyzers could flag the first pattern and produce a build warning if we were really concerned with it.

@jkotas
Copy link
Member

jkotas commented Jan 7, 2020

I am ok with the "Ref" suffix, but I think it should be a fully spelled "Reference" to match existing MemoryMarshal.GetReference(Span)

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 5.0 milestone Feb 1, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices
Projects
None yet
Development

No branches or pull requests