Undraft (tests, cleanup, documentation)

Author: ccastanedaucf

src/Build.UnitTests/BackEnd/BinaryTranslator_Tests.cs

@@ -51,40 +51,34 @@ internal static ITranslator GetWriteTranslator(Stream stream)
             return new BinaryWriteTranslator(stream);
         }
 
-        // TODO: Avoid expsoing write translator?
-        internal static ITranslator GetWriteTranslator(Stream stream, InterningWriteTranslator interner)
-        {
-            return new BinaryWriteTranslator(stream, interner, isInterning: true);
-        }
-
         /// <summary>
         /// Implementation of ITranslator for reading from a stream.
         /// </summary>
         private class BinaryReadTranslator : ITranslator
         {
             /// <summary>
-            /// The stream used as a source or destination for data.
+            /// The intern reader used in an intern scope.
             /// </summary>
-            private Stream _packetStream;
+            private readonly InterningReadTranslator _interner;
 
             /// <summary>
             /// The binary reader used in read mode.
             /// </summary>
             private BinaryReader _reader;
 
-            private InterningReadTranslator _interner;
-
-            public bool IsInterning { get; private set; }
+            /// <summary>
+            /// Whether the caller has entered an intern scope.
+            /// </summary>
+            private bool _isInterning;
 
 #nullable enable
             /// <summary>
             /// Constructs a serializer from the specified stream, operating in the designated mode.
             /// </summary>
             public BinaryReadTranslator(Stream packetStream, BinaryReaderFactory buffer)
             {
-                _packetStream = packetStream;
                 _reader = buffer.Create(packetStream);
-                _interner = new(this);
+                _interner = new InterningReadTranslator(this);
             }
 #nullable disable
 
@@ -803,71 +797,76 @@ public bool TranslateNullable<T>(T value)
 
             public void WithInterning(IEqualityComparer<string> comparer, int initialCapacity, Action<ITranslator> internBlock)
             {
-                if (IsInterning)
+                if (_isInterning)
                 {
                     throw new InvalidOperationException("Cannot enter recursive intern block.");
                 }
 
-                IsInterning = true;
+                _isInterning = true;
 
+                // Deserialize the intern header before entering the intern scope.
                 _interner.Translate(this);
+
+                // No other setup is needed since we can parse the packet directly from the stream.
                 internBlock(this);
 
-                IsInterning = false;
+                _isInterning = false;
             }
 
-            public void Intern(ref string str, bool nullable)
+            public void Intern(ref string str, bool nullable = true)
             {
-                if (!IsInterning)
+                if (!_isInterning)
                 {
                     Translate(ref str);
+                    return;
                 }
-                else if (nullable)
-                {
-                    str = _interner.ReadNullable();
-                }
-                else
+
+                if (nullable && !TranslateNullable(string.Empty))
                 {
-                    str = _interner.Read();
+                    str = null;
+                    return;
                 }
+
+                str = _interner.Read();
             }
 
             public void Intern(ref string[] array)
             {
-                if (!IsInterning)
+                if (!_isInterning)
                 {
                     Translate(ref array);
+                    return;
                 }
 
                 if (!TranslateNullable(array))
                 {
                     return;
                 }
 
-
                 int count = _reader.ReadInt32();
                 array = new string[count];
 
                 for (int i = 0; i < count; i++)
                 {
-                    array[i] = _interner.ReadNullable();
+                    array[i] = _interner.Read();
                 }
             }
 
-            public void InternPath(ref string str, bool nullable)
+            public void InternPath(ref string str, bool nullable = true)
             {
-                if (!IsInterning)
+                if (!_isInterning)
                 {
                     Translate(ref str);
+                    return;
                 }
-                else if (nullable)
-                {
-                    str = _interner.ReadNullablePath();
-                }
-                else
+
+                if (nullable && !TranslateNullable(string.Empty))
                 {
-                    str = _interner.ReadPath();
+                    str = null;
+                    return;
                 }
+
+                str = _interner.ReadPath();
             }
         }
 
