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

Refer directly to static data when ReadOnlySpan wraps arrays of bytes. #24621

Merged
merged 6 commits into from
Mar 5, 2018

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Feb 3, 2018

Refer directly to static data when ReadOnlySpan wraps strings or arrays of primitive literals.
No need to allocate anything in this case.

Fixes:#23358
Related:dotnet/corefx#25413

Ask Mode template not completed

Customer scenario

What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)

Bugs this fixes

(either VSO or GitHub links)

Workarounds, if any

Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW

Risk

This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code

Performance impact

(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")

Is this a regression from a previous update?

Root cause analysis

How did we miss it? What tests are we adding to guard against it in the future?

How was the bug found?

(E.g. customer reported it vs. ad hoc testing)

Test documentation updated?

If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.

@VSadov VSadov requested a review from a team as a code owner February 3, 2018 20:20
@VSadov
Copy link
Member Author

VSadov commented Feb 3, 2018

@dotnet/roslyn-compiler - please review

@VSadov
Copy link
Member Author

VSadov commented Feb 3, 2018

When available, this could be useful to our own tests. We allocate a lot of single-use strings for the code and IL. If plumbed through as ReadOnlySpan<char>, it could stay static data - no allocations. #Resolved

@VSadov
Copy link
Member Author

VSadov commented Feb 3, 2018

@stephentoub @jkotas @KrzysztofCwalina - you may like this :-) #Resolved

@jkotas
Copy link
Member

jkotas commented Feb 3, 2018

@marek-safar Is this going to work for Mono on big endian machines? #Resolved

IL_0000: call ""string Test.Hello()""
IL_0005: call ""System.ReadOnlySpan<char> System.ReadOnlySpan<char>.op_Implicit(string)""
IL_000a: stloc.0
IL_000b: ldsflda ""<PrivateImplementationDetails>.__StaticArrayInitTypeSize=10 <PrivateImplementationDetails>.D2EFCBBA102ED3339947E85F4141EB08926E40E9""
Copy link
Member

@jkotas jkotas Feb 3, 2018

Choose a reason for hiding this comment

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

Do the PrivateImplementationDetails types have the right alignment in the final binary? (E.g. if the Span element type long, they should be aligned at 8 bytes).

Are there enough breadcrumbs for IL rewriting tools to maintain the alignment? #Resolved

Copy link
Member Author

@VSadov VSadov Feb 3, 2018

Choose a reason for hiding this comment

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

We use the same for ordinary arrays inits.
Would that imply the use for span is ok or span has extra requirements? #Resolved

Copy link
Member

@jkotas jkotas Feb 3, 2018

Choose a reason for hiding this comment

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

InitializeArray gets the breadcrumbs about the element size via the RuntimeFieldHandle argument.

And the old CoreCLR big endian path through InitializeArray (currently unused) is explicitly handling the misasligned case: https://github.com/dotnet/coreclr/blob/46ab1d132c9ad471d79afa20c188c2f9c85e5f20/src/classlibnative/bcltype/arraynative.cpp#L1442 .

The optimization is definitely fine for span of bytes - it would be very useful even for just that. But it may need more plumbing to work well for spans of larger types to handle alignment and endianess. #Resolved

Copy link
Member Author

@VSadov VSadov Feb 3, 2018

Choose a reason for hiding this comment

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

Can this memory be written to?
I can re-end in a static ctor #Resolved

Copy link
Member

@jkotas jkotas Feb 3, 2018

Choose a reason for hiding this comment

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

It cannot be written to. The static data are R/O in IL-only images. #Resolved

Copy link
Member Author

@VSadov VSadov Feb 4, 2018

Choose a reason for hiding this comment

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

Ok, assuming that runtime can fixup endiannes, it would need to know the size of the "element" in the blob. It cannot know it now.

We refer to the blobs via FieldRVA static fields. The fields have types of appropriate size - byte, short, int, long or a synthetic struct with .size.
The types have no meaning in existing cases, only size matters. I.E. 4 shorts will be mapped via a long field...

What if for the blobs where we care about alignment/endiannes, we always use syntetic struct with .size specifying the size and .pack specifying the element size?

Would that be sufficient?

