Merge pull request #41955 from CyrusNajmabadi/dumpCommandLine

Persist command line strings to temporary storage to prevent excess in-memory use.

src/VisualStudio/Core/Def/Implementation/ProjectSystem/Legacy/AbstractLegacyProject_ICompilerOptionsHostObject.cs

@@ -10,7 +10,7 @@ internal partial class AbstractLegacyProject : ICompilerOptionsHostObject
     {
         int ICompilerOptionsHostObject.SetCompilerOptions(string compilerOptions, out bool supported)
         {
-            VisualStudioProjectOptionsProcessor.CommandLine = compilerOptions;
+            VisualStudioProjectOptionsProcessor.SetCommandLine(compilerOptions);
             supported = true;
             return VSConstants.S_OK;
         }

src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioProjectOptionsProcessor.cs

@@ -2,6 +2,8 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+#nullable enable
+
 using System;
 using System.Collections.Immutable;
 using System.IO;
@@ -16,57 +18,91 @@ internal class VisualStudioProjectOptionsProcessor : IDisposable
         private readonly VisualStudioProject _project;
         private readonly HostWorkspaceServices _workspaceServices;
         private readonly ICommandLineParserService _commandLineParserService;
+        private readonly ITemporaryStorageService _temporaryStorageService;
 
         /// <summary>
         /// Gate to guard all mutable fields in this class.
         /// The lock hierarchy means you are allowed to call out of this class and into <see cref="_project"/> while holding the lock.
         /// </summary>
         private readonly object _gate = new object();
-        private string _commandLine = "";
+
+        /// <summary>
+        /// A hashed checksum of the last command line we were set to.  We use this
+        /// as a low cost (in terms of memory) way to determine if the command line
+        /// actually changes and we need to make any downstream updates.
+        /// </summary>
+        private Checksum? _commandLineChecksum;
+
+        /// <summary>
+        /// To save space in the managed heap, we dump the entire command-line string to our
+        /// temp-storage-service. This is helpful as compiler command-lines can grow extremely large
+        /// (especially in cases with many references).
+        /// </summary>
+        /// <remarks>Note: this will be null in the case that the command line is an empty string.</remarks>
+        private ITemporaryStreamStorage? _commandLineStorage;
+
         private CommandLineArguments _commandLineArgumentsForCommandLine;
-        private string _explicitRuleSetFilePath;
-        private IReferenceCountedDisposable<ICacheEntry<string, IRuleSetFile>> _ruleSetFile = null;
+        private string? _explicitRuleSetFilePath;
+        private IReferenceCountedDisposable<ICacheEntry<string, IRuleSetFile>>? _ruleSetFile = null;
 
-        public VisualStudioProjectOptionsProcessor(VisualStudioProject project, HostWorkspaceServices workspaceServices)
+        public VisualStudioProjectOptionsProcessor(
+            VisualStudioProject project,
+            HostWorkspaceServices workspaceServices)
         {
             _project = project ?? throw new ArgumentNullException(nameof(project));
             _workspaceServices = workspaceServices;
             _commandLineParserService = workspaceServices.GetLanguageServices(project.Language).GetRequiredService<ICommandLineParserService>();
+            _temporaryStorageService = workspaceServices.GetRequiredService<ITemporaryStorageService>();
+
+            // Set up _commandLineArgumentsForCommandLine to a default. No lock taken since we're in
+            // the constructor so nothing can race.
 
-            // Set up _commandLineArgumentsForCommandLine to a default. No lock taken since we're in the constructor so nothing can race.
-            ReparseCommandLine_NoLock();
+            // Silence NRT warning.  This will be initialized by the call below to ReparseCommandLineIfChanged_NoLock.
+            _commandLineArgumentsForCommandLine = null!;
+            ReparseCommandLineIfChanged_NoLock(commandLine: "");
         }
 
-        public string CommandLine
+        /// <returns><see langword="true"/> if the command line was updated.</returns>
+        private bool ReparseCommandLineIfChanged_NoLock(string commandLine)
         {
-            get
+            var checksum = Checksum.Create(commandLine);
+            if (_commandLineChecksum == checksum)
+                return false;
+
+            _commandLineChecksum = checksum;
+
+            // Dispose the existing stored command-line and then persist the new one so we can
+            // recover it later.  Only bother persisting things if we have a non-empty string.
+
+            _commandLineStorage?.Dispose();
+            _commandLineStorage = null;
+            if (commandLine.Length > 0)
             {
-                return _commandLine;
+                _commandLineStorage = _temporaryStorageService.CreateTemporaryStreamStorage();
+                _commandLineStorage.WriteString(commandLine);
             }
 
-            set
-            {
-                if (value == null)
-                {
-                    throw new ArgumentNullException(nameof(value));
+            ReparseCommandLine_NoLock(commandLine);
+            return true;
         }
 
+        public void SetCommandLine(string commandLine)
+        {
+            if (commandLine == null)
+                throw new ArgumentNullException(nameof(commandLine));
+
             lock (_gate)
             {
-                    if (_commandLine == value)
+                // If we actually got a new command line, then update the project options, otherwise
+                // we don't need to do anything.
+                if (ReparseCommandLineIfChanged_NoLock(commandLine))
                 {
-                        return;
-                    }
-
-                    _commandLine = value;
-
-                    ReparseCommandLine_NoLock();
                     UpdateProjectOptions_NoLock();
                 }
             }
         }
 
-        public string ExplicitRuleSetFilePath
+        public string? ExplicitRuleSetFilePath
         {
             get => _explicitRuleSetFilePath;
 
@@ -89,7 +125,7 @@ public string ExplicitRuleSetFilePath
         /// <summary>
         /// Returns the active path to the rule set file that is being used by this project, or null if there isn't a rule set file.
         /// </summary>
-        public string EffectiveRuleSetFilePath
+        public string? EffectiveRuleSetFilePath
         {
             get
             {
@@ -112,9 +148,9 @@ private void DisposeOfRuleSetFile_NoLock()
             }
         }
 
-        private void ReparseCommandLine_NoLock()
+        private void ReparseCommandLine_NoLock(string commandLine)
         {
-            var arguments = CommandLineParser.SplitCommandLineIntoArguments(_commandLine, removeHashComments: false);
+            var arguments = CommandLineParser.SplitCommandLineIntoArguments(commandLine, removeHashComments: false);
             _commandLineArgumentsForCommandLine = _commandLineParserService.Parse(arguments, Path.GetDirectoryName(_project.FilePath), isInteractive: false, sdkDirectory: null);
         }
 
@@ -193,8 +229,10 @@ private void RuleSetFile_UpdatedOnDisk(object sender, EventArgs e)
                 // effective values was potentially done by the act of parsing the command line. Even though the command line didn't change textually,
                 // the effective result did. Then we call UpdateProjectOptions_NoLock to reapply any values; that will also re-acquire the new ruleset
                 // includes in the IDE so we can be watching for changes again.
+                var commandLine = _commandLineStorage?.ReadString() ?? "";
+
                 DisposeOfRuleSetFile_NoLock();
-                ReparseCommandLine_NoLock();
+                ReparseCommandLine_NoLock(commandLine);
                 UpdateProjectOptions_NoLock();
             }
         }
@@ -203,10 +241,8 @@ private void RuleSetFile_UpdatedOnDisk(object sender, EventArgs e)
         /// Overridden by derived classes to provide a hook to modify a <see cref="CompilationOptions"/> with any host-provided values that didn't come from
         /// the command line string.
         /// </summary>
-        protected virtual CompilationOptions ComputeCompilationOptionsWithHostValues(CompilationOptions compilationOptions, IRuleSetFile ruleSetFileOpt)
-        {
-            return compilationOptions;
-        }
+        protected virtual CompilationOptions ComputeCompilationOptionsWithHostValues(CompilationOptions compilationOptions, IRuleSetFile? ruleSetFileOpt)
+            => compilationOptions;
 
         /// <summary>
         /// Override by derived classes to provide a hook to modify a <see cref="ParseOptions"/> with any host-provided values that didn't come from 
@@ -234,6 +270,7 @@ public void Dispose()
             lock (_gate)
             {
                 DisposeOfRuleSetFile_NoLock();
+                _commandLineStorage?.Dispose();
             }
         }
     }

