perf(process): build snapshot map once on Windows to fix O(N²) process listing#2062
perf(process): build snapshot map once on Windows to fix O(N²) process listing#2062HarshalPatel1972 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Optimizes Windows process enumeration in gopsutil/process by avoiding repeated Toolhelp snapshot scans when collecting per-process metadata (name, PPID, thread count), addressing the O(N²) behavior reported in #818.
Changes:
- Adds snapshot pre-scan (
buildSnapProcessMap) to build a PID→ProcessEntry32map in one pass. - Populates per-
Processcaches (name,parent/PPID,numThreads) duringProcessesWithContext. - Adds simple caching fast-paths in
NameWithContextandNumThreadsWithContext.
Comments suppressed due to low confidence (1)
process/process_windows.go:339
NameWithContextnow checksp.namefirst, but whenp.nameis empty it computes the name fromExeWithContextand returns it without storing it back intop.name. That means processes created viaNewProcess*still pay the full cost on everyName*call, and the new cache is only effective whenProcessesWithContextpre-populates the field. Consider assigning the computed base name top.namebefore returning (and optionally caching the special-case PID 0/4 names too).
if p.name != "" {
return p.name, nil
}
if p.Pid == 0 {
return "System Idle Process", nil
}
if p.Pid == 4 {
return "System", nil
}
exe, err := p.ExeWithContext(ctx)
if err != nil {
return "", fmt.Errorf("could not get Name: %w", err)
}
return filepath.Base(exe), nil
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for { | ||
| snapMap[pe32.ProcessID] = pe32 | ||
| if err = windows.Process32Next(snap, &pe32); err != nil { | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
buildSnapProcessMap breaks out of the loop on any Process32Next error and returns the map, which can silently return an incomplete snapshot if Process32Next fails for reasons other than "no more files". Please explicitly treat windows.ERROR_NO_MORE_FILES (or the appropriate sentinel) as the normal termination condition, and return the error for any other failure.
|
|
||
| snapMap, err := buildSnapProcessMap() | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
On error from buildSnapProcessMap, ProcessesWithContext returns a nil slice (return nil, err), while other error paths in this function (and other OS implementations) return the already-initialized out slice. For API consistency and to avoid surprising nil slices for callers that don’t nil-check, consider returning out here as well (and optionally wrapping the error with context like the earlier PidsWithContext failure).
| return nil, err | |
| return out, fmt.Errorf("could not build process snapshot %w", err) |
…pping, NameWithContext cache write-back
|
@copilot AI I have addressed all 3 pieces of review feedback:
Also ran \golangci-lint\ to resolve the subsequent warnings. |
shirou
left a comment
There was a problem hiding this comment.
Good fix for a real performance problem. A few items to address:
- In
NameWithContext, thep.name != ""early return should be placed after thep.Pid == 0check, not before — otherwise PID 0 may return the snapshot's ExeFile name instead of the hardcoded "System Idle Process". - The
numThreads != 0cache guard is incorrect — 0 is a valid thread count. Either remove the early return (the snapshot-populated value will flow through the existing code), or use a separate bool to track whether it's been cached. - Minor: Consider adding a brief comment in
ProcessesWithContextnoting that the snapshot and PID enumeration are separate calls
and may not be perfectly consistent.
Fixes #818
Problem
On Windows,
ProcessesWithContextcallsgetFromSnapProcessonce per process to retrieve name, PPID, and thread count.getFromSnapProcessinternally iterates through the entireCreateToolhelp32Snapshotresult each time — making the overall complexity O(N²). On a machine with ~320 processes this results in ~13 seconds and 30–40% CPU usage, vs ~100ms on Linux.Fix
Added
buildSnapProcessMap()which callsCreateToolhelp32Snapshotexactly once and builds amap[uint32]windows.ProcessEntry32keyed by PID.ProcessesWithContextnow calls this once at startup and does O(1) per-process lookups for name, PPID, and thread count.getFromSnapProcessis left intact for single-process query paths — no API changes.Benchmark
Tested on Windows 11 with ~320 running processes.