Alternatively the synthetic struct could have a dummy primitive field of appropriate size. - byte, ..., long.
Or even the actual element type field, may be even N fields, although that seems a bit of an overkill, since there are often thousands of elements in these blobs.

What are the IL rewriters going to do?

IL rewriters should align on 8 bytes and keep the rest as-is. Most likely they do that already.
FWIW System.Reflection.Metadata exposes the desired alignment as ManagedPEBuilder.MappedFieldDataAlignment.

#Resolved

Copy link
Member

@jkotas jkotas Feb 4, 2018

Choose a reason for hiding this comment

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

Alternatively the synthetic struct could have a dummy primitive field of appropriate size.

I think this would be my pick. There is prior art in managed C++. Managed C++ emits dummy field like that (together with size directive) to control alignment of opaque structs. .pack does not make any guarantees about alignment today.

IL rewriters should align on 8 bytes and keep the rest as-is. Most likely they do that already.

It would be useful to confirm.

Should the availability of this optimization be controlled via new RuntimeFeature (for spans of larger types)? #Resolved

Copy link
Member Author

@VSadov VSadov Feb 4, 2018

Choose a reason for hiding this comment

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

Alternatively the synthetic struct could have a dummy primitive field of appropriate size.

Will add this to the PR, then. And add alignment tests.

Will have to check the rewriters, but I think that is not blocking, just need to tell them.

Need to think about RuntimeFeature. Maybe. #Resolved

Copy link
Member Author

@VSadov VSadov Feb 4, 2018

Choose a reason for hiding this comment

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

@alrz - FYI. Re: #24249
Whatever is decided here will have effect on the stackalloc initializers. The cpblk path in initializers is exposed to the endianness in the same way.

Nothing here is actionable to your current PR right now, so this is mostly FYI, but whatever approach is used for big types will have to be used in stackalloc initializers as well. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

since we are constrained to 1-byte elements, this is irrelevant now


In reply to: 165826041 [](ancestors = 165826041)

@marek-safar
Copy link
Contributor

marek-safar commented Feb 5, 2018

@jkotas that's a good question. I think it depends if used ReadOnlySpan ctor is endian friendly (we are using CoreFX implementation) #Resolved

@jkotas
Copy link
Member

jkotas commented Feb 5, 2018

I think it depends if used ReadOnlySpan ctor is endian friendly (we are using CoreFX implementation)

It is not. It just takes pointer to raw data. #Resolved

@marek-safar
Copy link
Contributor

marek-safar commented Feb 5, 2018

@jkotas right so it'll break unless the underlying Unsafe class implementation can handle it #Resolved

@jkotas
Copy link
Member

jkotas commented Feb 6, 2018

I cannot think of a magic in the underlying Unsafe class that can handle it.

To make this optimization endianess agnostic, I think we would need a new runtime API. It can be similar to the existing RuntimeHelpers.InitializeArray API. Something like:

