Skip to content

Reduce memory allocations for Process.GetProcessesByName #41137

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Reduce memory allocations for Process.GetProcessesByName #41137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 2 additions & 16 deletions src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,22 +28,8 @@ public static Process[] GetProcessesByName(string? processName, string machineNa
processName = string.Empty;
}

Process[] procs = GetProcesses(machineName);
var list = new List<Process>();

for (int i = 0; i < procs.Length; i++)
{
if (string.Equals(processName, procs[i].ProcessName, StringComparison.OrdinalIgnoreCase))
{
list.Add(procs[i]);
}
else
{
procs[i].Dispose();
}
}

return list.ToArray();
return GetProcesses(machineName, process =>
string.Equals(processName, process.ProcessName, StringComparison.OrdinalIgnoreCase));
}

[CLSCompliant(false)]
Expand Down
24 changes: 23 additions & 1 deletion src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1054,14 +1054,36 @@ public static Process[] GetProcesses()
/// </para>
/// </devdoc>
public static Process[] GetProcesses(string machineName)
{
return GetProcesses(machineName, null);
}

/// <devdoc>
/// <para>
/// Creates a new <see cref='System.Diagnostics.Process'/>
/// component for each
/// process resource on the specified computer.
/// </para>
/// </devdoc>
internal static Process[] GetProcesses(string machineName, Predicate<ProcessInfo>? processFilter)
{
bool isRemoteMachine = ProcessManager.IsRemoteMachine(machineName);
ProcessInfo[] processInfos = ProcessManager.GetProcessInfos(machineName);
Process[] processes = new Process[processInfos.Length];
int processesLength = 0;
for (int i = 0; i < processInfos.Length; i++)
{
ProcessInfo processInfo = processInfos[i];
processes[i] = new Process(machineName, isRemoteMachine, processInfo.ProcessId, processInfo);
if (processFilter == null || processFilter(processInfo))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can Linq's Where and ToArray be used here or would this allocate more?

Copy link
Contributor Author

@Serg046 Serg046 Aug 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean anything like this (and then Where(...) in another file)? That was my initial idea but then I realized that it allocates more and affects the paths when we don't filter anything.

public static Process[] GetProcesses(string machineName)
{
    return EnumerateProcesses(machineName).ToArray();
}

/// <devdoc>
///    <para>
///       Enumerates all <see cref='System.Diagnostics.Process'/>
///       components on the specified computer.
///    </para>
/// </devdoc>
internal static IEnumerable<Process> EnumerateProcesses(string machineName)
{
    bool isRemoteMachine = ProcessManager.IsRemoteMachine(machineName);
    ProcessInfo[] processInfos = ProcessManager.GetProcessInfos(machineName);
    for (int i = 0; i < processInfos.Length; i++)
    {
        ProcessInfo processInfo = processInfos[i];
        yield return new Process(machineName, isRemoteMachine, processInfo.ProcessId, processInfo);
    }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean something like this:

internal static Process[] GetProcesses(string machineName, Func<ProcessInfo, bool>? processFilter)
{
    bool isRemoteMachine = ProcessManager.IsRemoteMachine(machineName);
    ProcessInfo[] processInfos = ProcessManager.GetProcessInfos(machineName);

    // Usa a "dummy true" filter if no filter is given
    processFilter ??= _ => true;

    return processInfos
        .Where(processFilter)
        .Select(pi => new Process(/* ... */))
        .ToArray();
}

So if a filter is given, this saves the allocation for Process[] processes = new Process[processInfos.Length];.

Note: I changed the argument from Predicate to Fun<,>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I will aply the note and see if other guys will approve too.
I was confused by the following line from the ticket and just did the same for some reason:

runtime/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs

Line 360 in f4d3913

if (processIdFilter == null || processIdFilter(processInfoProcessId))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be useful to benchmark both time and total allocations for baseline and different variants of the fix.

Linq maybe saves some allocations, but it comes with other overheads and much larger static footprint so it is not a clear winner. We tend to avoid Linq in the runtime libraries implementation.

Copy link
Member

@jkotas jkotas Aug 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am more worried about ProcessManager.GetProcessInfos above being so expensive (both in time and allocations) that the changes proposed here won't make any difference.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can use this code

        int processesLength = 0;

        for (int i = 0; i < processInfos.Length; i++)
        {
            ProcessInfo processInfo = processInfos[i];
            if (processFilter == null || processFilter(processInfo))
            {
                if (i != processesLength)
                {
                    processInfos[processesLength] = processInfo;
                    processesLength++;
                }
            }
        }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thinks it's better to go a step back and eleminate the allocations from ProcessManager.GetProcessInfos. There can be a internal version that doesn't need to return an array -- so the final array just have to be allocated before returning the results. All intermediate stepds shouldn't need arrays.

I'll have to dig more into the code, to give a more concrete suggestion if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thinks it's better to go a step back and eleminate the allocations from ProcessManager.GetProcessInfos.

I thought about this option and found 2 ways that time:

  1. Passing a filter through all the methods (but there are quite a few calls).
    I didn't think a lot about this option because I thought it would complicate/litter the code without enough reason.

  2. IEnumerable (or kind of that) as a return type for all non-public methods in the chain.
    This one is better imo. But the thing is that ProcessInfo[] ProcessManager.GetProcessInfos(string machineName) is public and even the next ProcessInfo[] NtProcessManager.GetProcessInfos(string machineName, bool isRemoteMachine) is public too.

All intermediate steps shouldn't need arrays.

Unfortunately, that is not true.

I'll have to dig more into the code, to give a more concrete suggestion if you want.

Feel free to find a better way. If you find one, I can change this PR or let you create a new one.
My plan is still to measure performance of 3 approaches discussed above.

Maybe we can use this code

Interesting idea but not sure if it could help. As a result we need an array of Processes, not ProcessInfos. So the actual issue is that we don't know the right length in advance. The only way to get this value is to interate one more time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thinks it's better to go a step back and eleminate the allocations from ProcessManager.GetProcessInfos.

We could use the same approach as File/Directory enumeration API does with public ref partial struct FileSystemEntry.

{
processes[processesLength++] = new Process(machineName, isRemoteMachine, processInfo.ProcessId, processInfo);
}
}
if (processesLength < processInfos.Length)
{
Process[] filteredProcesses = new Process[processesLength];
Array.Copy(processes, filteredProcesses, processesLength);
return filteredProcesses;
}
return processes;
}
Expand Down