src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProject_IWorkspaceProjectContext.cs

@@ -139,12 +139,7 @@ public CPSProject(VisualStudioProject visualStudioProject, VisualStudioWorkspace
         public ProjectId Id => _visualStudioProject.Id;
 
         public void SetOptions(string commandLineForOptions)
-        {
-            if (_visualStudioProjectOptionsProcessor != null)
-            {
-                _visualStudioProjectOptionsProcessor.CommandLine = commandLineForOptions;
-            }
-        }
+            => _visualStudioProjectOptionsProcessor?.SetCommandLine(commandLineForOptions);
 
         public string? DefaultNamespace
         {

src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageServiceFactory.cs

@@ -364,11 +364,6 @@ private async Task WriteStreamMaybeAsync(Stream stream, bool useAsync, Cancellat
                         throw new InvalidOperationException(WorkspacesResources.Temporary_storage_cannot_be_written_more_than_once);
                     }
 
-                    if (stream.Length == 0)
-                    {
-                        throw new ArgumentOutOfRangeException();
-                    }
-
                     using (Logger.LogBlock(FunctionId.TemporaryStorageServiceFactory_WriteStream, cancellationToken))
                     {
                         var size = stream.Length;

src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/ITemporaryStreamStorageExtensions.cs

+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System.IO;
+
+namespace Microsoft.CodeAnalysis.Host
+{
+    internal static class ITemporaryStreamStorageExtensions
+    {
+        public static void WriteString(this ITemporaryStreamStorage storage, string value)
+        {
+            using var stream = SerializableBytes.CreateWritableStream();
+            using var writer = new StreamWriter(stream);
+
+            writer.Write(value);
+            writer.Flush();
+            stream.Position = 0;
+
+            storage.WriteStream(stream);
+        }
+
+        public static string ReadString(this ITemporaryStreamStorage storage)
+        {
+            using var stream = storage.ReadStream();
+            using var reader = new StreamReader(stream);
+            return reader.ReadToEnd();
+        }
+    }
+}

src/Workspaces/CoreTest/WorkspaceServiceTests/TemporaryStorageServiceTests.cs

@@ -13,7 +13,6 @@
 using Microsoft.CodeAnalysis.Test.Utilities;
 using Microsoft.CodeAnalysis.Text;
 using Roslyn.Test.Utilities;
-using Roslyn.Utilities;
 using Xunit;
 
 namespace Microsoft.CodeAnalysis.UnitTests
@@ -113,12 +112,8 @@ public void TestTemporaryStreamStorageExceptions()
             Assert.Throws<InvalidOperationException>(() => storage.ReadStream());
             Assert.Throws<AggregateException>(() => storage.ReadStreamAsync().Result);
 
-            // 0 length streams are not allowed
-            var stream = new MemoryStream();
-            Assert.Throws<ArgumentOutOfRangeException>(() => storage.WriteStream(stream));
-            Assert.Throws<AggregateException>(() => storage.WriteStreamAsync(stream).Wait());
-
             // write a normal stream
+            var stream = new MemoryStream();
             stream.Write(new byte[] { 42 }, 0, 1);
             stream.Position = 0;
             storage.WriteStreamAsync(stream).Wait();
@@ -128,6 +123,25 @@ public void TestTemporaryStreamStorageExceptions()
             Assert.Throws<AggregateException>(() => storage.WriteStreamAsync(null).Wait());
         }
 
+        [Fact]
+        public void TestZeroLengthStreams()
+        {
+            var textFactory = new TextFactoryService();
+            var service = new TemporaryStorageServiceFactory.TemporaryStorageService(textFactory);
+            var storage = service.CreateTemporaryStreamStorage(CancellationToken.None);
+
+            // 0 length streams are allowed
+            using (var stream1 = new MemoryStream())
+            {
+                storage.WriteStream(stream1);
+            }
+
+            using (var stream2 = storage.ReadStream())
+            {
+                Assert.Equal(0, stream2.Length);
+            }
+        }
+
         [Fact, Trait(Traits.Feature, Traits.Features.Workspace)]
         public void TestTemporaryStorageMemoryMappedFileManagement()
         {

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/SerializableBytes.cs

@@ -22,7 +22,6 @@ internal static class SerializableBytes
         internal static PooledStream CreateReadableStream(byte[] bytes)
             => CreateReadableStream(bytes, bytes.Length);
 
-
         internal static PooledStream CreateReadableStream(byte[] bytes, int length)
         {
             var stream = CreateWritableStream();