Merged PR 680832: [BuildXL Perf] Reduce the size of EnvironmentVariab…

…le struct from 48 to 32

An array of `EnvironmentVariable` structs is created per pip and such arrays occupy quite a bit of memory. Here is an example from bxl dump for COSINE:

```
305412   2828539152 BuildXL.Pips.Operations.EnvironmentVariable[]
```

I.e. the arrays are almost 3Gb.

This PR reduces the size of `EnvironmentVariable` struct from 48 to 32 bytes by flattening types used in it.

It also adds a reference to `ObjectLayoutInspector` nuget package that allows inspecting the layout at runtime.

The layout before:
![image (2).png](https://dev.azure.com/mseng/9ed2c125-1cd5-4a17-886b-9d267f3a5fab/_apis/git/repositories/50d331c7-ea65-45eb-833f-0303c6c2387e/pullRequests/680832/attachments/image%20%282%29.png)

Here is the layout after:
![image.png](https://dev.azure.com/mseng/9ed2c125-1cd5-4a17-886b-9d267f3a5fab/_apis/git/repositories/50d331c7-ea65-45eb-833f-0303c6c2387e/pullRequests/680832/attachments/image.png)

Public/Src/Pips/Dll/Operations/EnvironmentVariable.cs

@@ -12,21 +12,55 @@ namespace BuildXL.Pips.Operations
     /// </summary>
     public readonly struct EnvironmentVariable : IEquatable<EnvironmentVariable>
     {
+        // This struct used to have the following structure:
+        // readonly record struct EnvironmentVariable(StringId Name, PipData Value, bool IsPassThrough);
+        // Unfortunately, due to layout issues, the old version was 48 bytes in size even though we can manually
+        // pack all the required data into 32 bytes.
+
+        // StringId
+        private readonly int m_nameValue;
+
+        // PipData
+        private readonly bool m_pipDataAvailable;
+
+        // PipData.EntriesBinarySegmentPointer
+        private readonly int m_pipDataEntriesBinarySegmentPointer;
+        // PipData.HeaderEntry
+        private readonly PipDataEntryType m_pipDataEntryType;
+        private readonly PipDataFragmentEscaping m_pipDataEscaping;
+        private readonly int m_pipDataValue;
+        // PipData.Entries
+        private readonly PipDataEntryList m_pipDataEntries;
+
         /// <summary>
         /// Name of the variable.
         /// </summary>
-        public readonly StringId Name;
+        public StringId Name => StringId.UnsafeCreateFrom(m_nameValue);
 
         /// <summary>
         /// Value of the variable.
         /// </summary>
-        public readonly PipData Value;
+        public PipData Value
+        {
+            get
+            {
+                if (!m_pipDataAvailable)
+                {
+                    return PipData.Invalid;
+                }
+
+                return PipData.CreateInternal(
+                    new PipDataEntry(m_pipDataEscaping, m_pipDataEntryType, m_pipDataValue),
+                    m_pipDataEntries,
+                    StringId.UnsafeCreateFrom(m_pipDataEntriesBinarySegmentPointer));
+            }
+        }
 
         /// <summary>
         /// Whether this is a pass-through environment variable
         /// </summary>
-        public readonly bool IsPassThrough;
-
+        public bool IsPassThrough { get; }
+        
         /// <summary>
         /// Creates an environment variable definition.
         /// </summary>
@@ -35,8 +69,26 @@ public EnvironmentVariable(StringId name, PipData value, bool isPassThrough = fa
             Contract.Requires(name.IsValid);
             Contract.Requires(value.IsValid || isPassThrough);
 
-            Name = name;
-            Value = value;
+            m_nameValue = name.Value;
+            if (value.IsValid)
+            {
+                m_pipDataAvailable = true;
+                m_pipDataEntriesBinarySegmentPointer = value.EntriesBinarySegmentPointer.Value;
+                m_pipDataEntryType = value.HeaderEntry.EntryType;
+                m_pipDataEscaping = value.HeaderEntry.RawEscaping;
+                m_pipDataValue = value.HeaderEntry.RawData;
+                m_pipDataEntries = value.Entries;
+            }
+            else
+            {
+                m_pipDataAvailable = false;
+                m_pipDataEntriesBinarySegmentPointer = default;
+                m_pipDataEntryType = default;
+                m_pipDataEscaping = default;
+                m_pipDataValue = default;
+                m_pipDataEntries = default;
+            }
+
             IsPassThrough = isPassThrough;
         }
 

Public/Src/Pips/Dll/Operations/PipData.cs

@@ -59,6 +59,7 @@ namespace BuildXL.Pips.Operations
 
         internal readonly PipDataEntry HeaderEntry;
         internal readonly PipDataEntryList Entries;
+        internal StringId EntriesBinarySegmentPointer => m_entriesBinarySegmentPointer;
 
         private PipData(PipDataEntry entry, PipDataEntryList entries, StringId entriesBinarySegmentPointer)
         {

Public/Src/Pips/Dll/Operations/PipDataBuilder.cs

@@ -15,7 +15,7 @@ namespace BuildXL.Pips.Operations
     /// </summary>
     /// <remarks>
     /// Representation of PipData is now a sequence of <see cref="PipDataEntry"/> values.
-    /// Each value is comprised of a single byte code and a 4 byte integer <see cref="PipDataEntry.m_data"/>.
+    /// Each value is comprised of a single byte code and a 4 byte integer <see cref="PipDataEntry.RawData"/>.
     /// The code stores the <see cref="PipDataEntryType"/> and optionally the <see cref="PipDataFragmentEscaping"/>.
     ///
     /// Entry types and interpretation:

Public/Src/Pips/Dll/Operations/PipDataEntry.cs

@@ -24,8 +24,11 @@ namespace BuildXL.Pips.Operations
         /// </summary>
         public PipDataEntryType EntryType { get; }
 
-        private readonly PipDataFragmentEscaping m_escaping;
-        private readonly int m_data;
+        /// <nodoc />
+        internal readonly PipDataFragmentEscaping RawEscaping;
+
+        /// <nodoc />
+        internal readonly int RawData;
 
         /// <nodoc />
         public PipDataEntry(PipDataFragmentEscaping escaping, PipDataEntryType entryType, uint data)
@@ -40,8 +43,8 @@ public PipDataEntry(PipDataFragmentEscaping escaping, PipDataEntryType entryType
         {
             Contract.Requires(escaping == PipDataFragmentEscaping.Invalid || entryType == PipDataEntryType.NestedDataHeader);
             EntryType = entryType;
-            m_escaping = escaping;
-            m_data = data;
+            RawEscaping = escaping;
+            RawData = data;
         }
 
         /// <summary>
@@ -55,7 +58,7 @@ public PipDataFragmentEscaping Escaping
             get
             {
                 Contract.Requires(EntryType == PipDataEntryType.NestedDataHeader);
-                return m_escaping;
+                return RawEscaping;
             }
         }
 
@@ -99,7 +102,7 @@ public PipFragmentType FragmentType
         public AbsolutePath GetPathValue()
         {
             Contract.Requires(EntryType == PipDataEntryType.AbsolutePath || EntryType == PipDataEntryType.VsoHashEntry1Path || EntryType == PipDataEntryType.FileId1Path);
-            return new AbsolutePath(m_data);
+            return new AbsolutePath(RawData);
         }
 
         /// <summary>
@@ -117,7 +120,7 @@ public int GetIntegralValue()
                 EntryType == PipDataEntryType.NestedDataEnd ||
                 EntryType == PipDataEntryType.VsoHashEntry2RewriteCount ||
                 EntryType == PipDataEntryType.FileId2RewriteCount);
-            return m_data;
+            return RawData;
         }
 
         /// <summary>
@@ -131,7 +134,7 @@ public uint GetUInt32Value()
         {
             Contract.Requires(
                 EntryType == PipDataEntryType.DirectoryIdHeaderSealId);
-            return unchecked((uint)m_data);
+            return unchecked((uint)RawData);
         }
 
         /// <summary>
@@ -148,7 +151,7 @@ public StringId GetStringValue()
                 EntryType == PipDataEntryType.StringLiteral ||
                 EntryType == PipDataEntryType.NestedDataHeader ||
                 EntryType == PipDataEntryType.IpcMoniker);
-            return new StringId(m_data);
+            return new StringId(RawData);
         }
 
         #region Conversions
