Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.ProcessName should use QueryFullProcessImageNameW or GetProcessImageFileNameW #55685

Closed
jaykrell opened this issue Jul 14, 2021 · 5 comments · Fixed by #59672
Closed

.ProcessName should use QueryFullProcessImageNameW or GetProcessImageFileNameW #55685

jaykrell opened this issue Jul 14, 2021 · 5 comments · Fixed by #59672
Assignees
Labels
area-System.Diagnostics.Process help wanted [up-for-grabs] Good issue for external contributors tenet-performance Performance related issue
Milestone

Comments

@jaykrell
Copy link
Contributor

jaykrell commented Jul 14, 2021

On Windows, .ProcessName should use QueryFullProcessImageNameW or GetProcessImageFileNameW.

Presently it used NtQuerySystemInformation(SystemProcessInformation).

The problem with NtQuerySystemInformation(SystemProcessInformation) is:

  • It is tremendously slower, returning all information about all processes,
    instead of just the name, for one process. This is pretty obviously terrible, unless there are very few processes, and perhaps if you only have the process id and not a handle already (two syscalls, open and query). (Too bad Windows doesn't allow query by id to cut the cost.) Enum also has an advantage, sometimes, like, if you actually want more information than just the name. But generally it seems silly.

  • It does not work with terminated processes, which NtQuerySystemInformation(SystemProcessInformation) omit.
    For example, you cannot have a generic:
    WriteLine("The process {0} ended", process.ProcessName);

unless you call .ProcessName prior to termination to populate the CLR's cache. Such order/cache-sensitivity is also fragile but maybe ok. And that is not even possible: .NET does not expose creating processes suspended, so you will always race with a fast process that terminates quickly. So .ProcessName is all but unusable. We are removing our uses pretty thoroughly as a result. Though it is ok on current process, which will never be terminated.

We have actual code like this, causing us much grief, because..complicated reasons (we actually run mostly on an alternate implementation of NT, which was enumerating terminated processes, which mostly masked these problems, but not entirely; the problem is worse on native NT).
We are going to reduce our uses of process.ProcessName as a result.

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Diagnostics.Process untriaged New issue has not been triaged by the area owner labels Jul 14, 2021
@ghost
Copy link

ghost commented Jul 14, 2021

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

Issue Details

On Windows, .ProcessName should use QueryFullProcessImageNameW or GetProcessImageFileNameW.

Presently it used NtQuerySystemInformation(SystemProcessInformation).

The problem with NtQuerySystemInformation(SystemProcessInformation) is:

  • It is tremendously slower, returning all information about all processes,
    instead of just the name, for one process. This is pretty obviously terrible, unless there are very processes, and perhaps if you only have the process id and not a handle already (two syscalls, open and query). (Too bad Windows doesn't allow query by id to cut the cost.)

  • It does not work with terminated processes, which NtQuerySystemInformation(SystemProcessInformation) omit.
    For example, you cannot have a generic:
    WriteLine("The process {0} ended", process.ProcessName);

unless you call .ProcessName prior to termination to populate the CLR's cache.

We have actual code like this, causing us much grief, because..complicated reasons.

Author: jaykrell
Assignees: -
Labels:

area-System.Diagnostics.Process, untriaged

Milestone: -

@danmoseley
Copy link
Member

Seems reasonable, if it works in all the same places. Do you want to offer a PR, @jaykrell ?

@danmoseley danmoseley added help wanted [up-for-grabs] Good issue for external contributors and removed untriaged New issue has not been triaged by the area owner labels Jul 29, 2021
@adamsitnik adamsitnik added this to the Future milestone Jul 30, 2021
@ryank425
Copy link

ryank425 commented Aug 4, 2021

I wanted to implement the feature but I do not have an industry experience with calling C++ Win32 codes from C#, so I will just leave where things are for people who will implement this in the future if this is indeed the way to go to fetch ProcessName.

ProcessName exists in https://github.com/dotnet/runtime/blob/a5842801e0c3fef66532da584528905c88b88f5b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs


public string ProcessName
{
    get
    {
        EnsureState(State.HaveProcessInfo);
        return _processInfo!.ProcessName;
    }
}

ProcessName ensures that we have _processInfo not be to null. If this is null, EnsureState will make following calls,

_processInfo = ProcessManager.GetProcessInfo(_processId, _machineName);

and for Windows, that goes to this file

public static ProcessInfo? GetProcessInfo(int processId, string machineName)

This is where NtProcessManager will call the problematic call

NtQuerySystemInformation from following code

internal static ProcessInfo[] GetProcessInfos(int? processIdFilter = null)

In short, OP doesn't want to call NtQuerySystemInformation in the first place, just to get ProcessName.
If you can utilize https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-queryfullprocessimagenamew or
https://docs.microsoft.com/en-us/windows/win32/api/psapi/nf-psapi-getprocessimagefilenamew , you can skip EnsureState and return process name.

I am curious about how the benchmark would look by skipping NtQuerySystemInformation.

@SteveDunn
Copy link
Contributor

Hi - I'm happy to work on this one.

@SteveDunn
Copy link
Contributor

SteveDunn commented Oct 8, 2021

After applying this change, getting the process name and working set is much quicker (relatively speaking) and allocates less memory. Benchmarks below.

It appears that the associated PR addresses one of @jaykrell 's issues in that ProcessName now doesn't use the slower NtQuerySystemInformation.

However, It doesn't satisfy the requirement that ProcessName will still return the name even if the process is terminated. On that subject, it does seem surprising to get different results for ProcessName depending on whether you've called it while the process was alive or terminated. But I guess that's a whole different issue.

Benchmarks

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000
Intel Core i7-7700K CPU 4.20GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET SDK=7.0.100-alpha.1.21467.19
  [Host]     : .NET 5.0.10 (5.0.1021.41214), X64 RyuJIT
  Job-YJXSLY : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT
  Job-NVCLTE : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT

IterationCount=15  LaunchCount=2  WarmupCount=10  
Method Job Toolchain Mean Error StdDev Ratio RatioSD Gen 0 Gen 1 Gen 2 Allocated
CurrentProcess_ProcessName_Only_Just_Once Job-YJXSLY main 2,615.28 μs 52.515 μs 78.602 μs 1.00 0.00 - - - 3,186 B
CurrentProcess_ProcessName_Only_Just_Once Job-NVCLTE PR 43.52 μs 0.642 μs 0.960 μs 0.02 0.00 0.1221 - - 672 B
CurrentProcess_ProcessName_AfterWorkingSet64_Just_Once Job-YJXSLY main 2,584.42 μs 37.714 μs 55.280 μs 1.00 0.00 - - - 3,282 B
CurrentProcess_ProcessName_AfterWorkingSet64_Just_Once Job-NVCLTE PR 2,411.35 μs 29.618 μs 43.414 μs 0.93 0.03 - - - 3,290 B
CurrentProcess_ProcessName_Only_Multiple Job-YJXSLY main 3,168.35 μs 107.675 μs 154.424 μs 1.00 0.00 1679.6875 - - 7,035,146 B
CurrentProcess_ProcessName_Only_Multiple Job-NVCLTE PR 432.88 μs 11.265 μs 16.860 μs 0.14 0.01 1679.6875 - - 7,032,632 B
CurrentProcess_ProcessName_AfterWorkingSet64 Job-YJXSLY main 3,019.24 μs 51.647 μs 77.303 μs 1.00 0.00 1683.5938 - - 7,051,226 B
CurrentProcess_ProcessName_AfterWorkingSet64 Job-NVCLTE PR 2,880.19 μs 38.718 μs 55.527 μs 0.96 0.03 1683.5938 - - 7,051,234 B
CurrentProcess_ProcessName_BeforeWorkingSet64_Multiple Job-YJXSLY main 2,962.78 μs 28.560 μs 42.747 μs 1.00 0.00 1683.5938 - - 7,049,226 B
CurrentProcess_ProcessName_BeforeWorkingSet64_Multiple Job-NVCLTE PR 3,011.14 μs 40.898 μs 58.655 μs 1.02 0.03 1683.5938 - - 7,049,618 B
AllProcesses_ProcessName_Only_Multiple Job-YJXSLY main 11,225.78 μs 196.514 μs 288.048 μs 1.00 0.00 33343.7500 234.3750 109.3750 142,780,460 B
AllProcesses_ProcessName_Only_Multiple Job-NVCLTE PR 10,814.94 μs 154.934 μs 227.100 μs 0.96 0.03 31453.1250 328.1250 156.2500 134,779,175 B
AllProcesses_ProcessName_AfterWorkingSet64_Multiple Job-YJXSLY main 12,916.11 μs 137.887 μs 197.753 μs 1.00 0.00 39828.1250 234.3750 109.3750 169,803,168 B
AllProcesses_ProcessName_AfterWorkingSet64_Multiple Job-NVCLTE PR 12,188.59 μs 230.248 μs 344.624 μs 0.95 0.03 37140.6250 312.5000 156.2500 158,539,815 B
AllProcesses_ProcessName_BeforeWorkingSet64_Multiple Job-YJXSLY main 12,849.65 μs 179.079 μs 268.037 μs 1.00 0.00 39828.1250 312.5000 156.2500 169,818,559 B
AllProcesses_ProcessName_BeforeWorkingSet64_Multiple Job-NVCLTE PR 11,890.46 μs 99.702 μs 149.229 μs 0.93 0.02 37156.2500 312.5000 156.2500 158,552,346 B

Legend:

  • CurrentProcess_ProcessName_Only_Just_Once - gets only the name of the current process, and only once
  • CurrentProcess_ProcessName_AfterWorkingSet64_Just_Once - gets the name and WS of the current process, and only once
  • CurrentProcess_ProcessName_Only_Multiple - gets the name of the current process, multiple times
  • CurrentProcess_ProcessName_AfterWorkingSet64 - gets the WS and name of the current process, multiple times
  • CurrentProcess_ProcessName_BeforeWorkingSet64_Multiple - get the WS and name of the current process, multiple times
  • AllProcesses_ProcessName_Only_Multiple - gets the name of all running processes, multiple times
  • AllProcesses_ProcessName_AfterWorkingSet64_Multiple - gets the WS and name of all runnining process, multiple times
  • AllProcesses_ProcessName_BeforeWorkingSet64_Multiple - gets the name and WS of all running process, mmultiple times

@adamsitnik adamsitnik modified the milestones: Future, 7.0.0 Dec 7, 2021
@adamsitnik adamsitnik added the tenet-performance Performance related issue label Dec 7, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jan 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Diagnostics.Process help wanted [up-for-grabs] Good issue for external contributors tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants