Add and use Environment.ProcessId

[stephentoub]

Fixes #38388

dotnet/roslyn-analyzers#3838 provides a Process.GetCurrentProcess().Id => Environment.ProcessId analyzer and fixer

cc: @jkotas, @ericstj, @jeffhandley

[Dotnet-GitSync-Bot]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[wfurt]

LGTM

[jkotas]

This does not need to have OS specific build anymore.

<TargetFrameworks>$(NetCoreAppCurrent)<TargetFrameworks>

[stephentoub]

Yeah, I tried that, but I got errors that I wasn't sure what to do with, and so decided to leave that for a follow-up, e.g.

C:\Users\stoub\.nuget\packages\microsoft.dotnet.build.tasks.targetframework.sdk\5.0.0-beta.20316.1\build\Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk.targets(74,5): error : Not able to find a compatible supported target framework for net5.0 in Project System.Diagnostics.Process.csproj. The Supported Configurations are net5.0-Windows_NT, net5.0-FreeBSD, net5.0-Linux, net5.0-OSX, net5.0-iOS, net5.0-tvOS [D:\repos\runtime\src\libraries\System.Diagnostics.TraceSource\src\System.Diagnostics.TraceSource.csproj]

[ericstj]

Can you remove the reference to System.Diagnostics.Process? Is its still used anywhere? IIRC there was a case for Process.GetCurrentProcess().ProcessName which was unused in some cases. Perhaps we can use a different pattern to obtain the name of the process, like AppDomain.CurrentDomain.FriendlyName 🤮 or add Environment.ProcessName.

[stephentoub]

@ericstj, I can remove the Process reference, but then the error just becomes:

C:\Users\stoub\.nuget\packages\microsoft.dotnet.build.tasks.targetframework.sdk\5.0.0-beta.20316.1\build\Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk.targets(74,5): error : Not able to find a compatible supported target framework for net5.0 in Project System.IO.FileSystem.csproj. The Supported Configurations are net5.0-Windows_NT, net5.0-Unix [D:\repos\runtime\src\libraries\System.Diagnostics.TraceSource\src\System.Diagnostics.TraceSource.csproj]

for FileSystem.

[ericstj]

It looks to me like you're talking about a different project? This comment is on TextWriterTraceListener and the error mentions System.Diagnostics.TraceSource.csproj and I don't see any project references in TextWriterTraceListener.

In the case of TraceSource you might be able to get away with changing from a ProjectReference to a simple Reference. ProjectReferences are only needed when you need to use a type that lives in System.Private.CoreLib at runtime (to avoid the duplicate type error from compiler).

[stephentoub]

It looks to me like you're talking about a different project? This comment is on TextWriterTraceListener and the error mentions System.Diagnostics.TraceSource.csproj and I don't see any project references in TextWriterTraceListener.

I was talking about TraceSource (the below "Same here" comment was applying to TraceSource as well, so I've just been using that as a guinea pig). For TextWriterTraceListener, it does still use Process, to get the current process name.

In the case of TraceSource you might be able to get away with changing from a ProjectReference to a simple Reference.

The purpose of this PR is to add Environment.ProcessId. I'm not going to hold it up right now fiddling with the csproj. Please open an issue about consolidating the target frameworks if there isn't already one tracking that. Thanks!

[ericstj]

Ok, that's fine by me. /cc @Anipik

[stephentoub]

Great, thanks.

[Anipik]

I can follow up with this and see where we can leverage this api and remove cross targeting

[GSPP]

Could s_processId be a static readonly field on a new static class? That way there would not need to be an initialization check at all. Obtaining the process id would amount to reading a static field. Maybe the JIT could even inline the value.

[stephentoub]

Could s_processId be a static readonly field on a new static class?

It could. I'm not sure it's worth it. @jkotas, do you have an opinion?

[jkotas]

I do not have an opinion. This optimization is not free - it adds static footprint. Is it a good tradeoff in this case? Unclear.

[stephentoub]

This optimization is not free - it adds static footprint. Is it a good tradeoff in this case? Unclear.

Right, thanks. That's why I went with the simple solution of just doing lazy initialization. If we get enough evidence to suggest this is so critical path that it's worth it, we can change it in the future. As of now, though, we're making something that was super slow way faster already.