Release FileAccessMemory after starting the process (#1182)

Public/Src/Engine/Processes/FileAccessManifest.cs

@@ -900,6 +900,32 @@ namespace BuildXL.Processes
             }
         }
 
+        /// <summary>
+        /// Explicitly releases most of its memory.
+        /// </summary>
+        internal void Release()
+        {
+            m_normalizedFragments?.Clear();
+            var workList = new Stack<Node>();
+            workList.Push(m_rootNode);
+            while (workList.Count > 0)
+            {
+                var node = workList.Pop();
+                if (node == null)
+                {
+                    continue;
+                }
+                if (node.Children != null)
+                {
+                    foreach (var child in node.Children)
+                    {
+                        workList.Push(child);
+                    }
+                }
+                node.ReleaseChildren();
+            }
+        }
+
         /// <summary>
         /// Serializes this manifest.
         /// </summary>
@@ -1249,6 +1275,14 @@ namespace BuildXL.Processes
             /// </summary>
             internal IEnumerable<Node> Children => m_children?.Values;
 
+            /// <summary>
+            /// Releases all children nodes, allowing all that memory to be reclaimed by the garbage collector.
+            /// </summary>
+            internal void ReleaseChildren()
+            {
+                m_children?.Clear();
+            }
+
             /// <summary>
             /// The path ID as understood by the owning path table.
             /// </summary>

Public/Src/Engine/Processes/SandboxedProcessMac.cs

@@ -58,20 +58,19 @@ namespace BuildXL.Processes
 
         private long m_processKilledFlag = 0;
 
-        /// <summary>
-        /// Returns the associated PipId
-        /// </summary>
-        private long PipId => ProcessInfo.FileAccessManifest.PipId;
-
         private ulong m_processExitTimeNs = ulong.MaxValue;
 
         private bool HasProcessExitBeenReceived => m_processExitTimeNs != ulong.MaxValue;
 
         private readonly CancellationTokenSource m_timeoutTaskCancelationSource = new CancellationTokenSource();
 
-        private ISandboxConnection SandboxConnection => ProcessInfo.SandboxConnection;
+        private long PipId { get; }
 
-        private TimeSpan ChildProcessTimeout => ProcessInfo.NestedProcessTerminationTimeout;
+        private ISandboxConnection SandboxConnection { get; }
+
+        private TimeSpan ChildProcessTimeout { get; }
+
+        private TimeSpan? ReportQueueProcessTimeoutForTests { get; }
 
         /// <summary>
         /// Accumulates the time (in microseconds) access reports spend in the report queue
@@ -87,7 +86,7 @@ namespace BuildXL.Processes
         /// Timeout period for inactivity from the sandbox kernel extension.
         /// </summary>
         internal TimeSpan ReportQueueProcessTimeout => SandboxConnection.IsInTestMode
-            ? ProcessInfo.ReportQueueProcessTimeoutForTests ?? TimeSpan.FromSeconds(100)
+            ? ReportQueueProcessTimeoutForTests ?? TimeSpan.FromSeconds(100)
             : TimeSpan.FromMinutes(45);
 
         private Task m_processTreeTimeoutTask;
@@ -95,7 +94,7 @@ namespace BuildXL.Processes
         /// <summary>
         /// Allowed surviving child process names.
         /// </summary>
-        private string[] AllowedSurvivingChildProcessNames => ProcessInfo.AllowedSurvivingChildProcessNames;
+        private string[] AllowedSurvivingChildProcessNames { get; }
 
         private bool IgnoreReportedAccesses { get; }
 
@@ -114,6 +113,11 @@ namespace BuildXL.Processes
             Contract.Requires(info.FileAccessManifest != null);
             Contract.Requires(info.SandboxConnection != null);
 
+            PipId = info.FileAccessManifest.PipId;
+            SandboxConnection = info.SandboxConnection;
+            ChildProcessTimeout = info.NestedProcessTerminationTimeout;
+            AllowedSurvivingChildProcessNames = info.AllowedSurvivingChildProcessNames;
+            ReportQueueProcessTimeoutForTests = info.ReportQueueProcessTimeoutForTests;
             IgnoreReportedAccesses = ignoreReportedAccesses;
 
             MeasureCpuTime = overrideMeasureTime.HasValue