@@ -876,35 +875,30 @@ public void InternPath(ref string str, bool nullable)
         /// </summary>
         private class BinaryWriteTranslator : ITranslator
         {
-            /// <summary>
-            /// The stream used as a source or destination for data.
-            /// </summary>
-            private Stream _packetStream;
-
             /// <summary>
             /// The binary writer used in write mode.
             /// </summary>
             private BinaryWriter _writer;
 
-            private readonly InterningWriteTranslator _interner = new();
+            /// <summary>
+            /// The intern writer used in an intern scope.
+            /// This must be lazily instantiated since the interner has its own internal write translator, and
+            /// would otherwise go into a recursive loop on initalization.
+            /// </summary>
+            private InterningWriteTranslator _interner;
 
-            public bool IsInterning { get; private set; }
+            /// <summary>
+            /// Whether the caller has entered an intern scope.
+            /// </summary>
+            private bool _isInterning;
 
             /// <summary>
             /// Constructs a serializer from the specified stream, operating in the designated mode.
             /// </summary>
             /// <param name="packetStream">The stream serving as the source or destination of data.</param>
             public BinaryWriteTranslator(Stream packetStream)
-                : this(packetStream, new InterningWriteTranslator())
-            {
-            }
-
-            internal BinaryWriteTranslator(Stream packetStream, InterningWriteTranslator interner, bool isInterning = false)
             {
-                _packetStream = packetStream;
                 _writer = new BinaryWriter(packetStream);
-                _interner = interner;
-                IsInterning = isInterning;
             }
 
             /// <summary>
@@ -1605,67 +1599,88 @@ public bool TranslateNullable<T>(T value)
 
             public void WithInterning(IEqualityComparer<string> comparer, int initialCapacity, Action<ITranslator> internBlock)
             {
-                if (IsInterning)
+                if (_isInterning)
                 {
                     throw new InvalidOperationException("Cannot enter recursive intern block.");
                 }
 
-                _interner.InitCapacity(comparer, initialCapacity);
-                internBlock(_interner.Translator);
+                // Every new scope requires the interner's state to be reset.
+                _interner ??= new InterningWriteTranslator();
+                _interner.Setup(comparer, initialCapacity);
+
+                // Temporaily swap our writer with the interner.
+                // This forwards all writes to this translator into the interning buffer, so that any non-interned
+                // writes which are interleaved will be in the correct order.
+                BinaryWriter streamWriter = _writer;
+                _writer = _interner.Writer;
+                _isInterning = true;
+
+                try
+                {
+                    internBlock(this);
+                }
+                finally
+                {
+                    _writer = streamWriter;
+                    _isInterning = false;
+                }
+
+                // Write the interned buffer into the real output stream.
                 _interner.Translate(this);
             }
 
-            public void Intern(ref string str, bool nullable)
+            public void Intern(ref string str, bool nullable = true)
             {
-                if (!IsInterning)
+                if (!_isInterning)
                 {
                     Translate(ref str);
+                    return;
                 }
-                else if (nullable)
-                {
-                    _interner.InternNullable(str);
-                }
-                else
+
+                if (nullable && !TranslateNullable(str))
                 {
-                    _interner.Intern(str);
+                    return;
                 }
+
+                _interner.Intern(str);
             }
 
             public void Intern(ref string[] array)
             {
-                if (!IsInterning)
+                if (!_isInterning)
                 {
                     Translate(ref array);
+                    return;
                 }
 
-                if (!_interner.Translator.TranslateNullable(array))
+                if (!TranslateNullable(array))
                 {
                     return;
                 }
 
                 int count = array.Length;
-                _interner.Translator.Translate(ref count);
+                Translate(ref count);
 
                 for (int i = 0; i < count; i++)
                 {
-                    _interner.InternNullable(array[i]);
+                    _interner.Intern(array[i]);
                 }
             }
 
-            public void InternPath(ref string str, bool nullable)
+            public void InternPath(ref string str, bool nullable = true)
             {
-                if (!IsInterning)
+                if (!_isInterning)
                 {
                     Translate(ref str);
+                    return;
                 }
-                else if (nullable)
-                {
-                    _interner.InternNullablePath(str);
-                }
-                else
+
+                if (nullable && !TranslateNullable(str))
                 {
-                    _interner.InternPath(str);
+                    return;
                 }
+
+                _interner.InternPath(str);
             }
         }
     }

