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

Investigate using Auto layout for structs in the framework #64261

Open
GSPP opened this issue Jan 25, 2022 · 10 comments
Open

Investigate using Auto layout for structs in the framework #64261

GSPP opened this issue Jan 25, 2022 · 10 comments
Labels
area-System.Runtime.InteropServices help wanted [up-for-grabs] Good issue for external contributors tenet-performance Performance related issue
Milestone

Comments

@GSPP
Copy link

GSPP commented Jan 25, 2022

By default, the C# compiler marks structs [StructLayout(LayoutKind.Sequential)]. I believe the historic reason for this is that structs were assumed to be mainly for interop. Unfortunately, this can cause unnecessary padding (and possibly other inefficiencies?).

The default in the C# language cannot be changed because large bodies of existing code depend on this behavior.

Even if the struct itself does not benefit from Auto, the runtime can rearrange fields together with surrounding structs: #7720 (comment). It's therefore not enough to look at a struct definition to conclude that it is not beneficial to mark it Auto. Profitability depends on usage as well.

I wonder if it can be considered a performance best practice to routinely mark all non-interop structs as LayoutKind.Auto. This strategy does not seem to have a downside except for the code clutter that this brings.

Would it be worth adopting a policy in which all eligible structs are marked as Auto? Should there be an analyzer for that?

This policy would be in contrast to making case-by-case decisions. The benefit is simplicity and ease of code review. "If the attribute is missing, it's a bug."

I loaded all system assemblies and made lists for how the struct layout is currently set:

5.0.8 Auto.txt
5.0.8 Explicit.txt
5.0.8 Sequential.txt

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jan 25, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@stephentoub
Copy link
Member

stephentoub commented Jan 25, 2022

Are there specific cases where you see there'd be a tangible benefit? Many of the examples in your Sequential.txt are for interop and making them non-sequential would break the functionality. Many others in Sequential.txt have only a single field and wouldn't benefit from a different layout. Still others have all fields of the same type and there'd be no benefit to a different layout. Others are already in the optimal order. Still others are typically only used on the stack and a few extra bytes here or there on the stack aren't going to be meaningful. I appreciate your interest here, but just outputting a list of all structs isn't particularly actionable.

As for an analyzer, I expect it'd be hard to get it "right". Sequential is the closest there is to a "safe" option, in case the dev is doing something that expects that visible layout, whether that be interop or pointer-based access to fields or Unsafe casting, and without a lot of case law demonstrating it's worth forcing Auto to be used, I suspect it'd cause more harm than good, even if that "harm" is noise because there's no real benefit to the different layout being required.

@stephentoub stephentoub added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 25, 2022
@ghost
Copy link

ghost commented Jan 25, 2022

This issue has been marked needs-author-action since it may be missing important information. Please refer to our contribution guidelines for tips on how to report issues effectively.

@stephentoub stephentoub added area-System.Runtime.InteropServices and removed untriaged New issue has not been triaged by the area owner labels Jan 25, 2022
@jkotas jkotas added the tenet-performance Performance related issue label Jan 25, 2022
@GSPP
Copy link
Author

GSPP commented Jan 26, 2022

Right, I have no concrete reason to believe that any of these cases would benefit. I am not qualified to make that assessment. My thinking is more about changing policy on this. I generated the files just as a convenience for auditing the current state. It's not meant to be an actionable list.

... and without a lot of case law demonstrating it's worth forcing Auto to be used ...

That's probably the critical point here. Is there a systemic benefit, or is it just a small benefit here or there? You are far better able to tell than I am, so if you don't think there's anything to do here, I wouldn't mind closing the issue. I just wanted to bring this to your attention.

@ghost ghost added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Jan 26, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the Future milestone Jan 30, 2022
@AaronRobinsonMSFT
Copy link
Member

I am dubious of the value of providing this guidance and the optimizations it may enable are likely very small or niche. That being said this does seem like an area where we should have a concrete understanding of when packing non-interop structs is beneficial. I think an investigation is warranted, whether that is done by the community or Interop team is TBD so I'll place this in "Future".

@AaronRobinsonMSFT AaronRobinsonMSFT added help wanted [up-for-grabs] Good issue for external contributors and removed needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration labels Jan 30, 2022
@molinch
Copy link

molinch commented Oct 9, 2023

@stephentoub This issue brings another discussion: don't you think having an optional per project csproj configuration <DefaultStructLayout>Auto</DefaultStructLayout> could bring benefits while not bringing noise?

Not necessarily for the framework but developers most of the time these days don't do interop. And unsafe/pointer functionalities are really niche. IMO, these days, 99% of the time we want Auto Layout, while 1% of the time we want the opposite. Historically it wasn't like that, but time flies :)

@AaronRobinsonMSFT
Copy link
Member

IMO, these days, 99% of the time we want Auto Layout

This is the statement that @stephentoub is alluding to when he says "Are there specific cases where you see there'd be a tangible benefit?" Why is this the case? Is there real measurable benefit? Having concrete examples and traces that show an improvement would go a long way to substantiating that statement.

@molinch
Copy link

molinch commented Oct 9, 2023

Good point, I did some tests with BenchmarkDotNet, in term of speed in fact not specifying the layout is always a tad faster on my PC. That's for 1000000 iterations with 2 structs defined as:

[StructLayout(LayoutKind.Auto)]
public record struct DateAndSlotWithLayout(DateOnly Date, int Int, byte Byte);
public record struct DateAndSlotWithoutLayout(DateOnly Date, int Int, byte Byte);
Method Mean Error StdDev
StructWithLayout 244.5 μs 4.04 μs 3.78 μs
StructWithoutLayout 239.0 μs 2.49 μs 2.08 μs

So given this, please ignore my remark on this issue ;)

@MichalPetryka
Copy link
Contributor

I guess an important thing is that changing layout from Sequential (or Explicit) to Auto should be considered a binary breaking change in case some code relies on it.

@laicasaane
Copy link

In Unity, Burst compiler relies on Sequential layout and currently doesn't support Auto layout (I can't imagine how it should support Auto). And since Burst is going to be integrated deep into the core engine itself, this change will break the whole engine. In Unity, we need high performance really bad so unsafe is a must-have. But we don't need to utilize unsafe by ourselves since there is Burst compiler to lift the heavy work. Finally, I would like to remind you that Unity is not a niche ecosystem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Runtime.InteropServices help wanted [up-for-grabs] Good issue for external contributors tenet-performance Performance related issue
Projects
Status: No status
Development

No branches or pull requests

7 participants