@@ -244,8 +247,8 @@ public static implicit operator PipDataEntry(AbsolutePath data)
 
         public void Write(byte[] bytes, ref int index)
         {
-            bytes[index++] = checked((byte)(((int)EntryType << 4) | (int)m_escaping));
-            Bits.WriteInt32(bytes, ref index, m_data);
+            bytes[index++] = checked((byte)(((int)EntryType << 4) | (int)RawEscaping));
+            Bits.WriteInt32(bytes, ref index, RawData);
         }
 
         public static PipDataEntry Read<TBytes>(TBytes bytes, ref int index)
@@ -264,28 +267,28 @@ public void Serialize(BuildXLWriter writer)
         {
             Contract.Requires(writer != null);
             Contract.Assert((int)EntryType < 16);
-            Contract.Assert((int)m_escaping < 16);
-            writer.Write(checked((byte)(((int)EntryType << 4) | (int)m_escaping)));
+            Contract.Assert((int)RawEscaping < 16);
+            writer.Write(checked((byte)(((int)EntryType << 4) | (int)RawEscaping)));
             switch (EntryType)
             {
                 case PipDataEntryType.NestedDataHeader:
                 case PipDataEntryType.StringLiteral:
-                    writer.Write(new StringId(m_data));
+                    writer.Write(new StringId(RawData));
                     break;
                 case PipDataEntryType.NestedDataStart:
                 case PipDataEntryType.NestedDataEnd:
                 case PipDataEntryType.VsoHashEntry2RewriteCount:
                 case PipDataEntryType.FileId2RewriteCount:
                 case PipDataEntryType.DirectoryIdHeaderSealId:
-                    writer.WriteCompact(m_data);
+                    writer.WriteCompact(RawData);
                     break;
                 case PipDataEntryType.AbsolutePath:
                 case PipDataEntryType.VsoHashEntry1Path:
                 case PipDataEntryType.FileId1Path:
-                    writer.Write(new AbsolutePath(m_data));
+                    writer.Write(new AbsolutePath(RawData));
                     break;
                 case PipDataEntryType.IpcMoniker:
-                    writer.Write(new StringId(m_data));
+                    writer.Write(new StringId(RawData));
                     break;
                 default:
                     Contract.Assert(false, "EntryType not handled: " + EntryType);
@@ -340,7 +343,7 @@ public static PipDataEntry Deserialize(BuildXLReader reader)
         /// <inheritdoc />
         public override int GetHashCode()
         {
-            return HashCodeHelper.Combine((int)EntryType, (int)m_escaping, m_data);
+            return HashCodeHelper.Combine((int)EntryType, (int)RawEscaping, RawData);
         }
 
         /// <inheritdoc />
@@ -353,8 +356,8 @@ public override bool Equals(object obj)
         public bool Equals(PipDataEntry other)
         {
             return other.EntryType == EntryType &&
-                other.m_data == m_data &&
-                other.m_escaping == m_escaping;
+                other.RawData == RawData &&
+                other.RawEscaping == RawEscaping;
         }
 
         public static bool operator ==(PipDataEntry left, PipDataEntry right)

Public/Src/Pips/UnitTests/Pips/EnvironmentVariableLayoutTests.cs

@@ -0,0 +1,61 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+using BuildXL.Pips.Operations;
+using BuildXL.Utilities;
+using Test.BuildXL.TestUtilities.Xunit;
+using Xunit;
+using Xunit.Abstractions;
+
+namespace Test.BuildXL.Pips
+{
+    public sealed class EnvironmentVariableLayoutTests : XunitBuildXLTest
+    {
+        private readonly ITestOutputHelper _output;
+        public EnvironmentVariableLayoutTests(ITestOutputHelper output)
+            : base(output)
+        {
+            _output = output;
+        }
+
+        [Fact]
+        public void PipDataStoredCorrectly()
+        {
+            // EnvironmentVariable flattens PipData structure, so its important to make sure
+            // that if PipData structure changes EnvironmentVariable is changed as well.
+            // This test ensures the consistency.
+            var pipData = PipData.Invalid;
+            var envVar = new EnvironmentVariable(StringId.UnsafeCreateFrom(42), pipData, isPassThrough: true);
+            Assert.Equal(pipData, envVar.Value);
+
+            var pipDataEntry = new PipDataEntry(PipDataFragmentEscaping.NoEscaping, PipDataEntryType.NestedDataHeader, 42);
+            pipData = PipData.CreateInternal(
+                pipDataEntry,
+                PipDataEntryList.FromEntries(new[] {pipDataEntry}),
+                StringId.UnsafeCreateFrom(1));
+            envVar = new EnvironmentVariable(StringId.UnsafeCreateFrom(42), pipData, isPassThrough: false);
+            Assert.Equal(pipData, envVar.Value);
+        }
+
+        [Fact]
+        public void EnvironmentVariableSizeIs32()
+        {
+            // EnvironmentVariable structs are stored for every pip and reducing the size of such structs
+            // reasonably reduces the peak memory consumption.
+
+            // Flattening the layout saves 30% of space compared to the naive old version.
+            var layout = ObjectLayoutInspector.TypeLayout.GetLayout<EnvironmentVariable>();
+            _output.WriteLine(layout.ToString());
+
+            Assert.Equal(32, layout.Size);
+            var oldLayout = ObjectLayoutInspector.TypeLayout.GetLayout<OldEnvironmentVariable>();
+            _output.WriteLine(oldLayout.ToString());
+            Assert.True(layout.Size < oldLayout.Size);
+        }
+
+        // Using StringIdStable and not StringId, because StringId layout is different for debug builds.
+        private record struct OldEnvironmentVariable(StringIdStable Name, PipData Value, bool IsPassThrough);
+
+        private record struct StringIdStable(int Value);
+    }
+}

Public/Src/Pips/UnitTests/Pips/Test.BuildXL.Pips.dsc

@@ -7,6 +7,12 @@ namespace Core {
         assemblyName: "Test.BuildXL.Pips",
         sources: globR(d`.`, "*.cs"),
         references: [
+            ...addIf(
+                BuildXLSdk.isFullFramework,
+                NetFx.Netstandard.dll
+            ),
+
+            importFrom("ObjectLayoutInspector").pkg,
             importFrom("BuildXL.Cache.ContentStore").Hashing.dll,
             importFrom("BuildXL.Cache.ContentStore").UtilitiesCore.dll,
             importFrom("BuildXL.Cache.ContentStore").Interfaces.dll,

cg/nuget/cgmanifest.json

@@ -2809,6 +2809,15 @@
         }
       }
     },
+    {
+      "Component": {
+        "Type": "NuGet",
+        "NuGet": {
+          "Name": "ObjectLayoutInspector",
+          "Version": "0.1.4"
+        }
+      }
+    },
     {
       "Component": {
         "Type": "NuGet",

config.dsc

@@ -340,6 +340,8 @@ config({
 
                 { id: "SharpZipLib", version: "1.3.3" },
 
+                { id: "ObjectLayoutInspector", version: "0.1.4" },
+
                 // Ninja JSON graph generation helper
                 { id: "BuildXL.Tools.Ninjson", version: "0.0.6" },
                 { id: "BuildXL.Tools.AppHostPatcher", version: "1.0.0" },