src/Framework/BinaryTranslator.cs

@@ -7,8 +7,6 @@
 using System.IO;
 using Microsoft.Build.Framework;
 
-#nullable disable
-
 namespace Microsoft.Build.BackEnd
 {
     /// <summary>
@@ -98,8 +96,6 @@ BinaryWriter Writer
             get;
         }
 
-        bool IsInterning { get; }
-
         /// <summary>
         /// Translates a boolean.
         /// </summary>
@@ -369,13 +365,58 @@ void TranslateDictionary<D, T>(ref D dictionary, ObjectTranslator<T> objectTrans
         /// <returns>True if the object should be written, false otherwise.</returns>
         bool TranslateNullable<T>(T value);
 
-        void Intern(ref string str, bool nullable = false);
-
-        void Intern(ref string[] array);
+        /// <summary>
+        /// Creates a scope which activates string interning / deduplication for any Intern_xx method.
+        /// This should generally be called from the root level packet.
+        /// </summary>
+        /// <param name="comparer">The strseaparating comparer to use when populating the intern cache.</param>
+        /// <param name="initialCapacity">The initial capacity of the intern cache.</param>
+        /// <param name="internBlock">A delegate providing a translator, in which all Intern_xx calls will go through the intern cache.</param>
+        /// <remarks>
+        /// Packet interning is implemented via a header with an array of all interned strings, followed by the body in
+        /// which any interned / duplicated strings are replaced by their ID.
+        /// <see cref="TranslationDirection"/> modes have different ordering requirements, so it would not be
+        /// possible to implement direction-agnostic serialization via the Intern_xx methods alone:
+        /// - Write: Because we don't know the full list of strings ahead of time, we need to create a temporary buffer
+        ///   for the packet body, which we can later offset when flushing to the real stream.
+        /// - Read: The intern header needs to be deserialized before the packet body, otherwise we won't know what
+        ///   string each ID maps to.
+        /// This method abstracts these requirements to the caller, such that the underlying translator will
+        /// automatically handle the appropriate IO ordering when entering / exiting the delegate scope.
+        /// </remarks>
+        void WithInterning(IEqualityComparer<string> comparer, int initialCapacity, Action<ITranslator> internBlock);
 
+        /// <summary>
+        /// Interns the string if the translator is currently within an intern block.
+        /// Otherwise, this forwards to the regular Translate method.
+        /// </summary>
+        /// <param name="str">The value to be translated.</param>
+        /// <param name="nullable">
+        /// Whether to null check and translate the nullable marker.
+        /// Setting this to false can reduce packet sizes when interning large numbers of strings
+        /// which are validated to always be non-null, such as dictionary keys.
+        /// </param>
+        void Intern(ref string str, bool nullable = true);
 
-        void InternPath(ref string str, bool nullable = false);
+        /// <summary>
+        /// Interns each string in the array if the translator is currently within an intern block.
+        /// Otherwise, this forwards to the regular Translate method. To match behavior, all strings
+        /// assumed to be non-null.
+        /// </summary>
+        /// <param name="array">The array to be translated.</param>
+        void Intern(ref string[] array);
 
-        void WithInterning(IEqualityComparer<string> comparer, int initialCapacity, Action<ITranslator> internBlock);
+        /// <summary>
+        /// Interns the string if the translator is currently within an intern block.
+        /// Otherwise, this forwards to the regular Translate method.
+        /// If the string is determined to be path-like, the path components will be interned separately.
+        /// </summary>
+        /// <param name="str">The value to be translated.</param>
+        /// <param name="nullable">
+        /// Whether to null check and translate the nullable marker.
+        /// Setting this to false can reduce packet sizes when interning large numbers of strings
+        /// which are validated to always be non-null, such as dictionary keys.
+        /// </param>
+        void InternPath(ref string str, bool nullable = true);
     }
 }

src/Framework/ITranslator.cs

@@ -0,0 +1,7 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.Build.BackEnd
+{
+    internal readonly record struct InternPathIds(int DirectoryId, int FileNameId);
+}