Unnecessary padding in struct field auto layout

[acaly]

Description

object and nint should have identical size and alignement when used as fields. However, in some cases, changing an object field into nint changes the struct size. This problem is also related to other fields in the struct.

public class Program
{
    [StructLayout(LayoutKind.Sequential)]
    public struct Empty
    {
    }
    [StructLayout(LayoutKind.Sequential)]
    public struct WrappedPointer1
    {
        public nint Ptr;
    }
    [StructLayout(LayoutKind.Sequential)]
    public struct WrappedPointer2
    {
        public object Ptr;
    }
    [StructLayout(LayoutKind.Sequential)]
    public struct TestSize1 //16
    {
        public WrappedPointer1 A; //struct { nint }
        public int B;
        public Empty C;
        public Empty D;
    }
    [StructLayout(LayoutKind.Sequential)]
    public struct TestSize2 //32
    {
        public WrappedPointer2 A; //struct { object }
        public int B;
        public Empty C;
        public Empty D;
    }
    [StructLayout(LayoutKind.Sequential)]
    public struct TestSize3 //16
    {
        public WrappedPointer2 A; //struct { object }
        public int B;
        public byte C;
        public byte D;
    }
    [StructLayout(LayoutKind.Sequential)]
    public struct TestSize4 //32
    {
        public object A;
        public int B;
        public Empty C;
        public Empty D;
    }

    public static unsafe void Main()
    {
        Console.WriteLine(Unsafe.SizeOf<nint>()); //8
        Console.WriteLine(Unsafe.SizeOf<WrappedPointer1>()); //8
        Console.WriteLine(Unsafe.SizeOf<WrappedPointer2>()); //8
        Console.WriteLine(Unsafe.SizeOf<TestSize1>()); //16
        Console.WriteLine(Unsafe.SizeOf<TestSize2>()); //32
        Console.WriteLine(Unsafe.SizeOf<TestSize3>()); //16
        Console.WriteLine(Unsafe.SizeOf<TestSize4>()); //32

        Console.WriteLine();

        TestSize1 t1 = default;
        Console.WriteLine((nint)Unsafe.AsPointer(ref t1.A) - (nint)Unsafe.AsPointer(ref t1)); //0
        Console.WriteLine((nint)Unsafe.AsPointer(ref t1.B) - (nint)Unsafe.AsPointer(ref t1)); //8
        Console.WriteLine((nint)Unsafe.AsPointer(ref t1.C) - (nint)Unsafe.AsPointer(ref t1)); //12
        Console.WriteLine((nint)Unsafe.AsPointer(ref t1.D) - (nint)Unsafe.AsPointer(ref t1)); //13

        TestSize4 t4 = default;
        Console.WriteLine((nint)Unsafe.AsPointer(ref t4.A) - (nint)Unsafe.AsPointer(ref t4)); //0
        Console.WriteLine((nint)Unsafe.AsPointer(ref t4.B) - (nint)Unsafe.AsPointer(ref t4)); //8
        Console.WriteLine((nint)Unsafe.AsPointer(ref t4.C) - (nint)Unsafe.AsPointer(ref t4)); //16
        Console.WriteLine((nint)Unsafe.AsPointer(ref t4.D) - (nint)Unsafe.AsPointer(ref t4)); //24
    }
}

On 64-bit platform, TestSize1-4 should all have the same size of 16 bytes (2 pointers), but some extend into 32 bytes.

Configuration

Windows 10.0.19043.1165 (21H1/May2021Update)
Intel Core i7-7700HQ CPU 2.80GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET SDK=6.0.100-preview.7.21379.14

Regression?

.NET 5 shows the same results.

[dotnet-issue-labeler]

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.

[tannergooding]

nint and object are both 4 or 8 bytes, but they are not considered equivalent or interchangeable. In particular, nint is a blittable primitive type, it is simply a pointer-sized integer. object on the other hand is a managed (GC tracked) reference, it is not considered blittable.

The remarks section of the StructLayoutAttribute covers this scenario and elaborates that LayoutKind.Sequential only controls the layout for managed memory if the type is considered blittable. For non-blittable types (such as object, string, T[], bool, sometimes char, and others), instead it only controls the layout when the class or structure is marshaled to unmanaged code (and therefore what Marshal.SizeOf will return), it does not control the layout of the class or structure in managed memory (and therefore what Unsafe.SizeOf will return).

You can use LayoutKind.Explicit to control the layout of memory for non-blittable types, but there are rules about managed references and other types overlapping and potentially other quirks since these types are variable sized.

[tannergooding]

In particular, the managed layout for nested value type fields looks to force an alignment of TARGET_POINTER_SIZE: https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/methodtablebuilder.cpp#L8184-L8194

#if !defined(TARGET_64BIT) && (DATA_ALIGNMENT > 4)
                dwCumulativeInstanceFieldPos = (DWORD)ALIGN_UP(dwCumulativeInstanceFieldPos,
                    (pByValueMT->GetNumInstanceFieldBytes() >= DATA_ALIGNMENT) ? DATA_ALIGNMENT : TARGET_POINTER_SIZE);