static class RuntimeHelpers {
    static ReadOnlySpan<T> GetReadOnlySpanFromTemplate<T>(RuntimeFieldHandle fldHandle);
}
``` #Resolved

@marek-safar
Copy link
Contributor

marek-safar commented Feb 6, 2018

Yep, that would work #Resolved

@VSadov
Copy link
Member Author

VSadov commented Feb 6, 2018

If we go with the new API, how soon can we have it?
I assume on littleendian system it would be very simple.

Also -We will face the same issue with the stackalloc initializers. Efficient implementation would cpblk from a blob, but bigendian is a problem.
We could cpblk from a ReadonlySpan as well, so GetReadOnlySpanFromTemplate solution would work there too. #Resolved

@jkotas
Copy link
Member

jkotas commented Feb 6, 2018

If we go with the new API, how soon can we have it?

Getting this API to .NET Core 2.1 would be a stretch. We are trying to lock down for .NET Core 2.1 stabilization in a week or so.

I assume on littleendian system it would be very simple.

Yes, it should be simple for littleendian system. But it would be nice to give Mono team time to validate that the design works for bigendian systems before we ship it as stable. #Resolved

@VSadov
Copy link
Member Author

VSadov commented Feb 6, 2018

Also, I assume, GetReadOnlySpanFromTemplate solution will not need that mapping structs have element fields. Just ".size" would be sufficient - like in the case with regular array initializers. #Resolved

@VSadov
Copy link
Member Author

VSadov commented Feb 6, 2018

The other advantage of GetReadOnlySpanFromTemplate approach is that we do not need any special feature checks - can simply check for the presence of the API. #Resolved

@VSadov
Copy link
Member Author

VSadov commented Feb 7, 2018

Ok, until the https://github.com/dotnet/corefx/issues/26948 is resolved we will not try optimizing the cases where elements are larger then 1 byte. #Resolved

@jcouv jcouv added this to the 15.7 milestone Feb 23, 2018
@jcouv
Copy link
Member

jcouv commented Feb 23, 2018

@VSadov Is this PR ready for review or still design or dependency to figure out? #Resolved

@VSadov
Copy link
Member Author

VSadov commented Feb 23, 2018

There is a dependency that is unlikely to be ready soon.

I will scale this down to be applicable only to 1-byte sized element types. #Resolved

@jcouv jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Feb 23, 2018
@jcouv
Copy link
Member

jcouv commented Feb 23, 2018

Ok. I added the "personal PR" label for now. #Resolved

@VSadov VSadov changed the title Refer directly to static data when ReadOnlySpan wraps strings or arrays of primitive literals. Refer directly to static data when ReadOnlySpan wraps arrays of bytes. Feb 23, 2018
…literals.

No need to allocate anything in this case.
@KrzysztofCwalina
Copy link
Member

KrzysztofCwalina commented Feb 24, 2018

We should do this for Utf8String where there are no endiannes issues.
#Resolved

@VSadov
Copy link
Member Author

VSadov commented Feb 24, 2018

Yes, once we have Utf8String literals, this optimization will be applicable. #Resolved

@VSadov
Copy link
Member Author

VSadov commented Feb 24, 2018

@dotnet-bot test windows_debug_vs-integration_prtest please #Resolved

@VSadov
Copy link
Member Author

VSadov commented Feb 24, 2018

license/cla seems to be stuck #Resolved

MykolaBalakin added a commit to MykolaBalakin/roslyn that referenced this pull request Aug 14, 2019
jcouv added a commit that referenced this pull request Aug 14, 2019
Test ReadOnlySpan initialization from static data (#24621)
eerhardt added a commit to apache/arrow that referenced this pull request May 26, 2020
Adding an `.editorconfig` using the rules from dotnet/runtime repo. See:

* https://github.com/dotnet/runtime/blob/master/docs/coding-guidelines/coding-style.md
* https://github.com/dotnet/runtime/blob/master/.editorconfig

Violating these rules won't fail the build, but Visual Studio will respect these rules when formatting the code and make suggestions.

The bulk of the violations were `var` usages. There was one unnecessary usage of `this.`. And the rest were naming static fields with `s_`. In `BitUtility.cs`, I made a slight optimization using the pattern recognized by dotnet/roslyn#24621 instead of prefixing the fields with `s_`.

Tagging anyone who has contributed to the C# library (please let me know if I missed anyone):
@chutchinson @zgramana @nhustler @HashidaTKS @abbotware @pgovind @stephentoub

Closes #7246 from eerhardt/Editorconfig

Authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Signed-off-by: Eric Erhardt <eric.erhardt@microsoft.com>
wieslawsoltes pushed a commit to wieslawsoltes/Svg.Skia that referenced this pull request Jul 18, 2020
* Use ReadOnlySpan<byte> optimization

see: dotnet/roslyn#24621

* Reduce array allocations

* Prefer OrdinalIgnoreCase to InvariantCultureIgnoreCase

* Use StringComparison.Ordinal with StartsWith

* Add readonly to readonly Rect members
jstedfast pushed a commit to jstedfast/MimeKit that referenced this pull request Feb 14, 2022
* Eliminate array allocation in MimeUtils.Unquote()
* Use stackalloc in Header.Unfold() to reduce string allocations
* Use stackalloc in Header constructor
* Eliminate two buffer allocations in DecodeRfc2231
* Use static ReadOnlySpan<byte> compiler feature to reference to static data directly in the encoders

See dotnet/roslyn#24621 & https://vcsjones.dev/csharp-readonly-span-bytes-static/ for
more information about the ReadOnlySpan<byte> static array usage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants