csharplang icon indicating copy to clipboard operation
csharplang copied to clipboard

Five ideas for improving working with fixed buffers

Open gafter opened this issue 6 years ago • 22 comments

I just spent an hour or so brainstorming with @JeremyKuhne about issues he's had with fixed buffers, readonly struct, and ref structs. He'd like to declare some structures for interop that are fairly large, and would like to avoid having them copied by the compiler for defensive reasons. There are a few corners of the language that make this awkward in some circumstances.

For example, if you declare a struct readonly, so as to avoid defensive copies when calling its methods, then you cannot place a fixed struct inside it (because a fixed struct can't be declared readonly, and a readonly struct cannot contain a field that isn't declared readonly).

We came up with the following four (prioritized) suggestions for how to improve things.

  1. Have a way to get warnings for situations in which the compiler would or does produce a defensive copy. Because the compiler's precise rules are subtle, the most precision would be achieved if this were done in the compiler. One way to do this would be to have the compiler always produce a warning when a compiler-generated defensive copy is generated for a struct type that has some particular annotation on it. One could also imagine doing this in an analyzer, though that would be harder and possibly less precise. We placed this number one on our list because no matter the other tools are available to address the issues, this is needed to know when to deploy them. (https://github.com/dotnet/roslyn/issues/26937) [Under consideration as part of a warning wave in C# 9.0]

  2. Permit a fixed field to be declared inside a readonly struct. (#1793) [Under consideration for C# 9.0]

  3. Permit a "readonly" modifier to be placed on the individual function members of the struct, which would be a declaration that the this parameter is ref readonly for that member instead of ref which is usual. This is a little bit like the semantics of a readonly struct, but applied to the members one-by-one. Invocations of such members would not require a defensive copy of the receiver if the receiver is readonly. (See #1710) [Done in C# 8.0]

  4. Provide some kind of new autoproperty syntax for a property of type Span<T> or ReadonlySpan<T> with a compiler-generated fixed backing field, perhaps something like fixed ReadonlySpan<int> fixedBuffer[16] { get; }. The compiler would generate a fixed field to back it and would provide the implementation of the get accessor. [Not currently under consideration. Although there is more boilerplate, it is possible that the other issues might make the effect of this achievable by hand]

  5. A fixed statement should not be required on a fixed field of a ref struct, because it is never moveable. (#1792) [Under consideration for C# 9.0]

After some discussion, we'll probably promote these individual ideas to separate, possibly championed issues.

gafter avatar May 07 '18 23:05 gafter

I am agree on 3. But could we make it backward compatibility automatically?

Thaina avatar May 08 '18 01:05 Thaina

We ought to be able to declare fixed fields consisting of any unmanaged structs and not just the basic scalar types.

asyncawaitmvvm avatar May 08 '18 02:05 asyncawaitmvvm

@gafter @asyncawaitmvvm

We ought to be able to declare fixed fields consisting of any unmanaged structs and not just the basic scalar types.

I think that we ought to be able to declare fixed fields consisting of any types, not only unmanaged structs. Furtherly, we should also be able to declare fixed buffers in classes, not only in structs. We should even be able to declare static fixed array too. In one word, we should be able to use any types of fixed buffers anywhere. For example:

class fixedbuffer
{
    private object obj_buf[100];
    private static object obj_sbuf[10];
}

ygc369 avatar May 08 '18 05:05 ygc369

I really like 1 and 3 and think they would be useful beyond fixed buffers.

An attribute that triggers warnings on copies could possibly be done by an analyzer outside of the language though. And there are times where I think it would be nice to warn on copies of a struct defined in another assembly. Perhaps if you could simply flip a switch and temporarily enable warnings on any struct copy for a build...

bbarry avatar May 08 '18 13:05 bbarry

no. 3 would be very useful to have, in general.

In my math library, I have structs like Vector3 or Matrix4x4 that I don't necessarily want to declare as read-only structs. Defensive copies due to ref readonly was a nasty surprise for non-mutating functions like .Length().

Starnick avatar May 08 '18 17:05 Starnick

@ygc369, there is already a championed proposal for extending fixed-sized-buffers to support arbitrary types (see https://github.com/dotnet/csharplang/blob/master/proposals/fixed-sized-buffers.md).

tannergooding avatar May 08 '18 17:05 tannergooding

Interop scenarios are the key driver for me. For reference, here is a fixed pattern that we've been starting to use in CoreFX:

[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)]
internal unsafe struct WIN32_FIND_DATA
{
    internal uint dwFileAttributes;
    internal FILE_TIME ftCreationTime;
    internal FILE_TIME ftLastAccessTime;
    internal FILE_TIME ftLastWriteTime;
    internal uint nFileSizeHigh;
    internal uint nFileSizeLow;
    internal uint dwReserved0;
    internal uint dwReserved1;
    private fixed char _cFileName[MAX_PATH];
    private fixed char _cAlternateFileName[14];

    internal ReadOnlySpan<char> cFileName
    {
        get { fixed (char* c = _cFileName) return new ReadOnlySpan<char>(c, MAX_PATH); }
    }
}

In this case:

  1. I'd love to express in code that it is read-only and that we don't ever want copies
  2. The struct is particularly large (~600 bytes), copies aren't trivial
  3. The ROS property is mildly annoying to write and potentially error prone (e.g. getting the size wrong- more likely with some of the more complicated structs)

We've also started wrapping native data in public ref structs to reduce unnecessary allocations. I expect to continue to add similar wrapper structs. Being able to detect defensive copies and prevent them would be fantastically useful.

JeremyKuhne avatar May 08 '18 18:05 JeremyKuhne

@asyncawaitmvvm @ygc369 Your suggestions don't address the scenarios that are driving these five proposals. See @JeremyKuhne 's note, just above, for a summary.

gafter avatar May 08 '18 20:05 gafter

3 is not only useful for structs but also classes. There are already quite a few discussions on const/pure functions.

qrli avatar May 09 '18 09:05 qrli

3 is not only useful for structs but also classes. There are already quite a few discussions on const/pure functions.

Yeah, even if it only boils down to a compile-time analyzer for class members instead of including the runtime implications that would be applied to struct members (classes are always copied by pointer, after all), it would be nice if the concept could be used across the board.

Joe4evr avatar May 09 '18 11:05 Joe4evr

@JeremyKuhne

Was WIN32_FIND_DATA meant to be ref struct? Otherwise such struct (if it happen to get to the heap) can be moved.

    internal ReadOnlySpan<char> cFileName
    {
        get { fixed (char* c = _cFileName) return new ReadOnlySpan<char>(c, MAX_PATH); }
    }

In 7.3 you could you use..

 get { return new ReadOnlySpan<char>(ref _cFileName[0], MAX_PATH); }

.. if ReadOnlySpan had such ctor.

omariom avatar May 10 '18 14:05 omariom

@omariom

Was WIN32_FIND_DATA meant to be ref struct?

We haven't started changing our interop structs to ref structs (where possible) yet. I do plan to.

In 7.3 you could you use.. .. if ReadOnlySpan had such ctor.

Interesting. @ahsonkhan, thoughts?

JeremyKuhne avatar May 10 '18 17:05 JeremyKuhne

.. if ReadOnlySpan had such ctor.

We do have such a ctor but it is internal only and used mainly for Slicing. https://github.com/dotnet/corefx/blob/master/src/Common/src/CoreLib/System/ReadOnlySpan.Fast.cs#L115

What was the initial design reason for keeping this internal?

cc @jkotas, @krzysztofcwalina

ahsonkhan avatar May 10 '18 18:05 ahsonkhan

That constructor is very unsafe. We have moved all unsafe Span operations to MemoryMarshal. The public equivalent of this constructor is MemoryMarshal.CreateSpan API.

jkotas avatar May 10 '18 19:05 jkotas

So you can write get { return MemoryMarshal.CreateSpan<char>(ref _cFileName[0], MAX_PATH); } today.

jkotas avatar May 10 '18 19:05 jkotas

So you can write get { return MemoryMarshal.CreateSpan(ref _cFileName[0], MAX_PATH); } today.

Cool, totally missed that existed. Thanks!

JeremyKuhne avatar May 10 '18 19:05 JeremyKuhne

There is also MemoryMarshal.CreateReadOnlySpan. It is better to use that (to avoid the extra cast for JIT to optimize out) if you want to create ReadOnlySpan.

jkotas avatar May 10 '18 19:05 jkotas

One way to do this would be to have the compiler always produce a warning when a compiler-generated defensive copy is generated for a struct type that has some particular annotation on it.

Having a "no copy struct" annotation is something that me, @KrzysztofCwalina and others were wishing for a long time. It has many uses beyond interop. I know that it does not interact well with all different features of the language, but having a warning for this would great!

jkotas avatar May 10 '18 20:05 jkotas

One caveat to the MemoryMarshal approach is that it is only available when targeting netcoreapp. :/

JeremyKuhne avatar May 10 '18 21:05 JeremyKuhne

@gafter @jkotas

Having a "no copy struct" annotation is something that me, @KrzysztofCwalina and others were wishing for a long time. It has many uses beyond interop. I know that it does not interact well with all different features of the language, but having a warning for this would great!

Good idea.

ygc369 avatar May 11 '18 02:05 ygc369

Would love for MemoryMarshal.CreateSpan to be available in a netstandard2.0 project.

zone117x avatar May 17 '18 02:05 zone117x

need allow custom struct as fxied type,see the Type: CustomAsStructBufferTypeDemo


internal unsafe struct WIN32_FIND_DATA
{
    internal uint dwFileAttributes;
    //...fileds
}

unsafe  struct CustomAsStructBufferTypeDemo
{
  public fixed WIN32_FIND_DATA[10] Buffer; //it's cant define now
}

sgf avatar Jun 02 '21 22:06 sgf