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

RuntimeHelpers.CreateSpan<T> #61079

Merged
merged 21 commits into from
Nov 30, 2021
Merged

Conversation

davidwrighton
Copy link
Member

@davidwrighton davidwrighton commented Nov 2, 2021

Implement RuntimeHelpers.CreateSpan<T> #60948

Implementation provides for

  • Both non-intrinsic and intrinsic implementations in CoreCLR
  • Non-intrinsic implementation in Mono
    • Mono implementation also implements untested big endian support

@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.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@davidwrighton
Copy link
Member Author

@echesakov Could you help us out with the implementation of the intrinsic? I'd like to remove the hackathon comments and make the codegen as clean as possible. (Currently the codegen creates a ReadOnlySpan local, and then copies from that local whenever its used, but that seems like it should be fixable. Also, there are some hackathon comments in the IL intrinsic implementation that I'd like guidance on.

davidwrighton and others added 2 commits November 1, 2021 18:04
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@jaredpar
Copy link
Member

jaredpar commented Nov 2, 2021

Couple items I didn't see covered in the issue / PR

  1. What is the expected behavior when a type is provided which is not compatible with the current implementation? For example CreateSpan<Guid>() ?
  2. Why doesn't this have a where T : struct constraint? I can definitely see a world where we allowed any unmanaged type here but reference types seems very hard to envision?
  3. Lets say in the future we expand the set of types for which this API is legal. For example if we allowed for any unmanaged type and not just primitives. How is the C# compiler supposed to detect the set of legal types at compilation time? Basically how can it tell when it's compiling against versions of this API which only support primitive and those which support all unmanaged types? Will we add a runtime feature flag?

@EgorBo
Copy link
Member

EgorBo commented Nov 2, 2021

  1. What is the expected behavior when a type is provided which is not compatible with the current implementation? For example CreateSpan<Guid>() ?

Judging by the current implementation it throws ArgumentException for anything other than primitives and enums

  1. Why doesn't this have a where T : struct constraint? I can definitely see a world where we allowed any unmanaged type here but reference types seems very hard to envision?

Will it protect the API from passing structs with reference types inside? if not why bother 🙂 It needs a sort of "T : nogc" constraint, similar to T : unamanged

@jaredpar
Copy link
Member

jaredpar commented Nov 2, 2021

Will it protect the API from passing structs with reference types inside?

That is what where T: unmanaged is for but it's also lacking here

if not why bother 🙂

This is an argument I'm honestly having a hard time responding to. The goal of the typed languages is to move errors that occur at runtime to compile time. There is no path I'm aware of that would ever allow this to work with reference types hence why allow it as an argument when there is an easy option to stop it?

@jkotas
Copy link
Member

jkotas commented Nov 2, 2021

The constraint does not add much here since this API can be only ever meaningfully used by compiler generated code. We can add it. It does not matter.

well, if we add a struct constraint now we will never be able to drop/change it.

Yes, relaxing constraint is not considered a breaking change.

Basically how can it tell when it's compiling against versions of this API which only support primitive and those which support all unmanaged types? Will we add a runtime feature flag?

Yes, it is what runtime feature flags are for.

@EgorBo
Copy link
Member

EgorBo commented Nov 2, 2021

The constraint does not add much here since this API can be only ever meaningfully used by compiler generated code.

oops, yeah it makes my comments completely irrelevant, sorry for my 5 cents @jaredpar

@davidwrighton
Copy link
Member Author

@jaredpar, @jkotas, @EgorBo I don't see why we would want to use a constraint here. We don't have the ability to actually express a useful constraint that actually covers the actual behavior, all usage of this api is actually compiler controlled, and while a struct or unmanaged constraint seems like it might make sense, I can also see counterexamples. For instance, what if we decided to come up with an encoding for a ReadOnlySpan that specified that the PE file would hold the data as a series of null terminated utf8 strings and make that useable from RuntimeHelpers.InitializeArray and this new api.

While not in the api proposal, violations of the preconditions are documented in the api comments (which will turn into documentation) to specify that violations will turn into ArgumentException.

@davidwrighton davidwrighton marked this pull request as ready for review November 2, 2021 21:59
@jaredpar
Copy link
Member

jaredpar commented Nov 2, 2021

all usage of this api is actually compiler controlled,

All of the intended usage is compiler controlled. There is nothing stopping users from calling this directly.

I do understand and agree with the reasons of there isn't a perfect constraint here and hence we should do none. I would lean towards where T : struct but I don't feel that strongly about it.

I don't buy the reasoning in this thread about "this is a compiler API" factoring into the decision though. It's an API, if we have the right tools we should be expressing the right contracts.

While not in the api proposal, violations of the preconditions are documented in the api comments (which will turn into documentation) to specify that violations will turn into ArgumentException.

Sounds reasonable.

Comment on lines 119 to 128
public static ReadOnlySpan<T> CreateSpan<T>(RuntimeFieldHandle fldHandle)
{
unsafe
{
void* data = default;
int count = default;
GetSpanDataFrom(fldHandle, typeof(T).TypeHandle, &data, &count);
return new ReadOnlySpan<T>(data, count);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public static ReadOnlySpan<T> CreateSpan<T>(RuntimeFieldHandle fldHandle)
{
unsafe
{
void* data = default;
int count = default;
GetSpanDataFrom(fldHandle, typeof(T).TypeHandle, &data, &count);
return new ReadOnlySpan<T>(data, count);
}
}
public static unsafe ReadOnlySpan<T> CreateSpan<T>(RuntimeFieldHandle fldHandle)
=> new ReadOnlySpan<T>(GetSpanDataFrom(fldHandle, typeof(T).TypeHandle, out int length), length);

Nit: You can make it one-liner if you change GetSpanDataFrom to return the pointer.

const char *rvaData;

if (!image_is_dynamic (m_class_get_image (klass)))
{
Copy link
Contributor

Choose a reason for hiding this comment

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

This and other places should use K&R indentation.

@stephentoub
Copy link
Member

Awesome to this go in! Is someone on point to follow-up on productizing the prototype on the Roslyn side of things?

@ghost ghost locked as resolved and limited conversation to collaborators Dec 31, 2021
@davidwrighton davidwrighton deleted the CreateSpanIntrinsic branch April 13, 2023 18:50
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.

9 participants