Cache process info where possible and dispose Process objects. #11274
Conversation
|
Related to #11160 |
| #endif | ||
|
|
||
| #if !NETCOREAPP | ||
| private static volatile int s_processId; |
There was a problem hiding this comment.
Are we fine with defaults or do we want to be explicit in the initial value?
e.g. private static volatile int s_processId = 0 or something.
SimaTian
force-pushed
the
dev/erarndt/disposeProcess
branch
from
25b098e to
8a74a3e
Compare
|
I tried rebasing and/or merging with main, which made a mess. |
| @@ -40,6 +40,7 @@ | |||
| <Compile Include="..\Shared\BinaryWriterExtensions.cs"> | |||
| <Link>Shared\BinaryWriterExtensions.cs</Link> | |||
| </Compile> | |||
| <Compile Include="..\Shared\EnvironmentUtilities.cs" Link="EnvironmentUtilities.cs" /> | |||
There was a problem hiding this comment.
Out of curiosity, how long is this construct around please? (e.g. in other places we use an extra tag, while here we use an attribute)
| using Process currentProcess = Process.GetCurrentProcess(); | ||
| Interlocked.CompareExchange(ref s_processPath, currentProcess.MainModule.FileName ?? "", null); | ||
| processPath = s_processPath; | ||
| Debug.Assert(processPath != null); |
There was a problem hiding this comment.
Under which circumstance would this happen? (I noticed that there is a check that throws in src/MSBuild/MSBuildClientApp.cs and here we have an assert stating that we really don't want this to happen while debugging.)
| @@ -859,6 +858,14 @@ private NodeEngineShutdownReason HandleShutdown() | |||
| #endif | |||
|
|
|||
| return _shutdownReason; | |||
|
|
|||
| static StreamWriter GetDebugWriter(bool debugCommunications) | |||
There was a problem hiding this comment.
Can we use something like
if (_debugCommunications)
{
using StreamWriter debugWriter = File.CreateText(string.Format(CultureInfo.CurrentCulture, Path.Combine(FileUtilities.TempFileDirectory, @"MSBuild_NodeShutdown_{0}.txt"), Process.GetCurrentProcess().Id))
debugWriter.WriteLine("Node shutting down with reason {0}.", _shutdownReason);
}
Or something else altogether if anyone has an idea for a cleaner answer.
I found it somewhat difficult to reason about the previous state or the proposed solution:
- _debugCommunications is a local private bool
- which we then feed into a static wrapper function
- that either gives us nothing or a stream
- and if that stream exists, we write into it
Fixes #
Context
There are multiple places where information about the current process is retrieved. There is a non-trivial cost to getting this information and the Process objects should be disposed to avoid additional GC cost due to the finalizer running. This change adds properties in EnvironmentUtilities that cache info such as the current process ID (this implementation is copied from .NET 6), and in some instances, updates some callers to dispose of the Process object for uncommon usages.
Changes Made
Testing
Notes