@@ -145,7 +149,7 @@ namespace BuildXL.Processes
                 });
 
             // install a 'ProcessStarted' handler that informs the sandbox of the newly started process
-            ProcessStarted += () => OnProcessStartedAsync().GetAwaiter().GetResult();
+            ProcessStarted += (pid) => OnProcessStartedAsync(info).GetAwaiter().GetResult();
         }
 
         private void UpdatePerfCounters()
@@ -180,9 +184,9 @@ namespace BuildXL.Processes
         }
 
         /// <inheritdoc />
-        protected override System.Diagnostics.Process CreateProcess()
+        protected override System.Diagnostics.Process CreateProcess(SandboxedProcessInfo info)
         {
-            var process = base.CreateProcess();
+            var process = base.CreateProcess(info);
 
             process.StartInfo.FileName = "/bin/sh";
             process.StartInfo.Arguments = string.Empty;
@@ -199,7 +203,7 @@ namespace BuildXL.Processes
         /// piped to its standard input.  Therefore, in this handler we first notify the sandbox of the new process (so that
         /// it starts tracking it) and then just send the actual process command line to /bin/sh via its standard input.
         /// </summary>
-        private async Task OnProcessStartedAsync()
+        private async Task OnProcessStartedAsync(SandboxedProcessInfo info)
         {
             // Generate "Process Created" report because the rest of the system expects to see it before any other file access reports
             //
@@ -209,7 +213,7 @@ namespace BuildXL.Processes
             ReportProcessCreated();
 
             // Allow read access for /bin/sh
-            ProcessInfo.FileAccessManifest.AddPath(
+            info.FileAccessManifest.AddPath(
                 AbsolutePath.Create(PathTable, Process.StartInfo.FileName),
                 mask: FileAccessPolicy.MaskNothing,
                 values: FileAccessPolicy.AllowReadAlways);
@@ -219,14 +223,16 @@ namespace BuildXL.Processes
                 m_perfCollector.Start();
             }
 
-            if (!SandboxConnection.NotifyPipStarted(ProcessInfo.FileAccessManifest, this))
+            string processStdinFileName = await FlushStandardInputToFileIfNeededAsync(info);
+
+            if (!SandboxConnection.NotifyPipStarted(info.FileAccessManifest, this))
             {
                 ThrowCouldNotStartProcess("Failed to notify kernel extension about process start, make sure the extension is loaded");
             }
 
             try
             {
-                await FeedStdInAsync();
+                await FeedStdInAsync(info, processStdinFileName);
                 m_processTreeTimeoutTask = ProcessTreeTimeoutTask();
             }
             catch (IOException e)
@@ -236,6 +242,14 @@ namespace BuildXL.Processes
                 LogProcessState($"IOException caught while feeding the standard input: {e.ToString()}");
                 await KillAsync();
             }
+            finally
+            {
+                // release the FileAccessManifest memory
+                // NOTE: just by not keeping any references to 'info' should make the FileAccessManifest object 
+                //       unreachable and thus available for garbage collection.  We call Release() here explicitly 
+                //       just to emphasise the importance of reclaiming this memory.
+                info.FileAccessManifest.Release();
+            }
         }
 
         /// <inheritdoc />
@@ -375,13 +389,12 @@ namespace BuildXL.Processes
             }
         }
 
