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

Array -> Memory -> Span overly defensive? #10836

Closed
benaadams opened this issue Aug 5, 2018 · 5 comments
Closed

Array -> Memory -> Span overly defensive? #10836

benaadams opened this issue Aug 5, 2018 · 5 comments

Comments

@benaadams
Copy link
Member

Creating a {ReadOnly}Memory from an array does lots of checks; which is correct
https://github.com/dotnet/coreclr/blob/9d1e7dd8eb744b14eb4a90b401e854d7a5f79fff/src/System.Private.CoreLib/shared/System/Memory.cs#L96-L113

Slicing a {ReadOnly}Memory also performs bounds checks based on the confirmed offset and lengths

However creating a Span from and array backed {ReadOnly}Memory uses the regular array constructor
https://github.com/dotnet/coreclr/blob/9d1e7dd8eb744b14eb4a90b401e854d7a5f79fff/src/System.Private.CoreLib/shared/System/Memory.cs#L286-L289

Which then rechecks all the parameters again
https://github.com/dotnet/coreclr/blob/9d1e7dd8eb744b14eb4a90b401e854d7a5f79fff/src/System.Private.CoreLib/shared/System/Span.Fast.cs#L73-L89

This makes an array backed Memory the most expensive kind of Span creator.

Does it need to recheck everything or can it use an internal Span .ctor that bypasses the additional checks?

/cc @jkotas @stephentoub @GrabYourPitchforks @davidfowl @ahsonkhan

@benaadams
Copy link
Member Author

Issue with struct tearing on Memory<T> perhaps? Should still be able to skip the array type checks?

@jkotas
Copy link
Member

jkotas commented Aug 5, 2018

Issue with struct tearing on Memory<T> perhaps?

Right.

@GrabYourPitchforks
Copy link
Member

I think the JIT will automatically elide the null check in the Span<T> ctor, because it should be able to reason that the argument cannot be null due to the shape of the call site in Memory<T>.get_Span, and everything gets inlined. However I don't think it can automatically elide the typeof(T[]) check, even though within Memory<T>.get_Span we know this check will succeed. Maybe a potential micro-improvement opportunity?

@benaadams
Copy link
Member Author

@GrabYourPitchforks this is fixed now?

@ahsonkhan
Copy link
Member

However creating a Span from and array backed {ReadOnly}Memory uses the regular array constructor

Does it need to recheck everything or can it use an internal Span .ctor that bypasses the additional checks?

Yes, it no longer uses the regular (checked) span ctor:
https://github.com/dotnet/coreclr/blob/4e23b22ee5cce5db9f8aafb4c6e3b17dbfe07567/src/System.Private.CoreLib/shared/System/Memory.cs#L344

Fixed in dotnet/coreclr#20386

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants