ElementLocation optimisations

src/Build/ElementLocation/ElementLocation.cs

@@ -7,8 +7,6 @@
 using Microsoft.Build.Collections;
 using Microsoft.Build.Shared;
 
-#nullable disable
-
 namespace Microsoft.Build.Construction
 {
     /// <summary>
@@ -25,10 +23,10 @@ public abstract class ElementLocation : IElementLocation, ITranslatable, IImmuta
         /// <summary>
         /// The singleton empty element location.
         /// </summary>
-        private static ElementLocation s_emptyElementLocation = new SmallElementLocation(null, 0, 0);
+        private static readonly ElementLocation s_emptyElementLocation = new SmallElementLocation("", 0, 0);
 
         /// <summary>
-        /// 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.
@@ -40,7 +38,7 @@ public abstract class ElementLocation : IElementLocation, ITranslatable, IImmuta
         }
 
         /// <summary>
-        /// 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".
         /// </summary>
@@ -51,7 +49,7 @@ public abstract class ElementLocation : IElementLocation, ITranslatable, IImmuta
         }
 
         /// <summary>
-        /// 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".
         /// </summary>
@@ -62,7 +60,7 @@ public abstract class ElementLocation : IElementLocation, ITranslatable, IImmuta
         }
 
         /// <summary>
-        /// 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.
@@ -71,7 +69,17 @@ public abstract class ElementLocation : IElementLocation, ITranslatable, IImmuta
         /// </summary>
         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,
+                };
+            }
         }
 
         /// <summary>
@@ -85,29 +93,17 @@ public static ElementLocation EmptyLocation
             get { return s_emptyElementLocation; }
         }
 
-        /// <summary>
-        /// Get reasonable hash code.
-        /// </summary>
+        /// <inheritdoc />
         public override int GetHashCode()
         {
             // Line and column are good enough
-            return Line.GetHashCode() ^ Column.GetHashCode();
+            return Line ^ Column;
         }
 
-        /// <summary>
-        /// Override Equals so that identical
-        /// fields imply equal objects.
-        /// </summary>
-        public override bool Equals(object obj)
+        /// <inheritdoc />
+        public override bool Equals(object? obj)
         {
-            if (obj == null)
-            {
-                return false;
-            }
-
-            IElementLocation that = obj as IElementLocation;
-
-            if (that == null)
+            if (obj is not IElementLocation that)
             {
                 return false;
             }
@@ -125,23 +121,19 @@ public override bool Equals(object obj)
             return true;
         }
 
-        /// <summary>
-        /// Location of element.
-        /// </summary>
+        /// <inheritdoc />
         public override string ToString()
         {
             return LocationString;
         }
 
-        /// <summary>
-        /// 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.
-        /// </summary>
+        /// <inheritdoc />
         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;
@@ -156,7 +148,7 @@ void ITranslatable.Translate(ITranslator translator)
         /// </summary>
         internal static ElementLocation FactoryForDeserialization(ITranslator translator)
         {
-            string file = null;
+            string? file = null;
             int line = 0;
             int column = 0;
             translator.Translate(ref file);
@@ -185,116 +177,50 @@ 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.
         /// </remarks>
-        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)
+            // 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;
             }
 
-            if (line <= 65535 && column <= 65535)
-            {
-                return new ElementLocation.SmallElementLocation(file, line, column);
-            }
+            // When combinedValue is negative, it implies that either line or column were negative
+            ErrorUtilities.VerifyThrow(combinedValue > -1, "Use zero for unknown");
 
-            return new ElementLocation.RegularElementLocation(file, line, column);
-        }
+            file ??= "";
 
-        /// <summary>
-        /// 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.
-        /// </summary>
-        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)
+            // When combinedValue is less than a threshold, it implies that both line and column are less
+            // than that threshold.
+            if (combinedValue <= ushort.MaxValue)
             {
-                locationString = file + " (" + line + ")";
-            }
-            else
-            {
-                locationString = file;
+                return new SmallElementLocation(file, line, column);
             }
 
-            return locationString;
+            return new RegularElementLocation(file, line, column);
         }
 
         /// <summary>
         /// Rarer variation for when the line and column won't each fit in a ushort.
         /// </summary>
