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

Five ideas for improving working with fixed buffers #1502

Open
gafter opened this issue May 7, 2018 · 22 comments
Open

Five ideas for improving working with fixed buffers #1502

gafter opened this issue May 7, 2018 · 22 comments

Comments

@gafter
Copy link
Member

gafter commented May 7, 2018

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. (Warning for struct copy of selected types roslyn#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. (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 Champion: Readonly members on structs (16.3, Core 3) #1710) [Done in C# 8.0]

  4. Provide some kind of new autoproperty syntax for a property of type Span or ReadonlySpan 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. (Do not require fixing a fixed field of a ref struct #1792) [Under consideration for C# 9.0]

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

@Thaina
Copy link

Thaina commented May 8, 2018

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

@asyncawaitmvvm
Copy link

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

@ygc369
Copy link

ygc369 commented May 8, 2018

@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];
}

@bbarry
Copy link
Contributor

bbarry commented May 8, 2018

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...

@Starnick
Copy link

Starnick commented May 8, 2018

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().

@tannergooding
Copy link
Member

@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).

@JeremyKuhne
Copy link
Member

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.

@gafter
Copy link
Member Author

gafter commented May 8, 2018

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

@qrli
Copy link

qrli commented May 9, 2018

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

@Joe4evr
Copy link
Contributor

Joe4evr commented May 9, 2018

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.

@omariom
Copy link

omariom commented May 10, 2018

@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.

@JeremyKuhne
Copy link
Member

@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?

@ahsonkhan
Copy link
Member

.. 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

@jkotas
Copy link
Member

jkotas commented May 10, 2018

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
Copy link
Member

jkotas commented May 10, 2018

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

@JeremyKuhne
Copy link
Member

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

Cool, totally missed that existed. Thanks!

@jkotas
Copy link
Member

jkotas commented May 10, 2018

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
Copy link
Member

jkotas commented May 10, 2018

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!

@JeremyKuhne
Copy link
Member

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

@ygc369
Copy link

ygc369 commented May 11, 2018

@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.

@zone117x
Copy link

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

@gafter gafter modified the milestones: 9.0 candidate, X.0 candidate Aug 26, 2019
@MadsTorgersen MadsTorgersen modified the milestones: 9.0 candidate, 10.0 candidate Apr 22, 2020
@333fred 333fred modified the milestones: 10.0 candidate, 10.0 Working Set Sep 9, 2020
@sgf
Copy link

sgf commented Jun 2, 2021

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
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests