Skip to content

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 ghost assigned stephentoub Oct 6, 2023
@ghost ghost added the area-Meta label Oct 6, 2023
@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!

@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