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

Consider using process snapshotting APIs for System.Diagnostics.Process #99906

Open
kevingosse opened this issue Mar 18, 2024 · 13 comments
Open
Labels
area-System.Diagnostics.Process help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged tenet-performance Performance related issue
Milestone

Comments

@kevingosse
Copy link
Contributor

kevingosse commented Mar 18, 2024

We've been using Process.GetCurrentProcess() to retrieve information about the current process, such as the total number of threads. We've seen evidence that, on Windows, calling process.Threads could take tens of milliseconds for some customers, and even on my machine it consistently takes more than 3 milliseconds.
Under the hood, it relies on NtQuerySystemInformation which actually captures information for all the processes on the machine, and then cherry-picks the information for the current process. Windows 8.1 has introduced process snapshotting APIs which can be used to retrieve this kind of information in a much cheaper way.

I've done a quick prototype where I compare the speed of NtQuerySystemInformation vs PssCaptureSnapshot/PssWalkSnapshot and the results are very encouraging:

Method Mean Error StdDev
PssWalkSnapshot 102.19 us 3.360 us 9.747 us
NtQuerySystemInformation 3,100.48 us 60.486 us 105.937 us

My prototype only retrieves the id of each thread of the process, but it looks like all the information required to populate the .NET ProcessThread class is in there.

Would it be worth pursuing further?

@kevingosse kevingosse added the tenet-performance Performance related issue label Mar 18, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Mar 18, 2024
@stephentoub
Copy link
Member

Would it be worth pursuing further?

Sure. We might not be able to drop the existing implementation in order to keep older versions of Windows working, but if can produce the same results on newer versions more efficiently, and if there aren't other caveats (e.g. you must be an administrator), sounds like a good thing to experiment with.

@Symbai
Copy link

Symbai commented Mar 19, 2024

keep older versions of Windows working

Aren't older versions of Windows (below Windows 8.1) out of support?

@stephentoub
Copy link
Member

Even when OSes are out of support, we try to avoid breaking existing features.

@MichalPetryka
Copy link
Contributor

MichalPetryka commented Mar 19, 2024

Even when OSes are out of support, we try to avoid breaking existing features.

Worth noting that .NET 8 still supports Windows Server 2012 which corresponds to Windows 8 (not 8.1) API wise and so lacks APIs like PssCaptureSnapshot.

@jeffhandley
Copy link
Member

@kevingosse I'm moving this into the Future milestone, but if you're planning to submit a pull request for this, we will commit to reviewing it. I'll mark the issue as help wanted [up-for-grabs] Good issue for external contributors .

@jeffhandley jeffhandley added this to the Future milestone Mar 22, 2024
@jeffhandley jeffhandley 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 Mar 22, 2024
@kevingosse
Copy link
Contributor Author

I started implementing it, but PssCaptureSnapshot throws an access violation when I try to collect handles. I initially thought that maybe I got my p/invoke wrong, but I still have the issue with this simple C++ code:

https://gist.github.com/kevingosse/5dc76124ca986a41f1e8719c77b7987a

@stephentoub This looks like a bug in the PssCaptureSnapshot API. Do you have a contact at Microsoft that I could report this to? Now that Microsoft Connect is gone, I don't know what channel we're supposed to use.

@Symbai
Copy link

Symbai commented Mar 24, 2024

I dont get an access violation, works fine on my system (Windows 11 64bit)

Code
[Flags]
private enum PSS_CAPTURE_FLAGS : uint
{
  PSS_CAPTURE_HANDLES = 0x00000004,
}

private enum PSS_QUERY_INFORMATION_CLASS
{
  PSS_QUERY_HANDLE_INFORMATION = 4,
}

[DllImport("kernel32.dll")]
private static extern int PssCaptureSnapshot(IntPtr processHandle, PSS_CAPTURE_FLAGS captureFlags, int threadContextFlags, out IntPtr snapshotHandle);

[DllImport("kernel32.dll")]
private static extern unsafe int PssQuerySnapshot(IntPtr snapshotHandle, PSS_QUERY_INFORMATION_CLASS informationClass, void* buffer, int bufferLength);

[StructLayout(LayoutKind.Sequential)]
public struct PSS_HANDLE_INFORMATION
{
  public uint HandlesCaptured;
}

static unsafe async Task Main(string[] args)
{
  var result = PssCaptureSnapshot(Process.GetCurrentProcess().Handle, PSS_CAPTURE_FLAGS.PSS_CAPTURE_HANDLES, 0, out var snapshotHandle);
  
  if (result != 0)
  {
  	throw new Win32Exception(result);
  }
 
  PSS_HANDLE_INFORMATION handleInformation = default;

  result = PssQuerySnapshot(snapshotHandle, PSS_QUERY_INFORMATION_CLASS.PSS_QUERY_HANDLE_INFORMATION, &handleInformation, Marshal.SizeOf<PSS_HANDLE_INFORMATION>());
  
  if (result != 0)
  {
  	throw new Win32Exception(result);
  }
  var handleCount = handleInformation.HandlesCaptured;

  Console.WriteLine(handleCount);
}

@karakasa
Copy link
Contributor

karakasa commented Sep 20, 2024

The problem is PssCaptureSnapshot doesn't include information for ProcessThread.ThreadState & ProcessThread.WaitReason. Although rarely used, these two properties requires either:

  • Introduce a new Windows-only API that fills Process.Threads without the two properties;
  • When the two properties are accessed, re-enumerate threads using the original way.
    • The workaround is very slow and error-prone, as NtQuerySystemInformation is the only documented way to retrieve the values.
  • Use undocumented NtQueryInformationThread+ThreadSystemThreadInformation (THREADINFOCLASS = 0x28)

Besides, this may be a behavior breaking change since the two values are lazy-evaluated, rather than when Process.Threads is initialized as of now.

@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Sep 23, 2024
@jkotas
Copy link
Member

jkotas commented Sep 24, 2024

Use undocumented NtQueryInformationThread+ThreadSystemThreadInformation

We may also explore what it would take to have this API documented. It would need to be handled by System.Diagnostic.Process area maintainers.

@karakasa
Copy link
Contributor

We may also explore what it would take to have this API documented.

I suppose that needs to be evaluated by the Windows team?

@jkotas
Copy link
Member

jkotas commented Sep 25, 2024

I suppose that needs to be evaluated by the Windows team?

Right.

@Symbai
Copy link

Symbai commented Sep 25, 2024

  • Use undocumented NtQueryInformationThread

At least this one is documented and the documentation strongly advises to not use this API as it might change or become unavailable in future versions of Windows.

@karakasa
Copy link
Contributor

karakasa commented Sep 26, 2024

At least this one is documented and the documentation strongly advises to not use this API as it might change or become unavailable in future versions of Windows.

I'm not quite worried about that. The current approach utilizes NtQuerySystemInformation which is also advised not to use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Diagnostics.Process help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants