From 776ec7a2769b8e809a9023914b06e1e3b82878dd Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Thu, 18 Apr 2024 09:42:53 +1000 Subject: [PATCH 01/29] Remove redundant GetHashCode calls For Int32, GetHashCode just returns the value directly. --- src/Build/ElementLocation/ElementLocation.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index c9bf42b48b2..aec7f3e66fe 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -91,7 +91,7 @@ public static ElementLocation EmptyLocation public override int GetHashCode() { // Line and column are good enough - return Line.GetHashCode() ^ Column.GetHashCode(); + return Line ^ Column; } /// From 37def1127d383e5c8acd6de4514fbe4aab5afdf4 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Thu, 18 Apr 2024 09:44:52 +1000 Subject: [PATCH 02/29] Remove redundant null check --- src/Build/ElementLocation/ElementLocation.cs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index aec7f3e66fe..c193e27d0a0 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -100,11 +100,6 @@ public override int GetHashCode() /// public override bool Equals(object obj) { - if (obj == null) - { - return false; - } - IElementLocation that = obj as IElementLocation; if (that == null) From cb6ec145c6ef6dbfa8413a8702b8ded23682f205 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Thu, 18 Apr 2024 09:54:38 +1000 Subject: [PATCH 03/29] Update comment --- src/Build/ElementLocation/ElementLocation.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index c193e27d0a0..05652310c85 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -130,13 +130,13 @@ public override string ToString() /// /// Writes the packet to the serializer. - /// Always send as ints, even if ushorts are being used: otherwise it'd - /// need a byte to discriminate and the savings would be microscopic. /// void ITranslatable.Translate(ITranslator translator) { ErrorUtilities.VerifyThrow(translator.Mode == TranslationDirection.WriteToStream, "write only"); + // Translate int, even if ushort is being used. + // Internally, the translator uses a variable length (prefix) encoding. string file = File; int line = Line; int column = Column; From 093bee796c3626e1389cc31d570af83ac4571838 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Thu, 18 Apr 2024 10:02:22 +1000 Subject: [PATCH 04/29] Null annotate ElementLocation --- src/Build/ElementLocation/ElementLocation.cs | 14 ++++++-------- src/Shared/ErrorUtilities.cs | 4 +++- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index 05652310c85..1cd14fdd449 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -7,8 +7,6 @@ using Microsoft.Build.Collections; using Microsoft.Build.Shared; -#nullable disable - namespace Microsoft.Build.Construction { /// @@ -98,9 +96,9 @@ public override int GetHashCode() /// Override Equals so that identical /// fields imply equal objects. /// - public override bool Equals(object obj) + public override bool Equals(object? obj) { - IElementLocation that = obj as IElementLocation; + IElementLocation? that = obj as IElementLocation; if (that == null) { @@ -151,7 +149,7 @@ void ITranslatable.Translate(ITranslator translator) /// internal static ElementLocation FactoryForDeserialization(ITranslator translator) { - string file = null; + string? file = null; int line = 0; int column = 0; translator.Translate(ref file); @@ -180,7 +178,7 @@ internal static ElementLocation Create(string file) /// In AG there are 600 locations that have a file but zero line and column. /// In theory yet another derived class could be made for these to save 4 bytes each. /// - internal static ElementLocation Create(string file, int line, int column) + internal static ElementLocation Create(string? file, int line, int column) { if (string.IsNullOrEmpty(file) && line == 0 && column == 0) { @@ -247,7 +245,7 @@ private class RegularElementLocation : ElementLocation /// Numerical values must be 1-based, non-negative; 0 indicates unknown /// File may be null, indicating the file was not loaded from disk. /// - internal RegularElementLocation(string file, int line, int column) + internal RegularElementLocation(string? file, int line, int column) { ErrorUtilities.VerifyThrowArgumentLengthIfNotNull(file, nameof(file)); ErrorUtilities.VerifyThrow(line > -1 && column > -1, "Use zero for unknown"); @@ -323,7 +321,7 @@ private class SmallElementLocation : ElementLocation /// Numerical values must be 1-based, non-negative; 0 indicates unknown /// File may be null or empty, indicating the file was not loaded from disk. /// - internal SmallElementLocation(string file, int line, int column) + internal SmallElementLocation(string? file, int line, int column) { ErrorUtilities.VerifyThrow(line > -1 && column > -1, "Use zero for unknown"); ErrorUtilities.VerifyThrow(line <= 65535 && column <= 65535, "Use ElementLocation instead"); diff --git a/src/Shared/ErrorUtilities.cs b/src/Shared/ErrorUtilities.cs index 9bbd30e09c8..446e1918b25 100644 --- a/src/Shared/ErrorUtilities.cs +++ b/src/Shared/ErrorUtilities.cs @@ -537,17 +537,19 @@ internal static void VerifyThrowArgumentInvalidPath(string parameter, string par } } +#nullable enable /// /// Throws an ArgumentException if the string has zero length, unless it is /// null, in which case no exception is thrown. /// - internal static void VerifyThrowArgumentLengthIfNotNull(string parameter, string parameterName) + internal static void VerifyThrowArgumentLengthIfNotNull(string? parameter, string parameterName) { if (parameter?.Length == 0) { ThrowArgumentLength(parameterName); } } +#nullable disable /// /// Throws an ArgumentNullException if the given parameter is null. From 65ed4dd9a4a1a6d8e103bab9031d0e36335a7752 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Thu, 18 Apr 2024 10:15:04 +1000 Subject: [PATCH 05/29] Pack line and column values into four bytes The SmallElementLocation class exists because very few element locations require 32 bits to store the line/column values. It uses ushort (2 bytes) instead of int (4 bytes) for each value, in an attempt to reduce memory usage. However the CLR aligns ushort fields on classes at four-byte boundaries on most (all?) architectures, meaning the optimisation has no effect. This commit explicitly packs the two values into four bytes to ensure that four bytes is saved per instance. --- src/Build/ElementLocation/ElementLocation.cs | 22 ++++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index 1cd14fdd449..3d64e80006e 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -307,14 +307,15 @@ private class SmallElementLocation : ElementLocation private string file; /// - /// The source line. - /// - private ushort line; - - /// - /// The source column. + /// Packs both the line and column values into a single four-byte element. + /// The high two bytes are the line, and low two bytes are the column. /// - private ushort column; + /// + /// If we had two fields, the CLR would pad them each to + /// four-byte boundaries, meaning no space would actually be saved here. + /// So instead, we pack them manually. + /// + private int packedData; /// /// Constructor for the case where we have most or all information. @@ -327,8 +328,7 @@ internal SmallElementLocation(string? file, int line, int column) ErrorUtilities.VerifyThrow(line <= 65535 && column <= 65535, "Use ElementLocation instead"); this.file = file ?? String.Empty; - this.line = Convert.ToUInt16(line); - this.column = Convert.ToUInt16(column); + packedData = (line << 16) | column; } /// @@ -351,7 +351,7 @@ public override string File [DebuggerBrowsable(DebuggerBrowsableState.Never)] public override int Line { - get { return (int)line; } + get { return (packedData >> 16) & 0xFFFF; } } /// @@ -362,7 +362,7 @@ public override int Line [DebuggerBrowsable(DebuggerBrowsableState.Never)] public override int Column { - get { return (int)column; } + get { return packedData & ushort.MaxValue; } } } } From bb30e4fc5ed9e991d1185d94af4de9662281a557 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Thu, 18 Apr 2024 11:04:15 +1000 Subject: [PATCH 06/29] Remove redundant validation The caller performs this validation already, and no other code can call this. Avoid some indirection and branching. --- src/Build/ElementLocation/ElementLocation.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index 3d64e80006e..35221c75422 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -325,7 +325,6 @@ private class SmallElementLocation : ElementLocation internal SmallElementLocation(string? file, int line, int column) { ErrorUtilities.VerifyThrow(line > -1 && column > -1, "Use zero for unknown"); - ErrorUtilities.VerifyThrow(line <= 65535 && column <= 65535, "Use ElementLocation instead"); this.file = file ?? String.Empty; packedData = (line << 16) | column; From 57e0d5b6fc3333a0922c582e24b827b63dadb773 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Thu, 18 Apr 2024 11:17:41 +1000 Subject: [PATCH 07/29] Simplify LocationString construction The compiler will generate slightly better code from this switch statement, in cases where either line or column is zero. --- src/Build/ElementLocation/ElementLocation.cs | 39 ++++++-------------- 1 file changed, 11 insertions(+), 28 deletions(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index 35221c75422..cfcc1fe3ca3 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -69,7 +69,17 @@ public abstract int Column /// public string LocationString { - get { return GetLocationString(File, Line, Column); } + get + { + int line = Line; + int column = Column; + return (line, column) switch + { + (not 0, not 0) => ResourceUtilities.FormatResourceStringIgnoreCodeAndKeyword("FileLocation", File, line, column), + (not 0, 0) => $"{File} ({line})", + _ => File, + }; + } } /// @@ -193,33 +203,6 @@ internal static ElementLocation Create(string? file, int line, int column) return new ElementLocation.RegularElementLocation(file, line, column); } - /// - /// The location in a form suitable for replacement - /// into a message. - /// Example: "c:\foo\bar.csproj (12,34)" - /// Calling this creates and formats a new string. - /// PREFER TO PUT THE LOCATION INFORMATION AT THE START OF THE MESSAGE INSTEAD. - /// Only in rare cases should the location go within the message itself. - /// - private static string GetLocationString(string file, int line, int column) - { - string locationString; - if (line != 0 && column != 0) - { - locationString = ResourceUtilities.FormatResourceStringIgnoreCodeAndKeyword("FileLocation", file, line, column); - } - else if (line != 0) - { - locationString = file + " (" + line + ")"; - } - else - { - locationString = file; - } - - return locationString; - } - /// /// Rarer variation for when the line and column won't each fit in a ushort. /// From 80c1ea2403063c0447a5b13dc94bb3239e352182 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Thu, 18 Apr 2024 11:18:43 +1000 Subject: [PATCH 08/29] Consolidate validation There was inconsistent handling of validation between implementations. This moves it all into the `Create` method so that it can be handled in one place, consistently. --- src/Build/ElementLocation/ElementLocation.cs | 23 ++++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index cfcc1fe3ca3..cf683465673 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -23,7 +23,7 @@ public abstract class ElementLocation : IElementLocation, ITranslatable, IImmuta /// /// The singleton empty element location. /// - private static ElementLocation s_emptyElementLocation = new SmallElementLocation(null, 0, 0); + private static ElementLocation s_emptyElementLocation = new SmallElementLocation("", 0, 0); /// /// The file from which this particular element originated. It may @@ -195,6 +195,10 @@ internal static ElementLocation Create(string? file, int line, int column) return EmptyLocation; } + ErrorUtilities.VerifyThrow(line > -1 && column > -1, "Use zero for unknown"); + + file ??= ""; + if (line <= 65535 && column <= 65535) { return new ElementLocation.SmallElementLocation(file, line, column); @@ -226,14 +230,11 @@ private class RegularElementLocation : ElementLocation /// /// Constructor for the case where we have most or all information. /// Numerical values must be 1-based, non-negative; 0 indicates unknown - /// File may be null, indicating the file was not loaded from disk. + /// File may be empty, indicating the file was not loaded from disk. /// - internal RegularElementLocation(string? file, int line, int column) + internal RegularElementLocation(string file, int line, int column) { - ErrorUtilities.VerifyThrowArgumentLengthIfNotNull(file, nameof(file)); - ErrorUtilities.VerifyThrow(line > -1 && column > -1, "Use zero for unknown"); - - this.file = file ?? String.Empty; + this.file = file; this.line = line; this.column = column; } @@ -303,13 +304,11 @@ private class SmallElementLocation : ElementLocation /// /// Constructor for the case where we have most or all information. /// Numerical values must be 1-based, non-negative; 0 indicates unknown - /// File may be null or empty, indicating the file was not loaded from disk. + /// File may empty, indicating the file was not loaded from disk. /// - internal SmallElementLocation(string? file, int line, int column) + internal SmallElementLocation(string file, int line, int column) { - ErrorUtilities.VerifyThrow(line > -1 && column > -1, "Use zero for unknown"); - - this.file = file ?? String.Empty; + this.file = file; packedData = (line << 16) | column; } From 3ea9a86bc0421570ee30c45a0e81b2822b8c2034 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Thu, 18 Apr 2024 11:19:13 +1000 Subject: [PATCH 09/29] Simplify names (IDE0001) --- src/Build/ElementLocation/ElementLocation.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index cf683465673..528c5facd81 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -201,10 +201,10 @@ internal static ElementLocation Create(string? file, int line, int column) if (line <= 65535 && column <= 65535) { - return new ElementLocation.SmallElementLocation(file, line, column); + return new SmallElementLocation(file, line, column); } - return new ElementLocation.RegularElementLocation(file, line, column); + return new RegularElementLocation(file, line, column); } /// From 075ce08328937af3783f446ae6d337e9b2937f24 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Thu, 18 Apr 2024 11:22:24 +1000 Subject: [PATCH 10/29] Use auto properties --- src/Build/ElementLocation/ElementLocation.cs | 48 ++++---------------- 1 file changed, 8 insertions(+), 40 deletions(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index 528c5facd81..9c55a7720e2 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -212,21 +212,6 @@ internal static ElementLocation Create(string? file, int line, int column) /// private class RegularElementLocation : ElementLocation { - /// - /// The source file. - /// - private string file; - - /// - /// The source line. - /// - private int line; - - /// - /// The source column. - /// - private int column; - /// /// Constructor for the case where we have most or all information. /// Numerical values must be 1-based, non-negative; 0 indicates unknown @@ -234,9 +219,9 @@ private class RegularElementLocation : ElementLocation /// internal RegularElementLocation(string file, int line, int column) { - this.file = file; - this.line = line; - this.column = column; + File = file; + Line = line; + Column = column; } /// @@ -246,10 +231,7 @@ internal RegularElementLocation(string file, int line, int column) /// If not known, returns empty string. /// [DebuggerBrowsable(DebuggerBrowsableState.Never)] - public override string File - { - get { return file; } - } + public override string File { get; } /// /// The line number where this element exists in its file. @@ -257,10 +239,7 @@ public override string File /// Zero indicates "unknown location". /// [DebuggerBrowsable(DebuggerBrowsableState.Never)] - public override int Line - { - get { return line; } - } + public override int Line { get; } /// /// The column number where this element exists in its file. @@ -268,10 +247,7 @@ public override int Line /// Zero indicates "unknown location". /// [DebuggerBrowsable(DebuggerBrowsableState.Never)] - public override int Column - { - get { return column; } - } + public override int Column { get; } } /// @@ -285,11 +261,6 @@ public override int Column /// private class SmallElementLocation : ElementLocation { - /// - /// The source file. - /// - private string file; - /// /// Packs both the line and column values into a single four-byte element. /// The high two bytes are the line, and low two bytes are the column. @@ -308,7 +279,7 @@ private class SmallElementLocation : ElementLocation /// internal SmallElementLocation(string file, int line, int column) { - this.file = file; + File = file; packedData = (line << 16) | column; } @@ -319,10 +290,7 @@ internal SmallElementLocation(string file, int line, int column) /// If not known, returns empty string. /// [DebuggerBrowsable(DebuggerBrowsableState.Never)] - public override string File - { - get { return file; } - } + public override string File { get; } /// /// The line number where this element exists in its file. From 2f1c07864dad9ec03b2efc3194cab50451e762ab Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Thu, 18 Apr 2024 11:24:50 +1000 Subject: [PATCH 11/29] Use inheritdoc to avoid duplication --- src/Build/ElementLocation/ElementLocation.cs | 55 ++++---------------- 1 file changed, 10 insertions(+), 45 deletions(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index 9c55a7720e2..35fb8b75fe4 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -93,19 +93,14 @@ public static ElementLocation EmptyLocation get { return s_emptyElementLocation; } } - /// - /// Get reasonable hash code. - /// + /// public override int GetHashCode() { // Line and column are good enough return Line ^ Column; } - /// - /// Override Equals so that identical - /// fields imply equal objects. - /// + /// public override bool Equals(object? obj) { IElementLocation? that = obj as IElementLocation; @@ -128,17 +123,13 @@ public override bool Equals(object? obj) return true; } - /// - /// Location of element. - /// + /// public override string ToString() { return LocationString; } - /// - /// Writes the packet to the serializer. - /// + /// void ITranslatable.Translate(ITranslator translator) { ErrorUtilities.VerifyThrow(translator.Mode == TranslationDirection.WriteToStream, "write only"); @@ -224,28 +215,15 @@ internal RegularElementLocation(string file, int line, int column) Column = column; } - /// - /// The file from which this particular element originated. It may - /// differ from the ProjectFile if, for instance, it was part of - /// an import or originated in a targets file. - /// If not known, returns empty string. - /// + /// [DebuggerBrowsable(DebuggerBrowsableState.Never)] public override string File { get; } - /// - /// The line number where this element exists in its file. - /// The first line is numbered 1. - /// Zero indicates "unknown location". - /// + /// [DebuggerBrowsable(DebuggerBrowsableState.Never)] public override int Line { get; } - /// - /// The column number where this element exists in its file. - /// The first column is numbered 1. - /// Zero indicates "unknown location". - /// + /// [DebuggerBrowsable(DebuggerBrowsableState.Never)] public override int Column { get; } } @@ -283,31 +261,18 @@ internal SmallElementLocation(string file, int line, int column) packedData = (line << 16) | column; } - /// - /// The file from which this particular element originated. It may - /// differ from the ProjectFile if, for instance, it was part of - /// an import or originated in a targets file. - /// If not known, returns empty string. - /// + /// [DebuggerBrowsable(DebuggerBrowsableState.Never)] public override string File { get; } - /// - /// The line number where this element exists in its file. - /// The first line is numbered 1. - /// Zero indicates "unknown location". - /// + /// [DebuggerBrowsable(DebuggerBrowsableState.Never)] public override int Line { get { return (packedData >> 16) & 0xFFFF; } } - /// - /// The column number where this element exists in its file. - /// The first column is numbered 1. - /// Zero indicates "unknown location". - /// + /// [DebuggerBrowsable(DebuggerBrowsableState.Never)] public override int Column { From 28cfb199105bd0da7da2b40d70d74c92a1b0f601 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Thu, 18 Apr 2024 11:25:16 +1000 Subject: [PATCH 12/29] Make field readonly --- src/Build/ElementLocation/ElementLocation.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index 35fb8b75fe4..5647e361fb6 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -23,7 +23,7 @@ public abstract class ElementLocation : IElementLocation, ITranslatable, IImmuta /// /// The singleton empty element location. /// - private static ElementLocation s_emptyElementLocation = new SmallElementLocation("", 0, 0); + private static readonly ElementLocation s_emptyElementLocation = new SmallElementLocation("", 0, 0); /// /// The file from which this particular element originated. It may From 2acc6281ab25679ca96068e840bd380ec35f3a59 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Thu, 18 Apr 2024 11:29:36 +1000 Subject: [PATCH 13/29] Use constants --- src/Build/ElementLocation/ElementLocation.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index 5647e361fb6..7b31eca6b8d 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -190,7 +190,7 @@ internal static ElementLocation Create(string? file, int line, int column) file ??= ""; - if (line <= 65535 && column <= 65535) + if (line <= ushort.MaxValue && column <= ushort.MaxValue) { return new SmallElementLocation(file, line, column); } From 596c57485098014ff0b3bbd2c3be90ade44a4832 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Thu, 18 Apr 2024 11:32:45 +1000 Subject: [PATCH 14/29] Reduce branching when testing line/column values --- src/Build/ElementLocation/ElementLocation.cs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index 7b31eca6b8d..3865c1d6b31 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -181,16 +181,25 @@ internal static ElementLocation Create(string file) /// internal static ElementLocation Create(string? file, int line, int column) { - if (string.IsNullOrEmpty(file) && line == 0 && column == 0) + // Combine line and column values with bitwise OR so we can perform various + // checks on both values in a single comparison, reducing the amount of branching + // in the code. + int combinedValue = line | column; + + if (string.IsNullOrEmpty(file) && combinedValue == 0) { + // When combinedValue is zero, it implies that both line and column are zero. return EmptyLocation; } - ErrorUtilities.VerifyThrow(line > -1 && column > -1, "Use zero for unknown"); + // When combinedValue is negative, it implies that either line or column were negative + ErrorUtilities.VerifyThrow(combinedValue > -1, "Use zero for unknown"); file ??= ""; - if (line <= ushort.MaxValue && column <= ushort.MaxValue) + // When combinedValue is less than a threshold, it implies that both line and column are less + // than that threshold. + if (combinedValue <= ushort.MaxValue) { return new SmallElementLocation(file, line, column); } From d4615223b2d4b8cb77720d8e4e9b5d480bff0c5d Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Thu, 18 Apr 2024 11:35:03 +1000 Subject: [PATCH 15/29] Use pattern matching --- src/Build/ElementLocation/ElementLocation.cs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index 3865c1d6b31..7dbf228bbd9 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -103,9 +103,7 @@ public override int GetHashCode() /// public override bool Equals(object? obj) { - IElementLocation? that = obj as IElementLocation; - - if (that == null) + if (obj is not IElementLocation that) { return false; } From 5ef2a4b696ab07742f086f44d8dcd2e9da75abd6 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Thu, 18 Apr 2024 11:37:35 +1000 Subject: [PATCH 16/29] Use standard API doc prefix --- src/Build/ElementLocation/ElementLocation.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index 7dbf228bbd9..fca729ed64a 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -26,7 +26,7 @@ public abstract class ElementLocation : IElementLocation, ITranslatable, IImmuta private static readonly ElementLocation s_emptyElementLocation = new SmallElementLocation("", 0, 0); /// - /// The file from which this particular element originated. It may + /// Gets the file from which this particular element originated. It may /// differ from the ProjectFile if, for instance, it was part of /// an import or originated in a targets file. /// If not known, returns empty string. @@ -38,7 +38,7 @@ public abstract string File } /// - /// The line number where this element exists in its file. + /// Gets the line number where this element exists in its file. /// The first line is numbered 1. /// Zero indicates "unknown location". /// @@ -49,7 +49,7 @@ public abstract int Line } /// - /// The column number where this element exists in its file. + /// Gets the column number where this element exists in its file. /// The first column is numbered 1. /// Zero indicates "unknown location". /// @@ -60,7 +60,7 @@ public abstract int Column } /// - /// The location in a form suitable for replacement + /// Gets the location in a form suitable for replacement /// into a message. /// Example: "c:\foo\bar.csproj (12,34)" /// Calling this creates and formats a new string. From 2b81d60ceea2afa6febf1cce127586bcfa270cfe Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Thu, 18 Apr 2024 11:40:02 +1000 Subject: [PATCH 17/29] Use primary constructor --- src/Build/ElementLocation/ElementLocation.cs | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index fca729ed64a..aec4e64c3fa 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -208,31 +208,19 @@ internal static ElementLocation Create(string? file, int line, int column) /// /// Rarer variation for when the line and column won't each fit in a ushort. /// - private class RegularElementLocation : ElementLocation + private class RegularElementLocation(string file, int line, int column) : ElementLocation { - /// - /// Constructor for the case where we have most or all information. - /// Numerical values must be 1-based, non-negative; 0 indicates unknown - /// File may be empty, indicating the file was not loaded from disk. - /// - internal RegularElementLocation(string file, int line, int column) - { - File = file; - Line = line; - Column = column; - } - /// [DebuggerBrowsable(DebuggerBrowsableState.Never)] - public override string File { get; } + public override string File { get; } = file; /// [DebuggerBrowsable(DebuggerBrowsableState.Never)] - public override int Line { get; } + public override int Line { get; } = line; /// [DebuggerBrowsable(DebuggerBrowsableState.Never)] - public override int Column { get; } + public override int Column { get; } = column; } /// From fc82d47c92d112fa23fe0b4dbb5e5c0fcd7d09a4 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Thu, 18 Apr 2024 11:40:46 +1000 Subject: [PATCH 18/29] Seal private nested classes --- src/Build/ElementLocation/ElementLocation.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index aec4e64c3fa..2235c8999f8 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -208,7 +208,7 @@ internal static ElementLocation Create(string? file, int line, int column) /// /// Rarer variation for when the line and column won't each fit in a ushort. /// - private class RegularElementLocation(string file, int line, int column) : ElementLocation + private sealed class RegularElementLocation(string file, int line, int column) : ElementLocation { /// [DebuggerBrowsable(DebuggerBrowsableState.Never)] @@ -232,7 +232,7 @@ private class RegularElementLocation(string file, int line, int column) : Elemen /// A "very small" variation that used two bytes (or halves of a short) would fit about half of them /// and save 4 more bytes each, but the CLR packs each field to 4 bytes, so it isn't actually any smaller. /// - private class SmallElementLocation : ElementLocation + private sealed class SmallElementLocation : ElementLocation { /// /// Packs both the line and column values into a single four-byte element. From be277f6e099e3ccbdafc9498896779bddfc4b7aa Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Fri, 19 Apr 2024 09:49:28 +1000 Subject: [PATCH 19/29] Improve hash function --- src/Build/ElementLocation/ElementLocation.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index 2235c8999f8..80a22c39836 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -96,8 +96,8 @@ public static ElementLocation EmptyLocation /// public override int GetHashCode() { - // Line and column are good enough - return Line ^ Column; + // We don't include the file path in the hash + return (Line * 397) ^ Column; } /// From 2653348f3769a4421f24801dcf5d48ec5afa7872 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Fri, 19 Apr 2024 09:52:08 +1000 Subject: [PATCH 20/29] Revert field packing The CLR does in fact pack these fields adjacent to one another, so we don't have to do this in code ourselves. --- src/Build/ElementLocation/ElementLocation.cs | 38 +++----------------- 1 file changed, 5 insertions(+), 33 deletions(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index 80a22c39836..4832da68e8b 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -199,7 +199,7 @@ internal static ElementLocation Create(string? file, int line, int column) // than that threshold. if (combinedValue <= ushort.MaxValue) { - return new SmallElementLocation(file, line, column); + return new SmallElementLocation(file, (ushort)line, (ushort)column); } return new RegularElementLocation(file, line, column); @@ -232,47 +232,19 @@ private sealed class RegularElementLocation(string file, int line, int column) : /// A "very small" variation that used two bytes (or halves of a short) would fit about half of them /// and save 4 more bytes each, but the CLR packs each field to 4 bytes, so it isn't actually any smaller. /// - private sealed class SmallElementLocation : ElementLocation + private sealed class SmallElementLocation(string file, ushort line, ushort column) : ElementLocation { - /// - /// Packs both the line and column values into a single four-byte element. - /// The high two bytes are the line, and low two bytes are the column. - /// - /// - /// If we had two fields, the CLR would pad them each to - /// four-byte boundaries, meaning no space would actually be saved here. - /// So instead, we pack them manually. - /// - private int packedData; - - /// - /// Constructor for the case where we have most or all information. - /// Numerical values must be 1-based, non-negative; 0 indicates unknown - /// File may empty, indicating the file was not loaded from disk. - /// - internal SmallElementLocation(string file, int line, int column) - { - File = file; - packedData = (line << 16) | column; - } - /// [DebuggerBrowsable(DebuggerBrowsableState.Never)] - public override string File { get; } + public override string File => file; /// [DebuggerBrowsable(DebuggerBrowsableState.Never)] - public override int Line - { - get { return (packedData >> 16) & 0xFFFF; } - } + public override int Line => line; /// [DebuggerBrowsable(DebuggerBrowsableState.Never)] - public override int Column - { - get { return packedData & ushort.MaxValue; } - } + public override int Column => column; } } } From efb2f9ae0977c69877745bbd51f8726c20021155 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Fri, 19 Apr 2024 19:06:04 +1000 Subject: [PATCH 21/29] Inline field --- src/Build/ElementLocation/ElementLocation.cs | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index 4832da68e8b..2ce47830fd6 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -21,9 +21,12 @@ namespace Microsoft.Build.Construction public abstract class ElementLocation : IElementLocation, ITranslatable, IImmutable { /// - /// The singleton empty element location. + /// Gets the empty element location. + /// This is not to be used when something is "missing": that should have a null location. + /// It is to be used for the project location when the project has not been given a name. + /// In that case, it exists, but can't have a specific location. /// - private static readonly ElementLocation s_emptyElementLocation = new SmallElementLocation("", 0, 0); + public static ElementLocation EmptyLocation { get; } = new SmallElementLocation("", 0, 0); /// /// Gets the file from which this particular element originated. It may @@ -82,17 +85,6 @@ public string LocationString } } - /// - /// Gets the empty element location. - /// This is not to be used when something is "missing": that should have a null location. - /// It is to be used for the project location when the project has not been given a name. - /// In that case, it exists, but can't have a specific location. - /// - public static ElementLocation EmptyLocation - { - get { return s_emptyElementLocation; } - } - /// public override int GetHashCode() { From 4d4a4ee2e107dd4f5054e5a4425b383a16216426 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Mon, 22 Apr 2024 12:31:28 +1000 Subject: [PATCH 22/29] Pack ElementLocation more efficiently on 64-bit architectures Adds new subtypes for `ElementLocation` that pack to multiples of eight bytes, to avoid wasting space at runtime on padding between instances of this class in memory. The primary gain here comes from being able to use a smaller value for the `File` value. With this change, there's a lock-free cache of file paths which are then stored by index. When the index is small, as it usually will be, it can be packed for efficiently (e.g. in 2 bytes) than a string reference (8 bytes on 64-bit architectures). See code comment for more details. Also remove file IO from unit tests so they run faster. --- .../Construction/ElementLocation_Tests.cs | 514 +++++++----------- src/Build/ElementLocation/ElementLocation.cs | 310 +++++++++-- .../XmlDocumentWithLocation.cs | 10 + 3 files changed, 460 insertions(+), 374 deletions(-) diff --git a/src/Build.UnitTests/Construction/ElementLocation_Tests.cs b/src/Build.UnitTests/Construction/ElementLocation_Tests.cs index 7a33e0d5343..c4f23a15244 100644 --- a/src/Build.UnitTests/Construction/ElementLocation_Tests.cs +++ b/src/Build.UnitTests/Construction/ElementLocation_Tests.cs @@ -2,9 +2,12 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Collections.Generic; using System.IO; using System.Reflection; +using System.Text; using System.Xml; +using System.Xml.Linq; using Microsoft.Build.BackEnd; using Microsoft.Build.Construction; using Microsoft.Build.Exceptions; @@ -13,76 +16,101 @@ using Microsoft.Build.UnitTests.BackEnd; using Xunit; -#nullable disable +#pragma warning disable CA3075 // Insecure DTD processing in XML +#pragma warning disable IDE0022 // Use expression body for method +#pragma warning disable IDE0058 // Expression value is never used +#pragma warning disable SA1010 // Opening square brackets should be spaced correctly namespace Microsoft.Build.UnitTests.Construction { /// - /// Tests for the ElementLocation class + /// Unit tests for . /// public class ElementLocation_Tests { - /// - /// Path to the common targets - /// - private string _pathToCommonTargets = -#if FEATURE_INSTALLED_MSBUILD - Path.Combine(FrameworkLocationHelper.PathToDotNetFrameworkV45, "Microsoft.Common.targets"); -#else - Path.Combine(AppContext.BaseDirectory, "Microsoft.Common.targets"); -#endif + [Theory] + [MemberData(nameof(GetCreateTestCases))] + public void Create(string? file, int line, int column, string typeName) + { + ElementLocation location = ElementLocation.Create(file, line, column); - /// - /// Tests constructor specifying only file. - /// - [Fact] - public void ConstructorTest1() + Assert.Equal(file ?? "", location.File); + Assert.Equal(line, location.Line); + Assert.Equal(column, location.Column); + + Assert.Contains(typeName, location.GetType().FullName); + } + + [Theory] + [InlineData("file", -1, 0)] + [InlineData("file", 0, -1)] + [InlineData("file", int.MaxValue, -1)] + [InlineData("file", -1, int.MaxValue)] + [InlineData("file", -1, -1)] + [InlineData("file", int.MinValue, 0)] + [InlineData("file", 0, int.MinValue)] + [InlineData("file", int.MinValue, int.MinValue)] + public void Create_NegativeValuesThrow(string file, int line, int column) { - IElementLocation location = ElementLocation.Create("file", 65536, 0); - Assert.Equal("file", location.File); - Assert.Equal(65536, location.Line); - Assert.Equal(0, location.Column); - Assert.Contains("RegularElementLocation", location.GetType().FullName); + _ = Assert.Throws( + () => ElementLocation.Create(file, line, column)); } - /// - /// Tests constructor specifying only file. - /// [Fact] - public void ConstructorTest2() + public void Create_NullFile() { - IElementLocation location = ElementLocation.Create("file", 0, 65536); - Assert.Equal("file", location.File); + ElementLocation location = ElementLocation.Create(null); + + Assert.Equal("", location.File); Assert.Equal(0, location.Line); - Assert.Equal(65536, location.Column); - Assert.Contains("RegularElementLocation", location.GetType().FullName); + Assert.Equal(0, location.Column); } - /// - /// Tests constructor specifying only file. - /// - [Fact] - public void ConstructorTest3() + [Theory] + [MemberData(nameof(GetCreateTestCases))] + public void RoundTripSerialisation(string? file, int line, int column, string typeName) { - IElementLocation location = ElementLocation.Create("file", 65536, 65537); - Assert.Equal("file", location.File); - Assert.Equal(65536, location.Line); - Assert.Equal(65537, location.Column); - Assert.Contains("RegularElementLocation", location.GetType().FullName); + ElementLocation location = ElementLocation.Create(file, line, column); + + TranslationHelpers.GetWriteTranslator().Translate(ref location, ElementLocation.FactoryForDeserialization); + ElementLocation? deserializedLocation = null; + TranslationHelpers.GetReadTranslator().Translate(ref deserializedLocation, ElementLocation.FactoryForDeserialization); + Assert.NotNull(deserializedLocation); + + Assert.Equal(file ?? "", deserializedLocation.File); + Assert.Equal(line, deserializedLocation.Line); + Assert.Equal(column, deserializedLocation.Column); + + Assert.Contains(typeName, deserializedLocation.GetType().FullName); + } + + public static IEnumerable GetCreateTestCases() + { + yield return [null, 0, 0, "EmptyElementLocation"]; + yield return ["", 0, 0, "EmptyElementLocation"]; + yield return ["file", byte.MaxValue, 0, "SmallFileElementLocation"]; + yield return ["file", byte.MaxValue + 1, 0, "SmallLineElementLocation"]; + yield return ["file", 0, byte.MaxValue, "SmallFileElementLocation"]; + yield return ["file", 0, byte.MaxValue + 1, "SmallColumnElementLocation"]; + yield return ["file", ushort.MaxValue, 0, "SmallLineElementLocation"]; + yield return ["file", ushort.MaxValue + 1, 0, "LargeLineElementLocation"]; + yield return ["file", 0, ushort.MaxValue, "SmallColumnElementLocation"]; + yield return ["file", 0, ushort.MaxValue + 1, "LargeColumnElementLocation"]; + yield return ["file", ushort.MaxValue, ushort.MaxValue, "LargeFileElementLocation"]; + yield return ["file", ushort.MaxValue + 1, ushort.MaxValue, "FullElementLocation"]; + yield return ["file", ushort.MaxValue, ushort.MaxValue + 1, "FullElementLocation"]; + yield return ["file", ushort.MaxValue + 1, ushort.MaxValue + 1, "FullElementLocation"]; } - /// - /// Test equality - /// [Fact] public void Equality() { - IElementLocation location1 = ElementLocation.Create("file", 65536, 65537); - IElementLocation location2 = ElementLocation.Create("file", 0, 1); - IElementLocation location3 = ElementLocation.Create("file", 0, 65537); - IElementLocation location4 = ElementLocation.Create("file", 65536, 1); - IElementLocation location5 = ElementLocation.Create("file", 0, 1); - IElementLocation location6 = ElementLocation.Create("file", 65536, 65537); + ElementLocation location1 = ElementLocation.Create("file", line: 65536, column: 65537); + ElementLocation location2 = ElementLocation.Create("file", line: 0, column: 1); + ElementLocation location3 = ElementLocation.Create("file", line: 0, column: 65537); + ElementLocation location4 = ElementLocation.Create("file", line: 65536, column: 1); + ElementLocation location5 = ElementLocation.Create("file", line: 0, column: 1); + ElementLocation location6 = ElementLocation.Create("file", line: 65536, column: 65537); Assert.True(location1.Equals(location6)); Assert.True(location2.Equals(location5)); @@ -93,362 +121,204 @@ public void Equality() /// /// Check it will use large element location when it should. - /// Using file as BIZARRELY XmlTextReader+StringReader crops or trims. /// [Fact] public void TestLargeElementLocationUsedLargeColumn() { - string file = null; + StringBuilder xml = new(capacity: 71_000); + + xml.AppendLine(ObjectModelHelpers.CleanupFileContents( + """ + + + """)); + xml.Append(' ', 70_000); + xml.Append(""" + + + + """); - try - { - file = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); + Assert.Equal(71_000, xml.Capacity); - File.WriteAllText(file, ObjectModelHelpers.CleanupFileContents("\r\n") + new string(' ', 70000) + @""); + XmlDocumentWithLocation doc = new(); + doc.LoadXml(xml.ToString()); - ProjectRootElement.Open(file); - } - catch (InvalidProjectFileException ex) - { - Assert.Equal(70012, ex.ColumnNumber); - Assert.Equal(2, ex.LineNumber); - } - finally - { - File.Delete(file); - } + InvalidProjectFileException ex = Assert.Throws( + () => ProjectRootElement.Open(doc)); + + Assert.Equal(70_001, ex.ColumnNumber); + Assert.Equal(3, ex.LineNumber); } /// /// Check it will use large element location when it should. - /// Using file as BIZARRELY XmlTextReader+StringReader crops or trims. /// [Fact] public void TestLargeElementLocationUsedLargeLine() { - string file = null; - - try - { - string longstring = String.Empty; - - for (int i = 0; i < 7000; i++) - { - longstring += "\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n\r\n"; - } - - file = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); - - File.WriteAllText(file, ObjectModelHelpers.CleanupFileContents("\r\n") + longstring + @" "); - - ProjectRootElement.Open(file); - } - catch (InvalidProjectFileException ex) - { - Assert.Equal(70002, ex.LineNumber); - Assert.Equal(2, ex.ColumnNumber); - } - finally - { - File.Delete(file); - } - } - - /// - /// Tests serialization. - /// - [Fact] - public void SerializationTest() - { - IElementLocation location = ElementLocation.Create("file", 65536, 65537); + StringBuilder xml = new(capacity: 71_000); - TranslationHelpers.GetWriteTranslator().Translate(ref location, ElementLocation.FactoryForDeserialization); - IElementLocation deserializedLocation = null; - TranslationHelpers.GetReadTranslator().Translate(ref deserializedLocation, ElementLocation.FactoryForDeserialization); - - Assert.Equal(location.File, deserializedLocation.File); - Assert.Equal(location.Line, deserializedLocation.Line); - Assert.Equal(location.Column, deserializedLocation.Column); - Assert.Contains("RegularElementLocation", location.GetType().FullName); - } - - /// - /// Tests serialization of empty location. - /// - [Fact] - public void SerializationTestForEmptyLocation() - { - IElementLocation location = ElementLocation.EmptyLocation; - - TranslationHelpers.GetWriteTranslator().Translate(ref location, ElementLocation.FactoryForDeserialization); - IElementLocation deserializedLocation = null; - TranslationHelpers.GetReadTranslator().Translate(ref deserializedLocation, ElementLocation.FactoryForDeserialization); - - Assert.Equal(location.File, deserializedLocation.File); - Assert.Equal(location.Line, deserializedLocation.Line); - Assert.Equal(location.Column, deserializedLocation.Column); - Assert.Contains("SmallElementLocation", deserializedLocation.GetType().FullName); - } - - /// - /// Tests constructor specifying file, line and column. - /// - [Fact] - public void ConstructorWithIndicesTest_SmallElementLocation() - { - IElementLocation location = ElementLocation.Create("file", 65535, 65534); - Assert.Equal("file", location.File); - Assert.Equal(65535, location.Line); - Assert.Equal(65534, location.Column); - Assert.Contains("SmallElementLocation", location.GetType().FullName); - } - - /// - /// Tests constructor specifying file, negative line, column - /// - [Fact] - public void ConstructorWithNegativeIndicesTest1() - { - Assert.Throws(() => - { - ElementLocation.Create("file", -1, 2); - }); - } - /// - /// Tests constructor specifying file, line, negative column - /// - [Fact] - public void ConstructorWithNegativeIndicesTest2n() - { - Assert.Throws(() => - { - ElementLocation.Create("file", 1, -2); - }); - } - /// - /// Tests constructor with invalid null file. - /// - [Fact] - public void ConstructorTestNullFile() - { - IElementLocation location = ElementLocation.Create(null); - Assert.Equal(location.File, String.Empty); - } + xml.Append(ObjectModelHelpers.CleanupFileContents( + """ + + + """)); - /// - /// Tests constructor specifying only file. - /// - [Fact] - public void ConstructorTest1_SmallElementLocation() - { - IElementLocation location = ElementLocation.Create("file", 65535, 0); - Assert.Equal("file", location.File); - Assert.Equal(65535, location.Line); - Assert.Equal(0, location.Column); - Assert.Contains("SmallElementLocation", location.GetType().FullName); - } + xml.Append('\n', 70_000); - /// - /// Tests constructor specifying only file. - /// - [Fact] - public void ConstructorTest2_SmallElementLocation() - { - IElementLocation location = ElementLocation.Create("file", 0, 65535); - Assert.Equal("file", location.File); - Assert.Equal(0, location.Line); - Assert.Equal(65535, location.Column); - Assert.Contains("SmallElementLocation", location.GetType().FullName); - } + xml.Append( + """ + + + + """); - /// - /// Tests constructor specifying only file. - /// - [Fact] - public void ConstructorTest3_SmallElementLocation() - { - IElementLocation location = ElementLocation.Create("file", 65535, 65534); - Assert.Equal("file", location.File); - Assert.Equal(65535, location.Line); - Assert.Equal(65534, location.Column); - Assert.Contains("SmallElementLocation", location.GetType().FullName); - } + Assert.Equal(71_000, xml.Capacity); - /// - /// Tests serialization. - /// - [Fact] - public void SerializationTest_SmallElementLocation() - { - IElementLocation location = ElementLocation.Create("file", 65535, 2); + XmlDocumentWithLocation doc = new(); + doc.LoadXml(xml.ToString()); - TranslationHelpers.GetWriteTranslator().Translate(ref location, ElementLocation.FactoryForDeserialization); - IElementLocation deserializedLocation = null; - TranslationHelpers.GetReadTranslator().Translate(ref deserializedLocation, ElementLocation.FactoryForDeserialization); + InvalidProjectFileException ex = Assert.Throws( + () => ProjectRootElement.Open(doc)); - Assert.Equal(location.File, deserializedLocation.File); - Assert.Equal(location.Line, deserializedLocation.Line); - Assert.Equal(location.Column, deserializedLocation.Column); - Assert.Contains("SmallElementLocation", location.GetType().FullName); + Assert.Equal(70_002, ex.LineNumber); + Assert.Equal(5, ex.ColumnNumber); } - /// - /// Test many of the getters - /// [Fact] [Trait("Category", "netcore-osx-failing")] [Trait("Category", "netcore-linux-failing")] - public void LocationStringsMedleyReadOnlyLoad() + public void LocationsMatchWhenReadOnlyOrWriteable() { - string content = ObjectModelHelpers.CleanupFileContents(@" - - - - - foo bar - + string content = ObjectModelHelpers.CleanupFileContents( + """ + + + + + foo bar + - - + + -

+

- - + + - - + + - + - -

+ +

- + - + - "); + """); string readWriteLoadLocations = GetLocations(content, readOnly: false); string readOnlyLoadLocations = GetLocations(content, readOnly: true); - Console.WriteLine(readWriteLoadLocations); - Helpers.VerifyAssertLineByLine(readWriteLoadLocations, readOnlyLoadLocations); + + static string GetLocations(string content, bool readOnly) + { + string file = FileUtilities.GetTemporaryFileName(); + + XmlDocumentWithLocation doc = new(loadAsReadOnly: readOnly, fullPath: file); + doc.LoadXml(content); + + Assert.Equal(readOnly, doc.IsReadOnly); + + XmlNodeList? allNodes = doc.SelectNodes("//*|//@*"); + + Assert.NotNull(allNodes); + + StringBuilder locations = new(); + + foreach (object node in allNodes) + { + PropertyInfo? property = node.GetType().GetProperty("Location", BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance); + + var value = (ElementLocation?)property?.GetValue(node, null); + + if (value is not null) // null means attribute is not present + { + locations.Append(((XmlNode)node).Name).Append("==").Append(((XmlNode)node).Value ?? "").Append(": ").Append(value.LocationString.Replace(file, """c:\foo\bar.csproj""")).Append("\r\n"); + } + } + + return locations.ToString(); + } } - ///

- /// Save read only fails - /// [Fact] - public void SaveReadOnly1() + public void SaveReadOnly_FilePath_Throws() { - Assert.Throws(() => + XmlDocumentWithLocation doc = new(loadAsReadOnly: true); + + Assert.True(doc.IsReadOnly); + + _ = Assert.Throws(() => { - var doc = new XmlDocumentWithLocation(loadAsReadOnly: true); - doc.Load(_pathToCommonTargets); - Assert.True(doc.IsReadOnly); doc.Save(FileUtilities.GetTemporaryFile()); }); } - /// - /// Save read only fails - /// [Fact] [Trait("Category", "netcore-osx-failing")] [Trait("Category", "netcore-linux-failing")] - public void SaveReadOnly2() + public void SaveReadOnly_Stream_Throws() { - var doc = new XmlDocumentWithLocation(loadAsReadOnly: true); - doc.Load(_pathToCommonTargets); + XmlDocumentWithLocation doc = new(loadAsReadOnly: true); + Assert.True(doc.IsReadOnly); - Assert.Throws(() => + + _ = Assert.Throws(() => { doc.Save(new MemoryStream()); }); } - /// - /// Save read only fails - /// [Fact] [Trait("Category", "netcore-osx-failing")] [Trait("Category", "netcore-linux-failing")] - public void SaveReadOnly3() + public void SaveReadOnly_TextWriter_Throws() { - var doc = new XmlDocumentWithLocation(loadAsReadOnly: true); - doc.Load(_pathToCommonTargets); + XmlDocumentWithLocation doc = new(loadAsReadOnly: true); + Assert.True(doc.IsReadOnly); - Assert.Throws(() => + + _ = Assert.Throws(() => { doc.Save(new StringWriter()); }); } - /// - /// Save read only fails - /// [Fact] [Trait("Category", "netcore-osx-failing")] [Trait("Category", "netcore-linux-failing")] - public void SaveReadOnly4() - { - var doc = new XmlDocumentWithLocation(loadAsReadOnly: true); - doc.Load(_pathToCommonTargets); - Assert.True(doc.IsReadOnly); - using (XmlWriter wr = XmlWriter.Create(new FileStream(FileUtilities.GetTemporaryFileName(), FileMode.Create))) - { - Assert.Throws(() => - { - doc.Save(wr); - }); - } - } - - /// - /// Get location strings for the content, loading as readonly if specified - /// - private string GetLocations(string content, bool readOnly) + public void SaveReadOnly_XmlWriter_Throws() { - string file = null; + XmlDocumentWithLocation doc = new(loadAsReadOnly: true); - try - { - file = FileUtilities.GetTemporaryFileName(); - File.WriteAllText(file, content); - var doc = new XmlDocumentWithLocation(loadAsReadOnly: readOnly); - doc.Load(file); - Assert.Equal(readOnly, doc.IsReadOnly); - var allNodes = doc.SelectNodes("//*|//@*"); + Assert.True(doc.IsReadOnly); - string locations = String.Empty; - foreach (var node in allNodes) - { - foreach (var property in node.GetType().GetProperties(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance)) - { - if (property.Name.Equals("Location")) - { - var value = ((ElementLocation)property.GetValue(node, null)); - - if (value != null) // null means attribute is not present - { - locations += ((XmlNode)node).Name + "==" + ((XmlNode)node).Value ?? String.Empty + ": " + value.LocationString + "\r\n"; - } - } - } - } + using XmlWriter wr = XmlWriter.Create(new MemoryStream()); - return locations.Replace(file, "c:\\foo\\bar.csproj"); - } - finally + _ = Assert.Throws(() => { - File.Delete(file); - } + doc.Save(wr); + }); } } } diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index 2ce47830fd6..48fe3497c6c 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -2,7 +2,10 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Collections.Generic; +using System.Collections.Immutable; using System.Diagnostics; +using System.Threading; using Microsoft.Build.BackEnd; using Microsoft.Build.Collections; using Microsoft.Build.Shared; @@ -16,6 +19,8 @@ namespace Microsoft.Build.Construction /// /// This object is IMMUTABLE, so that it can be passed around arbitrarily. /// DO NOT make these objects any larger. There are huge numbers of them and they are transmitted between nodes. + /// + /// Although this class is called "element" location, it is also used for other XML node types, such as in . /// [Serializable] public abstract class ElementLocation : IElementLocation, ITranslatable, IImmutable @@ -26,7 +31,7 @@ public abstract class ElementLocation : IElementLocation, ITranslatable, IImmuta /// It is to be used for the project location when the project has not been given a name. /// In that case, it exists, but can't have a specific location. /// - public static ElementLocation EmptyLocation { get; } = new SmallElementLocation("", 0, 0); + public static ElementLocation EmptyLocation { get; } = new EmptyElementLocation(); /// /// Gets the file from which this particular element originated. It may @@ -35,10 +40,7 @@ public abstract class ElementLocation : IElementLocation, ITranslatable, IImmuta /// If not known, returns empty string. /// [DebuggerBrowsable(DebuggerBrowsableState.Never)] - public abstract string File - { - get; - } + public abstract string File { get; } /// /// Gets the line number where this element exists in its file. @@ -46,10 +48,7 @@ public abstract string File /// Zero indicates "unknown location". /// [DebuggerBrowsable(DebuggerBrowsableState.Never)] - public abstract int Line - { - get; - } + public abstract int Line { get; } /// /// Gets the column number where this element exists in its file. @@ -57,10 +56,7 @@ public abstract int Line /// Zero indicates "unknown location". /// [DebuggerBrowsable(DebuggerBrowsableState.Never)] - public abstract int Column - { - get; - } + public abstract int Column { get; } /// /// Gets the location in a form suitable for replacement @@ -155,11 +151,15 @@ internal static ElementLocation FactoryForDeserialization(ITranslator translator /// This is the case when we are creating a new item, for example, and it has /// not been evaluated from some XML. /// - internal static ElementLocation Create(string file) + internal static ElementLocation Create(string? file) { return Create(file, 0, 0); } + private static string[] s_fileByIndex = new string[32]; + private static int s_nextFileIndex; + private static ImmutableDictionary s_indexByFile = ImmutableDictionary.Empty.WithComparers(StringComparer.OrdinalIgnoreCase); + /// /// Constructor for the case where we have most or all information. /// Numerical values must be 1-based, non-negative; 0 indicates unknown @@ -169,74 +169,280 @@ internal static ElementLocation Create(string file) /// In AG there are 600 locations that have a file but zero line and column. /// In theory yet another derived class could be made for these to save 4 bytes each. /// - internal static ElementLocation Create(string? file, int line, int column) + internal static ElementLocation Create(string? filePath, int line, int column) { // Combine line and column values with bitwise OR so we can perform various // checks on both values in a single comparison, reducing the amount of branching // in the code. int combinedValue = line | column; - if (string.IsNullOrEmpty(file) && combinedValue == 0) + if (string.IsNullOrEmpty(filePath) && combinedValue == 0) { // When combinedValue is zero, it implies that both line and column are zero. - return EmptyLocation; + return EmptyElementLocation.Instance; } - // When combinedValue is negative, it implies that either line or column were negative + // When combinedValue is negative, it implies that either line or column were negative. ErrorUtilities.VerifyThrow(combinedValue > -1, "Use zero for unknown"); - file ??= ""; + // TODO store the last run's value and check if this is for the same file. If so, skip the dictionary lookup (tree walk). + int fileIndex = GetOrAddFileIndex(filePath); + + Debug.Assert(Equals(filePath, LookupFileByIndex(fileIndex))); + + // We use multiple packing schemes for this data. TypeSize below excludes the CLR's per-object overhead. + // + // Name TypeSize FileIndex Line Column + // + // EmptyElementLocation 0 0 (max 0) 0 (max 0) 0 (max 0) + // + // SmallFileElementLocation 8 4 (max 65,535) 2 (max 256) 2 (max 256) + // SmallLineElementLocation 8 2 (max 256) 4 (max 65,535) 2 (max 256) + // SmallColumnElementLocation 8 2 (max 256) 2 (max 256) 4 (max 65,535) + // + // LargeFileElementLocation 16 8 (max 2b) 4 (max 65,535) 4 (max 65,535) + // LargeLineElementLocation 16 4 (max 65,535) 8 (max 2b) 4 (max 65,535) + // LargeColumnElementLocation 16 4 (max 65,535) 4 (max 65,535) 8 (max 2b) + // + // FullElementLocation 24 8 (max 2b) 8 (max 2b) 8 (max 2b) + + // Check for empty first + if (fileIndex is 0 && line is 0 && column is 0) + { + return EmptyElementLocation.Instance; + } + + combinedValue |= fileIndex; + // We want class sizes as multiples of 8 on 64-bit architectures, for alignment reasons. + // Any space used between these multiples leads to unused bytes in padding. We want to + // take advantage of these sizes effectively, so we check for various requirements and + // choose. + // + // Search in popularity order to reduce the number of branches taken through this code. + // // When combinedValue is less than a threshold, it implies that both line and column are less // than that threshold. - if (combinedValue <= ushort.MaxValue) + + // Handle cases that fit in 0xFF and 0XFFFF. + if (combinedValue <= byte.MaxValue) + { + // All values fit within a byte. Pick one of the 8-byte types. + return new SmallFileElementLocation((ushort)fileIndex, (byte)line, (byte)column); + } + else if (combinedValue <= ushort.MaxValue) { - return new SmallElementLocation(file, (ushort)line, (ushort)column); + // At least one value needs ushort. Try to use an 8-byte type all the same. + if (line <= byte.MaxValue && column <= byte.MaxValue) + { + // Only fileIndex needs 4 bytes + return new SmallFileElementLocation((ushort)fileIndex, (byte)line, (byte)column); + } + else if (fileIndex <= byte.MaxValue && column <= byte.MaxValue) + { + // Only line needs 4 bytes + return new SmallLineElementLocation((byte)fileIndex, (ushort)line, (byte)column); + } + else if (fileIndex <= byte.MaxValue && line <= byte.MaxValue) + { + // Only column needs 4 bytes + return new SmallColumnElementLocation((byte)fileIndex, (byte)line, (ushort)column); + } + else + { + // + return new LargeFileElementLocation(fileIndex, (ushort)line, (ushort)column); + } } + else + { + // At least one value needs int. + if (fileIndex <= short.MaxValue && column <= short.MaxValue) + { + // Only line needs 8 bytes + return new LargeLineElementLocation((ushort)fileIndex, line, (ushort)column); + } + else if (line <= short.MaxValue && column <= short.MaxValue) + { + // Only fileIndex needs 8 bytes + return new LargeFileElementLocation(fileIndex, (ushort)line, (ushort)column); + } + else if (fileIndex <= short.MaxValue && line <= short.MaxValue) + { + // Only column needs 8 bytes + return new LargeColumnElementLocation((ushort)fileIndex, (ushort)line, column); + } + else + { + return new FullElementLocation(fileIndex, line, column); + } + } + + static int GetOrAddFileIndex(string? file) + { + if (file is null) + { + return 0; + } + + if (s_indexByFile.TryGetValue(file, out int index)) + { + return index + 1; + } + + return AddFile(); + + int AddFile() + { + int index = Interlocked.Increment(ref s_nextFileIndex) - 1; + + SetValue(index); + + _ = ImmutableInterlocked.TryAdd(ref s_indexByFile, file, index); + + return index + 1; + } + + void SetValue(int index) + { + while (true) + { + string[] array = Volatile.Read(ref s_fileByIndex); + + if (index < array.Length) + { + array[index] = file; + return; + } + + // Need to grow the array + + // Wait for the last value to be non-null, so that we have all values to copy + while (array[array.Length - 1] is null) + { + Thread.SpinWait(100); + } + + int newArrayLength = array.Length * 2; + + while (index >= newArrayLength) + { + newArrayLength *= 2; + } + + string[] newArray = new string[newArrayLength]; + array.AsSpan().CopyTo(newArray); + newArray[index] = file; + + string[] exchanged = Interlocked.CompareExchange(ref s_fileByIndex, newArray, array); + + if (ReferenceEquals(exchanged, array)) + { + // We replaced it + return; + } - return new RegularElementLocation(file, line, column); + // Otherwise, loop around again. We can't just return exchanged here, + // as theoretically the array might have been grown more than once. + } + } + } } - /// - /// Rarer variation for when the line and column won't each fit in a ushort. - /// - private sealed class RegularElementLocation(string file, int line, int column) : ElementLocation + internal static string LookupFileByIndex(int index) { - /// - [DebuggerBrowsable(DebuggerBrowsableState.Never)] - public override string File { get; } = file; + if (index is 0) + { + return ""; + } + + index -= 1; - /// - [DebuggerBrowsable(DebuggerBrowsableState.Never)] - public override int Line { get; } = line; + Thread.MemoryBarrier(); - /// - [DebuggerBrowsable(DebuggerBrowsableState.Never)] - public override int Column { get; } = column; + string[] array = Volatile.Read(ref s_fileByIndex); + + while (index >= array.Length) + { + // Data race! Spin. + array = Volatile.Read(ref s_fileByIndex); + } + + return array[index]; } - /// - /// For when the line and column each fit in a short - under 65536 - /// (almost always will: microsoft.common.targets is less than 5000 lines long) - /// When loading Australian Government, for example, there are over 31,000 ElementLocation - /// objects so this saves 4 bytes each = 123KB - /// - /// A "very small" variation that used two bytes (or halves of a short) would fit about half of them - /// and save 4 more bytes each, but the CLR packs each field to 4 bytes, so it isn't actually any smaller. - /// - private sealed class SmallElementLocation(string file, ushort line, ushort column) : ElementLocation + #region Element implementations + +#pragma warning disable SA1516 // Elements should be separated by blank line + + private sealed class EmptyElementLocation() : ElementLocation + { + /// + /// Gets the singleton, immutable empty element location. + /// + /// + /// Not to be be used when something is "missing". Use a location for that. + /// Use only for the project location when the project has not been given a name. + /// In that case, it exists, but can't have a specific location. + /// + public static EmptyElementLocation Instance { get; } = new(); + + public override string File => ""; + public override int Line => 0; + public override int Column => 0; + } + + private sealed class SmallFileElementLocation(ushort file, byte line, byte column) : ElementLocation + { + public override string File => LookupFileByIndex(file); + public override int Line => line; + public override int Column => column; + } + + private sealed class SmallLineElementLocation(byte file, ushort line, byte column) : ElementLocation + { + public override string File => LookupFileByIndex(file); + public override int Line => line; + public override int Column => column; + } + + private sealed class SmallColumnElementLocation(byte file, byte line, ushort column) : ElementLocation { - /// - [DebuggerBrowsable(DebuggerBrowsableState.Never)] - public override string File => file; + public override string File => LookupFileByIndex(file); + public override int Line => line; + public override int Column => column; + } - /// - [DebuggerBrowsable(DebuggerBrowsableState.Never)] + private sealed class LargeFileElementLocation(int file, ushort line, ushort column) : ElementLocation + { + public override string File => LookupFileByIndex(file); public override int Line => line; + public override int Column => column; + } - /// - [DebuggerBrowsable(DebuggerBrowsableState.Never)] + private sealed class LargeLineElementLocation(ushort file, int line, ushort column) : ElementLocation + { + public override string File => LookupFileByIndex(file); + public override int Line => line; public override int Column => column; } + + private sealed class LargeColumnElementLocation(ushort file, ushort line, int column) : ElementLocation + { + public override string File => LookupFileByIndex(file); + public override int Line => line; + public override int Column => column; + } + + private sealed class FullElementLocation(int file, int line, int column) : ElementLocation + { + public override string File => LookupFileByIndex(file); + public override int Line => line; + public override int Column => column; + } + +#pragma warning restore SA1516 // Elements should be separated by blank line + + #endregion } } diff --git a/src/Build/ElementLocation/XmlDocumentWithLocation.cs b/src/Build/ElementLocation/XmlDocumentWithLocation.cs index 71f51e9f272..b70dcd17d88 100644 --- a/src/Build/ElementLocation/XmlDocumentWithLocation.cs +++ b/src/Build/ElementLocation/XmlDocumentWithLocation.cs @@ -84,6 +84,16 @@ internal XmlDocumentWithLocation(bool? loadAsReadOnly) _loadAsReadOnly = loadAsReadOnly; } + /// + /// Constructor that allows specifying the full path. Intended for unit tests only, + /// to avoid having to files to disk during tests just to see location paths populated. + /// + internal XmlDocumentWithLocation(bool? loadAsReadOnly, string fullPath) + : this(loadAsReadOnly) + { + _fullPath = fullPath; + } + /// /// Whether to load files read only /// From e5ab818fe483e89b9a9d53f65e0dcf88cf6b6d4b Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Mon, 22 Apr 2024 22:15:02 +1000 Subject: [PATCH 23/29] Reset the file index before running ElementLocation tests --- .../Construction/ElementLocation_Tests.cs | 15 ++++++++++++++- src/Build/ElementLocation/ElementLocation.cs | 2 ++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/Build.UnitTests/Construction/ElementLocation_Tests.cs b/src/Build.UnitTests/Construction/ElementLocation_Tests.cs index c4f23a15244..c92ed9c018d 100644 --- a/src/Build.UnitTests/Construction/ElementLocation_Tests.cs +++ b/src/Build.UnitTests/Construction/ElementLocation_Tests.cs @@ -26,8 +26,21 @@ namespace Microsoft.Build.UnitTests.Construction /// /// Unit tests for . /// - public class ElementLocation_Tests + [Collection("ElementLocation")] + public class ElementLocation_Tests : IClassFixture { + /// + /// Reset the file path cache index to zero. We have tests which validate that + /// returns a specific storage type, and + /// that requires the index to be within certain ranges. + /// + public class Fixture : IDisposable + { + public Fixture() => ElementLocation.DangerousInternalResetFileIndex(); + + void IDisposable.Dispose() { } + } + [Theory] [MemberData(nameof(GetCreateTestCases))] public void Create(string? file, int line, int column, string typeName) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index 48fe3497c6c..f506d0b9940 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -160,6 +160,8 @@ internal static ElementLocation Create(string? file) private static int s_nextFileIndex; private static ImmutableDictionary s_indexByFile = ImmutableDictionary.Empty.WithComparers(StringComparer.OrdinalIgnoreCase); + internal static void DangerousInternalResetFileIndex() => s_nextFileIndex = 0; + /// /// Constructor for the case where we have most or all information. /// Numerical values must be 1-based, non-negative; 0 indicates unknown From c47008b903f5e4271ea6630195ad57cda1ee069c Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Mon, 22 Apr 2024 22:45:48 +1000 Subject: [PATCH 24/29] Simplify test a bit --- .../Construction/ElementLocation_Tests.cs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/Build.UnitTests/Construction/ElementLocation_Tests.cs b/src/Build.UnitTests/Construction/ElementLocation_Tests.cs index c92ed9c018d..c6da4758f1b 100644 --- a/src/Build.UnitTests/Construction/ElementLocation_Tests.cs +++ b/src/Build.UnitTests/Construction/ElementLocation_Tests.cs @@ -27,19 +27,14 @@ namespace Microsoft.Build.UnitTests.Construction /// Unit tests for . /// [Collection("ElementLocation")] - public class ElementLocation_Tests : IClassFixture + public class ElementLocation_Tests { /// /// Reset the file path cache index to zero. We have tests which validate that /// returns a specific storage type, and /// that requires the index to be within certain ranges. /// - public class Fixture : IDisposable - { - public Fixture() => ElementLocation.DangerousInternalResetFileIndex(); - - void IDisposable.Dispose() { } - } + public ElementLocation_Tests() => ElementLocation.DangerousInternalResetFileIndex(); [Theory] [MemberData(nameof(GetCreateTestCases))] From be049a8d301275beb35aa1c593759e962e3879c9 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Mon, 22 Apr 2024 22:46:15 +1000 Subject: [PATCH 25/29] Add test that shows file index packing --- .../Construction/ElementLocation_Tests.cs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/Build.UnitTests/Construction/ElementLocation_Tests.cs b/src/Build.UnitTests/Construction/ElementLocation_Tests.cs index c6da4758f1b..ce99b967fa5 100644 --- a/src/Build.UnitTests/Construction/ElementLocation_Tests.cs +++ b/src/Build.UnitTests/Construction/ElementLocation_Tests.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.IO; +using System.Linq; using System.Reflection; using System.Text; using System.Xml; @@ -64,6 +65,22 @@ public void Create_NegativeValuesThrow(string file, int line, int column) () => ElementLocation.Create(file, line, column)); } + [Fact] + public void Create_FileIndexPacking() + { + int i = 0; + + for (int j = 0; j < ushort.MaxValue; j++) + { + Assert.Contains("SmallFileElementLocation", Next()); + } + + // If the file index exceed 65,535 items, we use a larger storage type. + Assert.Contains("LargeFileElementLocation", Next()); + + string? Next() => ElementLocation.Create("file" + i++, 0, 0).GetType().FullName; + } + [Fact] public void Create_NullFile() { From 3fb49a824cb74d6d562fcd67b7fce3b5c34aa84d Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Mon, 22 Apr 2024 22:46:30 +1000 Subject: [PATCH 26/29] Add comment --- src/Build/ElementLocation/ElementLocation.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index f506d0b9940..2318a68ef8c 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -252,7 +252,9 @@ internal static ElementLocation Create(string? filePath, int line, int column) } else { - // + // All three values need ushort. Choose an implementation that gives the file + // index an easily-read value (i.e. within 4 bytes) to simplify reads. The + // assumption is that if you need line, you probably also need column. return new LargeFileElementLocation(fileIndex, (ushort)line, (ushort)column); } } From c3de3786a9381d094e35ac7d13b112899a49bc64 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Mon, 22 Apr 2024 23:43:31 +1000 Subject: [PATCH 27/29] More info in assertion message --- src/Build/ElementLocation/ElementLocation.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index 2318a68ef8c..ee52eea5d0a 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -190,7 +190,10 @@ internal static ElementLocation Create(string? filePath, int line, int column) // TODO store the last run's value and check if this is for the same file. If so, skip the dictionary lookup (tree walk). int fileIndex = GetOrAddFileIndex(filePath); - Debug.Assert(Equals(filePath, LookupFileByIndex(fileIndex))); +#if DEBUG + string lookedUpFilePath = LookupFileByIndex(fileIndex); + Debug.Assert(Equals(filePath, lookedUpFilePath), $"File index {fileIndex} returned for path '{filePath}', but lookup for that index returns '{lookedUpFilePath}'"); +#endif // We use multiple packing schemes for this data. TypeSize below excludes the CLR's per-object overhead. // From a3af7bf0583342f55b7504cb03886da383db9e0e Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Tue, 23 Apr 2024 00:43:21 +1000 Subject: [PATCH 28/29] Fix assertion --- src/Build/ElementLocation/ElementLocation.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index ee52eea5d0a..9e698b77abf 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -192,7 +192,9 @@ internal static ElementLocation Create(string? filePath, int line, int column) #if DEBUG string lookedUpFilePath = LookupFileByIndex(fileIndex); - Debug.Assert(Equals(filePath, lookedUpFilePath), $"File index {fileIndex} returned for path '{filePath}', but lookup for that index returns '{lookedUpFilePath}'"); + Debug.Assert( + StringComparer.OrdinalIgnoreCase.Equals(filePath ?? "", lookedUpFilePath), + $"File index {fileIndex} returned for path '{filePath}', but lookup for that index returns '{lookedUpFilePath}'."); #endif // We use multiple packing schemes for this data. TypeSize below excludes the CLR's per-object overhead. From eeed8717941adf6750f717214e7a7f5fd3567196 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Tue, 23 Apr 2024 08:07:54 +1000 Subject: [PATCH 29/29] Update test code --- src/Build/ElementLocation/ElementLocation.cs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/Build/ElementLocation/ElementLocation.cs b/src/Build/ElementLocation/ElementLocation.cs index 9e698b77abf..7cc973f6f45 100644 --- a/src/Build/ElementLocation/ElementLocation.cs +++ b/src/Build/ElementLocation/ElementLocation.cs @@ -160,7 +160,12 @@ internal static ElementLocation Create(string? file) private static int s_nextFileIndex; private static ImmutableDictionary s_indexByFile = ImmutableDictionary.Empty.WithComparers(StringComparer.OrdinalIgnoreCase); - internal static void DangerousInternalResetFileIndex() => s_nextFileIndex = 0; + internal static void DangerousInternalResetFileIndex() + { + s_nextFileIndex = 0; + s_fileByIndex = new string[32]; + s_indexByFile = ImmutableDictionary.Empty.WithComparers(StringComparer.OrdinalIgnoreCase); + } /// /// Constructor for the case where we have most or all information. @@ -192,9 +197,10 @@ internal static ElementLocation Create(string? filePath, int line, int column) #if DEBUG string lookedUpFilePath = LookupFileByIndex(fileIndex); - Debug.Assert( - StringComparer.OrdinalIgnoreCase.Equals(filePath ?? "", lookedUpFilePath), - $"File index {fileIndex} returned for path '{filePath}', but lookup for that index returns '{lookedUpFilePath}'."); + if (!StringComparer.OrdinalIgnoreCase.Equals(filePath ?? "", lookedUpFilePath)) + { + Debug.Fail($"File index {fileIndex} returned for path '{filePath}', but lookup for that index returns '{lookedUpFilePath}'."); + } #endif // We use multiple packing schemes for this data. TypeSize below excludes the CLR's per-object overhead.