-        private async Task FeedStdInAsync()
+        private async Task FeedStdInAsync(SandboxedProcessInfo info, [CanBeNull] string processStdinFileName)
         {
-            string processStdinFileName = await FlushStandardInputToFileIfNeededAsync();
             string redirectedStdin      = processStdinFileName != null ? $" < {processStdinFileName}" : string.Empty;
-            string escapedArguments     = ProcessInfo.Arguments.Replace(Environment.NewLine, "\\" + Environment.NewLine);
+            string escapedArguments     = info.Arguments.Replace(Environment.NewLine, "\\" + Environment.NewLine);
 
-            string line = I($"exec {ProcessInfo.FileName} {escapedArguments} {redirectedStdin}");
+            string line = I($"exec {info.FileName} {escapedArguments} {redirectedStdin}");
 
             LogProcessState("Feeding stdin: " + line);
             await Process.StandardInput.WriteLineAsync(line);
@@ -550,7 +563,7 @@ namespace BuildXL.Processes
 
         private void HandleAccessReport(AccessReport report)
         {
-            if (ProcessInfo.FileAccessManifest.ReportFileAccesses)
+            if (ShouldReportFileAccesses)
             {
                 LogProcessState("Access report received: " + AccessReportToString(report));
             }
@@ -650,16 +663,16 @@ namespace BuildXL.Processes
         /// If <see cref="SandboxedProcessInfo.StandardInputReader"/> is set, it flushes the content of that reader
         /// to a file in the process's working directory and returns the absolute path of that file; otherwise returns null.
         /// </summary>
-        private async Task<string> FlushStandardInputToFileIfNeededAsync()
+        private async Task<string> FlushStandardInputToFileIfNeededAsync(SandboxedProcessInfo info)
         {
-            if (ProcessInfo.StandardInputReader == null)
+            if (info.StandardInputReader == null)
             {
                 return null;
             }
 
-            string stdinFileName = Path.Combine(Process.StartInfo.WorkingDirectory, ProcessInfo.PipSemiStableHash + ".stdin");
-            string stdinText = await ProcessInfo.StandardInputReader.ReadToEndAsync();
-            Encoding encoding = ProcessInfo.StandardInputEncoding ?? Console.InputEncoding;
+            string stdinFileName = Path.Combine(Process.StartInfo.WorkingDirectory, info.PipSemiStableHash + ".stdin");
+            string stdinText = await info.StandardInputReader.ReadToEndAsync();
+            Encoding encoding = info.StandardInputEncoding ?? Console.InputEncoding;
             byte[] stdinBytes = encoding.GetBytes(stdinText);
             bool stdinFileWritten = await FileUtilities.WriteAllBytesAsync(stdinFileName, stdinBytes);
             if (!stdinFileWritten)
@@ -668,7 +681,7 @@ namespace BuildXL.Processes
             }
 
             // Allow read from the created stdin file
-            ProcessInfo.FileAccessManifest.AddPath(AbsolutePath.Create(PathTable, stdinFileName), mask: FileAccessPolicy.MaskNothing, values: FileAccessPolicy.AllowRead);
+            info.FileAccessManifest.AddPath(AbsolutePath.Create(PathTable, stdinFileName), mask: FileAccessPolicy.MaskNothing, values: FileAccessPolicy.AllowRead);
 
             return stdinFileName;
         }

Public/Src/Engine/Processes/UnSandboxedProcess.cs

@@ -11,7 +11,7 @@ using System.Threading.Tasks;
 using BuildXL.Interop;
 using BuildXL.Utilities;
 using BuildXL.Utilities.Configuration.Mutable;
-using static BuildXL.Utilities.FormattableStringEx;
+using BuildXL.Utilities.Instrumentation.Common;
 using JetBrains.Annotations;
 #if FEATURE_SAFE_PROCESS_HANDLE
 using Microsoft.Win32.SafeHandles;
@@ -22,6 +22,8 @@ using SafeProcessHandle = BuildXL.Interop.Windows.SafeProcessHandle;
 using static BuildXL.Interop.MacOS.IO;
 #endif
 
+using static BuildXL.Utilities.FormattableStringEx;
+
 namespace BuildXL.Processes
 {
     /// <summary>
@@ -34,10 +36,9 @@ namespace BuildXL.Processes
 
         private readonly SandboxedProcessOutputBuilder m_output;
         private readonly SandboxedProcessOutputBuilder m_error;
+        private readonly AsyncProcessExecutor m_processExecutor;
 
         private DateTime m_reportsReceivedTime;
-
-        private AsyncProcessExecutor m_processExecutor;
         private Exception m_dumpCreationException;
 
         internal class CpuTimes
@@ -57,7 +58,7 @@ namespace BuildXL.Processes
         /// <summary>
         /// Delegate type for <see cref="ProcessStarted"/> event.
         /// </summary>
-        protected delegate void ProcessStartedHandler();
+        protected delegate void ProcessStartedHandler(int processId);
 
         /// <summary>
         /// Raised right after the process is started.
@@ -70,20 +71,35 @@ namespace BuildXL.Processes
         /// </summary>
         protected virtual bool Killed => m_processExecutor?.Killed ?? false;
 
-        /// <summary>
-        /// Process information associated with this process.
-        /// </summary>
-        protected SandboxedProcessInfo ProcessInfo { get; }
-
         /// <summary>
         /// Underlying managed <see cref="Process"/> object.
         /// </summary>
         protected Process Process => m_processExecutor?.Process;
 
+        /// <summary>
+        /// Logging context from the <see cref="SandboxedProcessInfo"/> object passed to the constructor.
+        /// </summary>
+        protected LoggingContext LoggingContext { get; }
+
+        /// <summary>
+        /// Pip description from the <see cref="SandboxedProcessInfo"/> object passed to the constructor.
+        /// </summary>
+        protected string PipDescription { get; }
+
+        /// <summary>
+        /// Pip's semi-stable hash from the <see cref="SandboxedProcessInfo"/> object passed to the constructor.
+        /// </summary>
+        protected long PipSemiStableHash { get; }
+
         /// <summary>
         /// Returns the path table from the supplied <see cref="SandboxedProcessInfo"/>.
         /// </summary>
-        protected PathTable PathTable => ProcessInfo.PathTable;
+        protected PathTable PathTable { get; }
+
+        /// <summary>
+        /// Whether /logObservedFileAccesses has been requested
+        /// </summary>
+        protected bool ShouldReportFileAccesses { get; }
 
         /// <summary>
         /// Whether there were any failures regarding sandboxing (e.g., sandbox
@@ -91,33 +107,59 @@ namespace BuildXL.Processes
         /// </summary>
         protected virtual bool HasSandboxFailures => false;
 
-        /// <nodoc />
+        private string TimeoutDumpDirectory { get; }
+
+        /// <remarks>
+        /// IMPORTANT: For memory efficiency reasons don't keep a reference to <paramref name="info"/>
+        ///            or its  <see cref="SandboxedProcessInfo.FileAccessManifest"/> property
+        ///            (at least not after the process has been started)
+        /// </remarks>
         public UnsandboxedProcess(SandboxedProcessInfo info)
         {
             Contract.Requires(info != null);
 
+            Started = false;
+            PathTable = info.PathTable;
+            LoggingContext = info.LoggingContext;
+            PipDescription = info.PipDescription;
+            PipSemiStableHash = info.PipSemiStableHash;
+            TimeoutDumpDirectory = info.TimeoutDumpDirectory;
+            ShouldReportFileAccesses = info.FileAccessManifest?.ReportFileAccesses == true;
+
             info.Timeout = info.Timeout ?? DefaultProcessTimeout;
-            ProcessInfo = info;
 
             m_output = new SandboxedProcessOutputBuilder(
-                ProcessInfo.StandardOutputEncoding ?? Console.OutputEncoding,
-                ProcessInfo.MaxLengthInMemory,
-                ProcessInfo.FileStorage,
+                info.StandardOutputEncoding ?? Console.OutputEncoding,
+                info.MaxLengthInMemory,
+                info.FileStorage,
                 SandboxedProcessFile.StandardOutput,
-                ProcessInfo.StandardOutputObserver);
+                info.StandardOutputObserver);
 
             m_error = new SandboxedProcessOutputBuilder(
-                ProcessInfo.StandardErrorEncoding ?? Console.OutputEncoding,
-                ProcessInfo.MaxLengthInMemory,
-                ProcessInfo.FileStorage,
+                info.StandardErrorEncoding ?? Console.OutputEncoding,
+                info.MaxLengthInMemory,
+                info.FileStorage,
                 SandboxedProcessFile.StandardError,
-                ProcessInfo.StandardErrorObserver);
+                info.StandardErrorObserver);
+
+            m_processExecutor = new AsyncProcessExecutor(
+                CreateProcess(info),
+                info.Timeout ?? DefaultProcessTimeout,
+                line => FeedStdOut(m_output, line),
+                line => FeedStdErr(m_error, line),
+                info.Provenance,
+                msg => LogProcessState(msg));
+
+            if (info.ProcessIdListener != null)
+            {
+                ProcessStarted += (pid) => info.ProcessIdListener(pid);
+            }
         }
 
         /// <summary>
         /// Whether this process has been started (i.e., the <see cref="Start"/> method has been called on it).
         /// </summary>
