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

Why Nullable<byte> is aligned to 8 bytes instead of 2 bytes as short does? #12977

Closed
yyjdelete opened this issue Jun 26, 2019 · 7 comments · Fixed by #61759
Closed

Why Nullable<byte> is aligned to 8 bytes instead of 2 bytes as short does? #12977

yyjdelete opened this issue Jun 26, 2019 · 7 comments · Fixed by #61759
Assignees
Labels
Milestone

Comments

@yyjdelete
Copy link

Use System.Runtime.CompilerServices.Unsafe.SizeOf<T>() and ValueTuple to do the test, seems it's always the same on different version of .net.(x86 aligned to 4 bytes)
Don't know why it need such a big padding, and use 3 more times(1 more time for x86) memory than it actually needed.

Type Length
byte? 2
short 2
ValueTuple<byte?> 2
ValueTuple 2
ValueTuple<short, short> 4
ValueTuple<byte?, byte?> 16
ValueTuple<short, short, short> 8
ValueTuple<byte?, byte?, byte?> 24
@MichalStrehovsky
Copy link
Member

This is caused by ValueType<T1,T2> being marked as [StructLayout(LayoutKind.Auto)]. If it was Sequential, the size of ValueTuple<byte?, byte?> would probably be 4 bytes.

I don't know off the top of my head whether this is expected.

@stephentoub
Copy link
Member

stephentoub commented Jun 26, 2019

This is caused by ValueType<T1,T2> being marked as [StructLayout(LayoutKind.Auto)]. If it was Sequential

Really? I've always thought of Auto as being "let the runtime optimize the layout by ordering the fields optimally", in which case I'd expect Auto to always end up being as small or smaller than Sequential. Why would Auto cause this?

using System;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

class Program
{
    static void Main()
    {
        Console.WriteLine("Auto:       " + Unsafe.SizeOf<AutoWrapper>());
        Console.WriteLine("Sequential: " + Unsafe.SizeOf<SequentialWrapper>());
    }
}

[StructLayout(LayoutKind.Auto)]
internal struct AutoWrapper { public byte? One, Two; }

[StructLayout(LayoutKind.Sequential)]
internal struct SequentialWrapper { public byte? One, Two; }

outputs on 64-bit:

Auto:       16
Sequential: 4

EDIT: Fixed typos.

@MichalStrehovsky
Copy link
Member

The test program is looking at AutoWrapper twice. This is the expected output:

Auto:       16
Sequential: 4

@stephentoub
Copy link
Member

stephentoub commented Jun 26, 2019

Argh, sorry, copy and paste error. You're right. Fixed.

@stephentoub
Copy link
Member

Which begs the question... why is that the case?

@benaadams
Copy link
Member

Doesn't event need fields to be 16 which is weird?

using System;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

class Program
{
    static void Main()
    {
        Console.WriteLine("Auto:       " + Unsafe.SizeOf<AutoWrapper>());
        Console.WriteLine("Sequential: " + Unsafe.SizeOf<SequentialWrapper>());
    }
}

[StructLayout(LayoutKind.Auto)]
internal struct AutoWrapper { public Nullish<byte> One, Two; }

[StructLayout(LayoutKind.Sequential)]
internal struct SequentialWrapper { public Nullish<byte> One, Two; }

[StructLayout(LayoutKind.Auto)]
internal struct Nullish<T> where T : struct
{
    // No fields
}

Produces

Auto:       16
Sequential: 16

Changing Nullish to

[StructLayout(LayoutKind.Sequential)]
internal struct Nullish<T> where T : struct
{
}

Produces

Auto:       16
Sequential: 2

And changing to

[StructLayout(LayoutKind.Sequential)]
internal struct Nullish<T> where T : struct
{
    T Value;
    bool HashValue;
}

Produces

Auto:       16
Sequential: 4

@MichalStrehovsky
Copy link
Member

I suspect it's this:

https://github.com/dotnet/coreclr/blob/ff02091612aa42f79e5d3adbd608b36da9f47fdb/src/vm/methodtablebuilder.cpp#L8077-L8097

It's odd that we don't just pByValueMT->ContainsPointers() instead of speculating about "could have GC pointers". We could just try to do that padding conditionally on ContainsPointers() and see how much it breaks the CI.

The CLR has had this code there since at least 2007 (that's where the TFS history cuts off).

Warpten referenced this issue in Warpten/DBClientFiles.NET Sep 28, 2019
@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@mangod9 mangod9 modified the milestones: Future, 6.0.0 Sep 18, 2020
@mangod9 mangod9 modified the milestones: 6.0.0, Future Jul 26, 2021
@jkotas jkotas modified the milestones: Future, 6.0.0, 7.0.0 Sep 25, 2021
jkoritzinsky added a commit to jkoritzinsky/runtime that referenced this issue Dec 8, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 9, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Dec 15, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jan 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
8 participants