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

Replace confusing new T[] { ... } optimization with collection expressions #93125

Merged
merged 3 commits into from
Oct 6, 2023

Conversation

stephentoub
Copy link
Member

We rely heavily on the compiler rewriting:

ReadOnlySpan<T> span = new T[] { /* const data */ };

into variants that don't actually allocate and instead blit the data into the assembly data section and create a span that points directly to that data. But the syntax is very confusing and looks like it allocates on every use.

Now that we have collection expressions and they handle the same optimizations (and more), we can avoid the confusion and simplify the code by switching to use them. This does so for all the uses I could find with some greping around.

…essions

We rely heavily on the compiler rewriting:
```C#
ReadOnlySpan<T> span = new T[] { /* const data */ };
```
into variants that don't actually allocate and instead blit the data into the assembly data section and create a span that points directly to that data. But the syntax is very confusing and looks like it allocates on every use.

Now that we have collection expressions and they handle the same optimizations (and more), we can avoid the confusion and simplify the code by switching to use them. This does so for all the uses I could find with some greping around.
@ghost
Copy link

ghost commented Oct 6, 2023

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

Issue Details

We rely heavily on the compiler rewriting:

ReadOnlySpan<T> span = new T[] { /* const data */ };

into variants that don't actually allocate and instead blit the data into the assembly data section and create a span that points directly to that data. But the syntax is very confusing and looks like it allocates on every use.

Now that we have collection expressions and they handle the same optimizations (and more), we can avoid the confusion and simplify the code by switching to use them. This does so for all the uses I could find with some greping around.

Author: stephentoub
Assignees: stephentoub
Labels:

area-Meta

Milestone: -

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.

Nice!

@stephentoub stephentoub merged commit 7204fbe into dotnet:main Oct 6, 2023
171 of 174 checks passed
@stephentoub stephentoub deleted the avoidarrayconfusion branch October 6, 2023 21:19
@CyrusNajmabadi
Copy link
Member

Woohoo. Did you use the analyzer/fixer here? Or do this manually?

@stephentoub
Copy link
Member Author

stephentoub commented Oct 6, 2023

Woohoo. Did you use the analyzer/fixer here? Or do this manually?

Manual (regex). The analyzer doesn't flag these, or at least it doesn't in the version I'm using (Version 17.9.0 Preview 1.0 [34203.221.main]), e.g. for:

internal class Program
{
    private static void Main()
    {
        ReadOnlySpan<int> span = new int[] { 1, 2, 3, 4, 5, 6, 7, 8, 9 };
        Console.WriteLine(span[0]);
    }
}

no suggestion or fix is offered.


private static ReadOnlySpan<byte> EightZeros => new byte[8];
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this one was actually allocating on each call :(

Copy link
Member

@danmoseley danmoseley Oct 6, 2023

Choose a reason for hiding this comment

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

Is that necessarily bad? Often we avoid caching memory that would never leave Gen0 (I don't know this code OC)

Copy link
Member Author

Choose a reason for hiding this comment

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

Is that necessarily bad? Often we avoid caching memory that would never leave Gen0 (I don't know this code OC)

If this had instead been:

private static ReadOnlySpan<byte> EightZeros => new byte[8] { 0, 0, 0, 0, 0, 0, 0, 0 };

there wouldn't have been any allocation, not even on first use: it would have simply been eight zero'd bytes in the assembly data, and the span would have pointed directly to that memory.
SharpLab

And, yes, a property that looks like it should be non-allocating but that allocates on every access is bad :)

Copy link
Member

Choose a reason for hiding this comment

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

Oh, wow, TIL.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should just fix that in Roslyn. I don't see a good reason for these two to exhibit such a codegen difference. (Though moving forward we'll use that syntax much less and so hopefully won't trip over it again.)
cc: @jcouv

@CyrusNajmabadi
Copy link
Member

Manual (regex). The analyzer doesn't flag these, or at least it doesn't in the version I'm using (Version 17.9.0 Preview 1.0 [34203.221.main]), e.g. for:

I'll add support for that case.

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.

7 participants