-        public bool Started => Process != null;
+        public bool Started { get; private set; }
 
         /// <summary>
         /// Difference between now and when the process was started.
@@ -133,19 +175,9 @@ namespace BuildXL.Processes
         {
             Contract.Requires(!Started, "Process was already started.  Cannot start process more than once.");
 
-            m_processExecutor = new AsyncProcessExecutor(
-                CreateProcess(),
-                ProcessInfo.Timeout ?? DefaultProcessTimeout,
-                line => FeedStdOut(m_output, line),
-                line => FeedStdErr(m_error, line),
-                ProcessInfo.Provenance,
-                msg => LogProcessState(msg));
-
+            Started = true;
             m_processExecutor.Start();
-
-            ProcessStarted?.Invoke();
-
-            ProcessInfo.ProcessIdListener?.Invoke(ProcessId);
+            ProcessStarted?.Invoke(ProcessId);
         }
 
         /// <inheritdoc />
@@ -227,8 +259,7 @@ namespace BuildXL.Processes
 
             await m_processExecutor.WaitForStdOutAndStdErrAsync();
 
-            var reportFileAccesses = ProcessInfo.FileAccessManifest?.ReportFileAccesses == true;
-            var fileAccesses = reportFileAccesses ? (reports?.FileAccesses ?? s_emptyFileAccessesSet) : null;
+            var fileAccesses = ShouldReportFileAccesses ? (reports?.FileAccesses ?? s_emptyFileAccessesSet) : null;
 
             return new SandboxedProcessResult
             {
@@ -247,7 +278,7 @@ namespace BuildXL.Processes
                 Processes                           = CoalesceProcesses(reports?.Processes),
                 MessageProcessingFailure            = reports?.MessageProcessingFailure,
                 DumpCreationException               = m_dumpCreationException,
-                DumpFileDirectory                   = ProcessInfo.TimeoutDumpDirectory,
+                DumpFileDirectory                   = TimeoutDumpDirectory,
                 PrimaryProcessTimes                 = GetProcessTimes(),
                 SurvivingChildProcesses             = CoalesceProcesses(GetSurvivingChildProcesses())
             };