-        private class RegularElementLocation : ElementLocation
+        private sealed class RegularElementLocation(string file, int line, int column) : ElementLocation
         {
-            /// <summary>
-            /// The source file.
-            /// </summary>
-            private string file;
-
-            /// <summary>
-            /// The source line.
-            /// </summary>
-            private int line;
-
-            /// <summary>
-            /// The source column.
-            /// </summary>
-            private int column;
-
-            /// <summary>
-            /// 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.
-            /// </summary>
-            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.line = line;
-                this.column = column;
-            }
-
-            /// <summary>
-            /// 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.
-            /// </summary>
+            /// <inheritdoc />
             [DebuggerBrowsable(DebuggerBrowsableState.Never)]
-            public override string File
-            {
-                get { return file; }
-            }
+            public override string File { get; } = file;
 
-            /// <summary>
-            /// The line number where this element exists in its file.
-            /// The first line is numbered 1.
-            /// Zero indicates "unknown location".
-            /// </summary>
+            /// <inheritdoc />
             [DebuggerBrowsable(DebuggerBrowsableState.Never)]
-            public override int Line
-            {
-                get { return line; }
-            }
+            public override int Line { get; } = line;
 
-            /// <summary>
-            /// The column number where this element exists in its file.
-            /// The first column is numbered 1.
-            /// Zero indicates "unknown location".
-            /// </summary>
+            /// <inheritdoc />
             [DebuggerBrowsable(DebuggerBrowsableState.Never)]
-            public override int Column
-            {
-                get { return column; }
-            }
+            public override int Column { get; } = column;
         }
 
         /// <summary>
@@ -306,70 +232,46 @@ public override 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.
         /// </summary>
-        private class SmallElementLocation : ElementLocation
+        private sealed class SmallElementLocation : ElementLocation
         {
             /// <summary>
-            /// The source file.
-            /// </summary>
-            private string file;
-
-            /// <summary>
-            /// The source line.
-            /// </summary>
-            private ushort line;
-
-            /// <summary>
-            /// 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.
             /// </summary>
-            private ushort column;
+            /// <remarks>
+            /// If we had two <see cref="ushort"/> 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.
+            /// </remarks>
+            private int packedData;
 
             /// <summary>
             /// 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.
             /// </summary>
             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;
-                this.line = Convert.ToUInt16(line);
-                this.column = Convert.ToUInt16(column);
+                File = file;
+                packedData = (line << 16) | column;
             }
 
-            /// <summary>
-            /// 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.
-            /// </summary>
+            /// <inheritdoc />
             [DebuggerBrowsable(DebuggerBrowsableState.Never)]
-            public override string File
-            {
-                get { return file; }
-            }
+            public override string File { get; }
 
-            /// <summary>
-            /// The line number where this element exists in its file.
-            /// The first line is numbered 1.
-            /// Zero indicates "unknown location".
-            /// </summary>
+            /// <inheritdoc />
             [DebuggerBrowsable(DebuggerBrowsableState.Never)]
             public override int Line
             {
-                get { return (int)line; }
+                get { return (packedData >> 16) & 0xFFFF; }
             }
 
-            /// <summary>
-            /// The column number where this element exists in its file.
-            /// The first column is numbered 1.
-            /// Zero indicates "unknown location".
-            /// </summary>
+            /// <inheritdoc />
             [DebuggerBrowsable(DebuggerBrowsableState.Never)]
             public override int Column
             {
-                get { return (int)column; }
+                get { return packedData & ushort.MaxValue; }
             }
         }
     }

src/Shared/ErrorUtilities.cs

@@ -537,17 +537,19 @@ internal static void VerifyThrowArgumentInvalidPath(string parameter, string par
             }
         }
 
+#nullable enable
         /// <summary>
         /// Throws an ArgumentException if the string has zero length, unless it is
         /// null, in which case no exception is thrown.
         /// </summary>
-        internal static void VerifyThrowArgumentLengthIfNotNull(string parameter, string parameterName)
+        internal static void VerifyThrowArgumentLengthIfNotNull(string? parameter, string parameterName)
         {
             if (parameter?.Length == 0)
             {
                 ThrowArgumentLength(parameterName);
             }
         }
+#nullable disable
 
         /// <summary>
         /// Throws an ArgumentNullException if the given parameter is null.