#else // !(!defined(TARGET_64BIT) && (DATA_ALIGNMENT > 4))
#ifdef FEATURE_64BIT_ALIGNMENT
                if (pByValueMT->RequiresAlign8())
                    dwCumulativeInstanceFieldPos = (DWORD)ALIGN_UP(dwCumulativeInstanceFieldPos, 8);
                else
#endif // FEATURE_64BIT_ALIGNMENT
                    dwCumulativeInstanceFieldPos = (DWORD)ALIGN_UP(dwCumulativeInstanceFieldPos, TARGET_POINTER_SIZE);
#endif // !(!defined(TARGET_64BIT) && (DATA_ALIGNMENT > 4))

@jkotas, do you know why we do this rather than looking up the EEClassLayoutInfo from the MethodTable* (assuming HasLayout() returns true) and getting the computed alignment required (which already should have taken managed reference requirements into account)?

[jkotas]

This looks similar to #12977.

The field layout has number of corner cases that are either inefficient or too complex for no good reason.

@trylek tried to clean it up in #51416, but we run out of time. If you touch field layout, a lot of random things start falling apart. I hope we will get it done in .NET 7.

[mangod9]

@trylek @jkoritzinsky assume this is fixed as part of the type layout work in 7?

[jkoritzinsky]

This has been mitigated as part of the layout work, but it hasn't entirely been fixed.

Here's the report from ObjectLayoutInspector from Preview 6:

Details

Type layout for 'TestSize1'
Size: 16 bytes. Paddings: 2 bytes (%12 of empty space)
|====================================|
|   0-7: WrappedPointer1 A (8 bytes) |
| |=============================|    |
| |   0-7: IntPtr Ptr (8 bytes) |    |
| |=============================|    |
|------------------------------------|
|  8-11: Int32 B (4 bytes)           |
|------------------------------------|
|    12: Empty C (1 byte)            |
|------------------------------------|
|    13: Empty D (1 byte)            |
|------------------------------------|
| 14-15: padding (2 bytes)           |
|====================================|


Type layout for 'TestSize2'
Size: 24 bytes. Paddings: 10 bytes (%41 of empty space)
|====================================|
|   0-3: Int32 B (4 bytes)           |
|------------------------------------|
|   4-7: padding (4 bytes)           |
|------------------------------------|
|  8-15: WrappedPointer2 A (8 bytes) |
| |=============================|    |
| |   0-7: Object Ptr (8 bytes) |    |
| |=============================|    |
|------------------------------------|
|    16: Empty C (1 byte)            |
|------------------------------------|
|    17: Empty D (1 byte)            |
|------------------------------------|
| 18-23: padding (6 bytes)           |
|====================================|


Type layout for 'TestSize3'
Size: 16 bytes. Paddings: 2 bytes (%12 of empty space)
|====================================|
|   0-3: Int32 B (4 bytes)           |
|------------------------------------|
|     4: Byte C (1 byte)             |
|------------------------------------|
|     5: Byte D (1 byte)             |
|------------------------------------|
|   6-7: padding (2 bytes)           |
|------------------------------------|
|  8-15: WrappedPointer2 A (8 bytes) |
| |=============================|    |
| |   0-7: Object Ptr (8 bytes) |    |
| |=============================|    |
|====================================|


Type layout for 'TestSize4'
Size: 16 bytes. Paddings: 2 bytes (%12 of empty space)
|===========================|
|   0-7: Object A (8 bytes) |
|---------------------------|
|  8-11: Int32 B (4 bytes)  |
|---------------------------|
|    12: Empty C (1 byte)   |
|---------------------------|
|    13: Empty D (1 byte)   |
|---------------------------|
| 14-15: padding (2 bytes)  |
|===========================|

As you can see, TestSize4 is now 16 bytes, and TestSize2 has shrunk to 24 bytes. TestSize2 is still 24 bytes because of how the auto-layout algorithm handles non-primitive struct types. The auto-layout algorithm always handles non-primitive value types last and places them in metadata order at the end of the structure. This results in the TestSize2 struct being larger because even empty structs are 1 byte wide, so there ends up being quite a bit of padding at the end of the structure to meet the alignment requirements in arrays.

Changing how the auto-layout algorithm handles value types of small sizes is likely a larger change (and more of a feature than a bug), so I don't know if we should try to fix this in .NET 7 given how late we are in the release.

[trylek]

While @jkotas previously suggested we should strive to fix as many issues of this type as we can in .NET 7, changing the auto layout now sounds pretty scary to me (not mentioning it's a coordinated change between CoreCLR runtime and Crossgen2 that also requires bumping up Crossgen2 major version).

[jkotas]

I agree that this looks like a feature, not a bug fix. We can get to it during the next field layout overhaul.