diff --git a/Public/Src/Pips/Dll/Operations/EnvironmentVariable.cs b/Public/Src/Pips/Dll/Operations/EnvironmentVariable.cs index 3509333895..cd117bb870 100644 --- a/Public/Src/Pips/Dll/Operations/EnvironmentVariable.cs +++ b/Public/Src/Pips/Dll/Operations/EnvironmentVariable.cs @@ -12,21 +12,55 @@ namespace BuildXL.Pips.Operations /// public readonly struct EnvironmentVariable : IEquatable { + // 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; + /// /// Name of the variable. /// - public readonly StringId Name; + public StringId Name => StringId.UnsafeCreateFrom(m_nameValue); /// /// Value of the variable. /// - 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)); + } + } /// /// Whether this is a pass-through environment variable /// - public readonly bool IsPassThrough; - + public bool IsPassThrough { get; } + /// /// Creates an environment variable definition. /// @@ -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; } diff --git a/Public/Src/Pips/Dll/Operations/PipData.cs b/Public/Src/Pips/Dll/Operations/PipData.cs index a92bd28e49..9e3fc39c44 100644 --- a/Public/Src/Pips/Dll/Operations/PipData.cs +++ b/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) { diff --git a/Public/Src/Pips/Dll/Operations/PipDataBuilder.cs b/Public/Src/Pips/Dll/Operations/PipDataBuilder.cs index 1687c17e96..b3fab6b5ee 100644 --- a/Public/Src/Pips/Dll/Operations/PipDataBuilder.cs +++ b/Public/Src/Pips/Dll/Operations/PipDataBuilder.cs @@ -15,7 +15,7 @@ namespace BuildXL.Pips.Operations /// /// /// Representation of PipData is now a sequence of values. - /// Each value is comprised of a single byte code and a 4 byte integer . + /// Each value is comprised of a single byte code and a 4 byte integer . /// The code stores the and optionally the . /// /// Entry types and interpretation: diff --git a/Public/Src/Pips/Dll/Operations/PipDataEntry.cs b/Public/Src/Pips/Dll/Operations/PipDataEntry.cs index cce9173292..92ebd8a611 100644 --- a/Public/Src/Pips/Dll/Operations/PipDataEntry.cs +++ b/Public/Src/Pips/Dll/Operations/PipDataEntry.cs @@ -24,8 +24,11 @@ namespace BuildXL.Pips.Operations /// public PipDataEntryType EntryType { get; } - private readonly PipDataFragmentEscaping m_escaping; - private readonly int m_data; + /// + internal readonly PipDataFragmentEscaping RawEscaping; + + /// + internal readonly int RawData; /// 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; } /// @@ -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); } /// @@ -117,7 +120,7 @@ public int GetIntegralValue() EntryType == PipDataEntryType.NestedDataEnd || EntryType == PipDataEntryType.VsoHashEntry2RewriteCount || EntryType == PipDataEntryType.FileId2RewriteCount); - return m_data; + return RawData; } /// @@ -131,7 +134,7 @@ public uint GetUInt32Value() { Contract.Requires( EntryType == PipDataEntryType.DirectoryIdHeaderSealId); - return unchecked((uint)m_data); + return unchecked((uint)RawData); } /// @@ -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 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) /// public override int GetHashCode() { - return HashCodeHelper.Combine((int)EntryType, (int)m_escaping, m_data); + return HashCodeHelper.Combine((int)EntryType, (int)RawEscaping, RawData); } /// @@ -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) diff --git a/Public/Src/Pips/UnitTests/Pips/EnvironmentVariableLayoutTests.cs b/Public/Src/Pips/UnitTests/Pips/EnvironmentVariableLayoutTests.cs new file mode 100644 index 0000000000..cd595e4868 --- /dev/null +++ b/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(); + _output.WriteLine(layout.ToString()); + + Assert.Equal(32, layout.Size); + var oldLayout = ObjectLayoutInspector.TypeLayout.GetLayout(); + _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); + } +} diff --git a/Public/Src/Pips/UnitTests/Pips/Test.BuildXL.Pips.dsc b/Public/Src/Pips/UnitTests/Pips/Test.BuildXL.Pips.dsc index 1a082b43f1..b72db96acd 100644 --- a/Public/Src/Pips/UnitTests/Pips/Test.BuildXL.Pips.dsc +++ b/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, diff --git a/cg/nuget/cgmanifest.json b/cg/nuget/cgmanifest.json index 2f0d7ddc1f..2e8979e628 100644 --- a/cg/nuget/cgmanifest.json +++ b/cg/nuget/cgmanifest.json @@ -2809,6 +2809,15 @@ } } }, + { + "Component": { + "Type": "NuGet", + "NuGet": { + "Name": "ObjectLayoutInspector", + "Version": "0.1.4" + } + } + }, { "Component": { "Type": "NuGet", diff --git a/config.dsc b/config.dsc index f5ee29bc26..6506d7b974 100644 --- a/config.dsc +++ b/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" },