Prefer static ReadOnlySpan<byte> properties over static readonly byte[] fields

[terrajobst]

With a pattern like private static readonly byte[] s_array = new byte[] { ... } where the static readonly byte[] is internal/private, all consumers of s_array could instead operate on a span, and the field is initialized to a new byte[] of constant values, it can be changed instead to static ReadOnlySpan<byte> Data => new byte[] { ... }, and the C# compiler will optimize the implementation.

Category: Performance

[jeffhandley]

Estimates:

• Analyzer: Medium
• Fixer: Small

[Mrnikbobjeff]

Should this only trigger for byte or for other primitives as well?

[stephentoub]

byte, sbyte, and bool

[carlossanlop]

The analyzer makes sense, but the fixer is potentially problematic. Take a look at the referenced PR from above, where after converting the byte array to a ROS, the places where it was used had to be heavily adjusted.

Applying a fixer would cause build failures if methods that are exclusive for that data type were being called (byte[].Rank, for example).

Question:

• Why should only byte, sbyte, bool be considered for this analyzer?

Cases that the analyzer would handle:

// Yes
private static readonly byte[] s_byte = new byte[] { ... };
internal static readonly byte[] s_byte = new byte[] { ... };

// No
public static readonly byte[] s_byte = new byte[] { ... }; // public
protected static readonly byte[] s_byte = new byte[] { ... }; // protected
private readonly byte[] s_byte = new byte[] { ... }; // no static
private static byte[] s_byte = new byte[] { ... }; // no readonly

[stephentoub]

Why should only byte, sbyte, bool be considered for this analyzer?

Until #24961 is implemented and Roslyn optimizes with it, Roslyn only applies the relevant optimization to single-byte primitive types, i.e. byte, sbyte, and bool.

[ghost]

This issue has been automatically marked no recent activity because it has been marked as needs author feedback but has not had any activity for 14 days. It will be closed if no further activity occurs within 7 more days. Any new comment (by anyone, not necessarily the author) will remove no recent activity

[carlossanlop]

Removing the label so it doesn't get autoclosed by the bot.

This could be ready to mark as ready for review, keeping in mind that the analyzer would be limited to byte/sbyte/bool. We can decide if we want a fixer or not in the API review meeting.

@buyaa-n @stephentoub any additional thoughts you'd like to share before I apply the label?

[En3Tho]

@stephentoub Is there an option to get static read-only ros backing memory region by reflection? Can we actually consider adding const modifier to such ros? Like with { 1, 2, 3, 4, 5 } value

[stephentoub]

Can we actually consider adding const modifier to such ros? Like with { 1, 2, 3, 4, 5 } value

This is a question for C#. I suggest opening an issue/proposal in the csharplang repo, if there isn't one already.

[stephentoub]

We can decide if we want a fixer or not in the API review meeting.

We do :-)

[AraHaan]

I would prefer this for other things like short[], and ushort[] on static fields/properties as well so that way ``new`ing them up can be avoided for fixed data.

But that could be for really much any type[] arrays where type is any type the developer is returning as an array on a property.

[AraHaan]

Interesting I tried applying it to the parts of my classes where byte[] was returned from a field / property and it said it only works for members of ref structs and not classes...

I would really like that relaxed since that could result in improvements to static class members in terms of eliminating an memcpy call in the end to copy the data inside of it 😄 when it could be avoided.

Edit: The compiler seems to not like this:

internal static class Tree
{
    internal static ReadOnlySpan<byte> BlOrder { get; } = CreateBlOrder();

    // other static properties.

    // other things.

    private static ReadOnlySpan<byte> CreateBlOrder()
        => new byte[]
        {
            16, 17, 18, 0, 8, 7, 9, 6, 10, 5, 11, 4, 12, 3, 13, 2, 14, 1, 15,
        };
}

And when I try to wrap that property in a dummy ref struct inside of Tree that is internal as well it still does the same compile error 😆.

[stephentoub]

You can't store a span in a field, which this does:

    internal static ReadOnlySpan<byte> BlOrder { get; } = CreateBlOrder();

This would need to be:

    internal static ReadOnlySpan<byte> BlOrder => CreateBlOrder();

[bartonjs]

Video

Category: Performance
Level: Info

• The analyzer should look at all static readonly field declarations where the field type is an array of byte/sbyte/bool, the array is initialized with a literal array initializer. (Note that in the future some other types may also be allowed, but that will depend on the presence of a RuntimeFeatures feature)
  • Then do a candidate elimination round: Any time that the array was passed to some other method as an array (vs going through AsSpan explicitly or implicitly) then do not report on this field.
• Any remaining fields can be changed to the new pattern.

[NewellClark]

I would like to be assigned to this, please.

[Youssef1313]

it can be changed instead to static ReadOnlySpan<byte> Data => new byte[] { ... },

@terrajobst Should be { get; } = instead of =>? (this was caught by @jaredpar in dotnet/roslyn#56845 (comment))

Edit: Actually this might be intentional per #33780 (comment). I'm not sure.

[Youssef1313]

@jaredpar It seems like static ReadOnlySpan<byte> Data => new byte[] { 0, 0, 1, 2, 3 }; doesn't actually allocate?

internal class C
{
    private unsafe static ReadOnlySpan<byte> Data
    {
        get
        {
            return new ReadOnlySpan<byte>(System.Runtime.CompilerServices.Unsafe.AsPointer(ref <PrivateImplementationDetails>.A498EFA8D0759E7B704095853DB9BC1EF8CF65AF37BED9D006C4B2917E695061), 5);
        }
    }
}
[CompilerGenerated]
internal sealed class <PrivateImplementationDetails>
{
    [StructLayout(LayoutKind.Explicit, Pack = 1, Size = 5)]
    private struct __StaticArrayInitTypeSize=5
    {
    }

    internal static readonly __StaticArrayInitTypeSize=5 A498EFA8D0759E7B704095853DB9BC1EF8CF65AF37BED9D006C4B2917E695061/* Not supported: data(00 00 01 02 03) */;
}

[jaredpar]

That's correct. I frequently forget that this pattern is optimized to be non-allocating.

[AraHaan]

That is until you turn the span into a list or array, then it allocates a copy.

However what if I did not want to allocate an array but instead operate on the data in the span, (and modify it) I am assuming then it would modify it for then whenever the application ever uses that property again to get a span, it would get not the original one, but the modified one which would be an regression on my end that I would have to be careful with while at the same time I want to reduce allocations to as little to nothing if possible.

[gfoidl]

instead operate on the data in the span, (and modify it)

It's a ReadOnlySpan (or sbyte, bool), so any modification should be prohibited in safe code.

[skyoxZ]

It seems that this change can also help to avoid some potential bug:

byte[] arr = new DataWrapper().GetArray();
arr[0] = 2;
Console.WriteLine(new DataWrapper().GetArray()[0]);

class DataWrapper
{
    private static readonly byte[] array = new byte[] { 1 };

    public byte[] GetArray() => array;
}