@@ -269,7 +300,7 @@ namespace BuildXL.Processes
         {
             Contract.Requires(Started);
 
-            ProcessDumper.TryDumpProcessAndChildren(ProcessId, ProcessInfo.TimeoutDumpDirectory, out m_dumpCreationException);
+            ProcessDumper.TryDumpProcessAndChildren(ProcessId, TimeoutDumpDirectory, out m_dumpCreationException);
 
             LogProcessState($"UnsandboxedProcess::KillAsync()");
             return m_processExecutor.KillAsync();
@@ -278,22 +309,22 @@ namespace BuildXL.Processes
         /// <summary>
         /// Creates a <see cref="Process"/>.
         /// </summary>
-        protected virtual Process CreateProcess()
+        protected virtual Process CreateProcess(SandboxedProcessInfo info)
         {
-            Contract.Requires(Process == null);
+            Contract.Requires(!Started);
 
 #if !PLATFORM_WIN
-            var mode = GetFilePermissionsForFilePath(ProcessInfo.FileName, followSymlink: false);
+            var mode = GetFilePermissionsForFilePath(info.FileName, followSymlink: false);
             if (mode < 0)
             {
-                ThrowBuildXLException($"Process creation failed: File '{ProcessInfo.FileName}' not found", new Win32Exception(0x2));
+                ThrowBuildXLException($"Process creation failed: File '{info.FileName}' not found", new Win32Exception(0x2));
             }
 
             var filePermissions = checked((FilePermissions)mode);
             FilePermissions exePermission = FilePermissions.S_IXUSR;
             if (!filePermissions.HasFlag(exePermission))
             {
-                SetFilePermissionsForFilePath(ProcessInfo.FileName, exePermission);
+                SetFilePermissionsForFilePath(info.FileName, exePermission);
             }
 #endif
 
@@ -301,9 +332,9 @@ namespace BuildXL.Processes
             {
                 StartInfo = new ProcessStartInfo
                 {
-                    FileName = ProcessInfo.FileName,
-                    Arguments = ProcessInfo.Arguments,
-                    WorkingDirectory = ProcessInfo.WorkingDirectory,
+                    FileName = info.FileName,
+                    Arguments = info.Arguments,
+                    WorkingDirectory = info.WorkingDirectory,
                     StandardErrorEncoding = m_output.Encoding,
                     StandardOutputEncoding = m_error.Encoding,
                     RedirectStandardError = true,
@@ -315,9 +346,9 @@ namespace BuildXL.Processes
             };
 
             process.StartInfo.EnvironmentVariables.Clear();
-            if (ProcessInfo.EnvironmentVariables != null)
+            if (info.EnvironmentVariables != null)
             {
-                foreach (var envKvp in ProcessInfo.EnvironmentVariables.ToDictionary())
+                foreach (var envKvp in info.EnvironmentVariables.ToDictionary())
                 {
                     process.StartInfo.EnvironmentVariables[envKvp.Key] = envKvp.Value;
                 }
@@ -350,7 +381,7 @@ namespace BuildXL.Processes
         protected void ThrowBuildXLException(string message, Exception inner = null)
         {
             Process?.StandardInput?.Close();
-            throw new BuildXLException($"[Pip{ProcessInfo.PipSemiStableHash} -- {ProcessInfo.PipDescription}] {message}", inner);
+            throw new BuildXLException($"[Pip{PipSemiStableHash:X} -- {PipDescription}] {message}", inner);
         }
 
         /// <nodoc />
@@ -359,10 +390,10 @@ namespace BuildXL.Processes
         /// <nodoc />
         protected void LogProcessState(string message)
         {
-            if (ProcessInfo.LoggingContext != null)
+            if (LoggingContext != null)
             {
                 string fullMessage = I($"Exited: {m_processExecutor?.ExitCompleted ?? false}, StdOut: {m_processExecutor?.StdOutCompleted ?? false}, StdErr: {m_processExecutor?.StdErrCompleted ?? false}, Reports: {ReportsCompleted()} :: {message}");
-                Tracing.Logger.Log.LogDetoursDebugMessage(ProcessInfo.LoggingContext, ProcessInfo.PipSemiStableHash, ProcessInfo.PipDescription, fullMessage);
+                Tracing.Logger.Log.LogDetoursDebugMessage(LoggingContext, PipSemiStableHash, PipDescription, fullMessage);
             }
         }
 

bxl.sh

@@ -10,7 +10,8 @@ source "$MY_DIR/Public/Src/Sandbox/MacOs/scripts/env.sh"
 
 declare arg_Positional=()
 declare arg_DeployDev=""
-declare arg_UseDev=()
+declare arg_DeployDevRelease=""
+declare arg_UseDev=""
 declare arg_Minimal=""
 declare arg_Internal=""
 
@@ -97,6 +98,10 @@ function parseArgs() {
             arg_DeployDev="1"
             shift
             ;;
+        --deploy-dev-release)
+            arg_DeployDevRelease="1"
+            shift
+            ;;
         --use-dev)
             arg_UseDev="1"
             shift
@@ -123,7 +128,7 @@ function deployBxl { # (fromDir, toDir)
 
     mkdir -p "$toDir"
     /usr/bin/rsync -arhq "$fromDir/" "$toDir" --delete
-    print_info "Successfully deployed developer build to: $toDir; use it with the '--use-dev' flag now."
+    print_info "Successfully deployed developer build from $fromDir to: $toDir; use it with the '--use-dev' flag now."
 }
 
 parseArgs "$@"
@@ -134,6 +139,10 @@ if [[ -n "$arg_DeployDev" || -n "$arg_Minimal" ]]; then
     setMinimal
 fi
 
+if [[ -n "$arg_DeployDevRelease" ]]; then
+    arg_Positional+=(/q:ReleaseDotNetCoreMac "/f:output='$MY_DIR/Out/bin/release/osx-x64/*'")
+fi
+
 if [[ -n "$arg_Internal" ]]; then
     setInternal $@
 fi
@@ -155,4 +164,8 @@ if [[ -n "$arg_DeployDev" ]]; then
     deployBxl "$MY_DIR/Out/Bin/debug/osx-x64" "$MY_DIR/Out/Selfhost/Dev"
 fi
 
+if [[ -n "$arg_DeployDevRelease" ]]; then
+    deployBxl "$MY_DIR/Out/Bin/release/osx-x64" "$MY_DIR/Out/Selfhost/Dev"
+fi
+
 popd