fix: performance — WMI caching, allocation-free stats, tray re-entrancy guard#406
Conversation
…cy guard - PERF-M1: SystemInfoService caches static WMI data (OS, CPU, disks) - PERF-M2: NetworkSharedState.RecomputeStats uses manual loops (no LINQ allocs) - PERF-M4: TrayIconService skips overlapping WMI calls via Interlocked guard
📝 WalkthroughWalkthroughThis PR introduces three independent performance and concurrency improvements for system monitoring. SystemInfoService now caches static WMI data on first call and refreshes only dynamic values per capture cycle. TrayIconService adds an Interlocked re-entrancy guard to prevent overlapping tooltip updates during high-frequency timer ticks. NetworkSharedState eliminates LINQ allocations in its stats computation by switching to explicit loops. All changes are documented in the v0.48.26 release notes. ChangesPerformance & Caching Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
SysManager/SysManager/ViewModels/NetworkSharedState.cs (1)
374-391: ⚡ Quick winRemove remaining hot-path allocation in jitter computation.
Line 375 still allocates a
List<double>on every recompute. You can keep the same jitter result and eliminate that allocation by computing mean/variance online over the most recent successful samples.♻️ Proposed refactor (allocation-free jitter window)
- // Collect recent samples for jitter (walk backwards, no allocation beyond the list). - var recent = new List<double>(JitterSampleWindow); - for (int i = buffer.Count - 1; i >= 0 && recent.Count < JitterSampleWindow; i--) - { - if (buffer[i].Value.HasValue) recent.Add(buffer[i].Value!.Value - offset); - } + // Collect up to JitterSampleWindow recent successful samples with online variance (Welford). + int jitterCount = 0; + double jitterMean = 0; + double m2 = 0; + for (int i = buffer.Count - 1; i >= 0 && jitterCount < JitterSampleWindow; i--) + { + var v = buffer[i].Value; + if (!v.HasValue) continue; + var sample = v.Value - offset; + jitterCount++; + var delta = sample - jitterMean; + jitterMean += delta / jitterCount; + var delta2 = sample - jitterMean; + m2 += delta * delta2; + } target.AverageMs = successful > 0 ? Math.Round(sum / successful, 1) : null; target.LossPercent = Math.Round(100.0 * (total - successful) / total, 1); - if (recent.Count >= 2) + if (jitterCount >= 2) { - double mean = 0; - for (int i = 0; i < recent.Count; i++) mean += recent[i]; - mean /= recent.Count; - double variance = 0; - for (int i = 0; i < recent.Count; i++) variance += (recent[i] - mean) * (recent[i] - mean); - variance /= recent.Count; + double variance = m2 / jitterCount; target.JitterMs = Math.Round(Math.Sqrt(variance), 1); } else { target.JitterMs = null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SysManager/SysManager/ViewModels/NetworkSharedState.cs` around lines 374 - 391, The jitter calculation currently allocates a List<double> named recent each recompute (constructed with JitterSampleWindow) and fills it from buffer, causing a hot-path allocation; replace this by computing mean and variance online over the most recent successful samples without creating recent—iterate backwards over buffer (same loop conditions as before) and update running count, runningSum, runningMean (for example via Welford’s algorithm) and runningM2, then derive variance = runningM2 / count; assign the same target fields (target.AverageMs, target.LossPercent already set) and compute jitter from the online mean/variance so no temporary List<double> is allocated.SysManager/SysManager/Services/SystemInfoService.cs (1)
32-45: ⚡ Quick winMove the dynamic WMI calls out of
_cacheLock.
QueryUptime()andQueryCpuLoad()still run inside the critical section, so concurrent captures serialize on slow WMI even after the caches are warm. Only the cache initialization needs the lock.Proposed refactor
- lock (_cacheLock) - { - // Static OS info (caption, version, build, arch) — cache it. - // Uptime is dynamic, so we always refresh it. - _cachedOs ??= QueryOsStatic(); - os = _cachedOs with { Uptime = QueryUptime() }; - - // CPU name/cores/threads/clock are static; only LoadPercentage is dynamic. - _cachedCpuStatic ??= QueryCpuStatic(); - cpu = _cachedCpuStatic with { LoadPercent = QueryCpuLoad() }; - - // Disk info is static (models don't change at runtime). - _cachedDisks ??= QueryDisks(); - disks = _cachedDisks; - } + OsInfo cachedOs; + CpuInfo cachedCpuStatic; + List<DiskInfo> cachedDisks; + lock (_cacheLock) + { + _cachedOs ??= QueryOsStatic(); + _cachedCpuStatic ??= QueryCpuStatic(); + _cachedDisks ??= QueryDisks(); + + cachedOs = _cachedOs; + cachedCpuStatic = _cachedCpuStatic; + cachedDisks = _cachedDisks; + } + + os = cachedOs with { Uptime = QueryUptime() }; + cpu = cachedCpuStatic with { LoadPercent = QueryCpuLoad() }; + disks = cachedDisks;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SysManager/SysManager/Services/SystemInfoService.cs` around lines 32 - 45, Inside the _cacheLock critical section only initialize the static caches (_cachedOs via QueryOsStatic, _cachedCpuStatic via QueryCpuStatic, and _cachedDisks via QueryDisks) and assign disks = _cachedDisks; then release the lock and perform the dynamic WMI calls (QueryUptime and QueryCpuLoad) outside the lock and build the final objects (os = _cachedOs with { Uptime = QueryUptime() } and cpu = _cachedCpuStatic with { LoadPercent = QueryCpuLoad() }); ensure _cacheLock, _cachedOs, _cachedCpuStatic, QueryOsStatic, QueryCpuStatic, QueryUptime, QueryCpuLoad and QueryDisks are used as the unique anchors so only cache initialization is synchronized and slow WMI queries run concurrently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SysManager/SysManager/Services/SystemInfoService.cs`:
- Around line 115-126: QueryCpuLoad currently uses
ManagementObjectSearcher("SELECT LoadPercentage FROM Win32_Processor") and
returns the first Win32_Processor instance, which misses
multi-socket/system-wide load; replace the WMI query to read from
Win32_PerfFormattedData_Counters_ProcessorInformation (select
PercentProcessorTime where Name = "_Total" or use the _Total instance) and parse
that value to a double, keeping the same method signature, disposing the
searcher/collection as before (referencing QueryCpuLoad,
ManagementObjectSearcher, and ManagementObjectCollection) and return 0 on
missing/null values.
- Around line 18-20: The cached mutable list _cachedDisks is returned directly
which allows callers to mutate the cache; update the code that builds
SystemSnapshot (the place using _cachedDisks to set SystemSnapshot.Disks) to
return a defensive copy (e.g., new List<DiskInfo>(_cachedDisks)) or change the
SystemSnapshot.Disks type to IReadOnlyList<DiskInfo> and construct an immutable
copy; ensure this mirrors how OsInfo and CpuInfo are cloned via record with so
the cache cannot be mutated by consumers.
---
Nitpick comments:
In `@SysManager/SysManager/Services/SystemInfoService.cs`:
- Around line 32-45: Inside the _cacheLock critical section only initialize the
static caches (_cachedOs via QueryOsStatic, _cachedCpuStatic via QueryCpuStatic,
and _cachedDisks via QueryDisks) and assign disks = _cachedDisks; then release
the lock and perform the dynamic WMI calls (QueryUptime and QueryCpuLoad)
outside the lock and build the final objects (os = _cachedOs with { Uptime =
QueryUptime() } and cpu = _cachedCpuStatic with { LoadPercent = QueryCpuLoad()
}); ensure _cacheLock, _cachedOs, _cachedCpuStatic, QueryOsStatic,
QueryCpuStatic, QueryUptime, QueryCpuLoad and QueryDisks are used as the unique
anchors so only cache initialization is synchronized and slow WMI queries run
concurrently.
In `@SysManager/SysManager/ViewModels/NetworkSharedState.cs`:
- Around line 374-391: The jitter calculation currently allocates a List<double>
named recent each recompute (constructed with JitterSampleWindow) and fills it
from buffer, causing a hot-path allocation; replace this by computing mean and
variance online over the most recent successful samples without creating
recent—iterate backwards over buffer (same loop conditions as before) and update
running count, runningSum, runningMean (for example via Welford’s algorithm) and
runningM2, then derive variance = runningM2 / count; assign the same target
fields (target.AverageMs, target.LossPercent already set) and compute jitter
from the online mean/variance so no temporary List<double> is allocated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ce19885-3553-4bfa-8cea-6dd16b151ecb
📒 Files selected for processing (4)
CHANGELOG.mdSysManager/SysManager/Services/SystemInfoService.csSysManager/SysManager/Services/TrayIconService.csSysManager/SysManager/ViewModels/NetworkSharedState.cs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & unit tests
- GitHub Check: Analyze (csharp)
🔇 Additional comments (3)
SysManager/SysManager/Services/TrayIconService.cs (1)
25-25: LGTM!Also applies to: 141-143, 169-172
CHANGELOG.md (1)
9-21: LGTM!SysManager/SysManager/ViewModels/NetworkSharedState.cs (1)
361-373: LGTM!
| private OsInfo? _cachedOs; | ||
| private CpuInfo? _cachedCpuStatic; | ||
| private List<DiskInfo>? _cachedDisks; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== SystemSnapshot definition =="
fd -i '^SystemSnapshot\.cs$' | while read -r f; do
echo "--- $f ---"
sed -n '1,220p' "$f"
done
echo
echo "== Disk collection usages =="
rg -n -C2 '\bDisks\b|\bSystemSnapshot\s*\(' --glob '!**/bin/**' --glob '!**/obj/**'Repository: laurentiu021/SystemManager
Length of output: 12110
🏁 Script executed:
cat -n SysManager/SysManager/Services/SystemInfoService.csRepository: laurentiu021/SystemManager
Length of output: 11614
Defensive copy required: _cachedDisks is returned without cloning.
Lines 44–45 directly reference the cached list without creating a defensive copy. Unlike OsInfo and CpuInfo (which use record with for cloning), List<DiskInfo> is mutable. If a consumer mutates snap.Disks, the cache is corrupted for subsequent snapshots.
Change line 45 to:
disks = new List<DiskInfo>(_cachedDisks);Alternatively, update SystemSnapshot.Disks to use IReadOnlyList<DiskInfo> to enforce immutability at the type level.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SysManager/SysManager/Services/SystemInfoService.cs` around lines 18 - 20,
The cached mutable list _cachedDisks is returned directly which allows callers
to mutate the cache; update the code that builds SystemSnapshot (the place using
_cachedDisks to set SystemSnapshot.Disks) to return a defensive copy (e.g., new
List<DiskInfo>(_cachedDisks)) or change the SystemSnapshot.Disks type to
IReadOnlyList<DiskInfo> and construct an immutable copy; ensure this mirrors how
OsInfo and CpuInfo are cloned via record with so the cache cannot be mutated by
consumers.
| private static double QueryCpuLoad() | ||
| { | ||
| using var searcher = new ManagementObjectSearcher("SELECT LoadPercentage FROM Win32_Processor"); | ||
| using var cpuCollection = searcher.Get(); | ||
| foreach (ManagementObject mo in cpuCollection) | ||
| { | ||
| using (mo) | ||
| { | ||
| return Convert.ToDouble(mo["LoadPercentage"] ?? 0.0); | ||
| } | ||
| } | ||
| return 0; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For WMI Win32_Processor, does LoadPercentage return one instance per physical processor package, and should a system-wide CPU usage value aggregate all returned instances?
💡 Result:
The WMI class Win32_Processor returns one instance per physical processor package (socket) [1][2][3]. The LoadPercentage property represents the load capacity of that specific physical processor, averaged over the last second [4][2]. Regarding system-wide CPU usage, you should not simply aggregate or average the LoadPercentage values from all Win32_Processor instances to determine total system CPU usage [5]. This approach is often inaccurate because Win32_Processor does not reliably report load across all modern multi-core or multi-threaded architectures [5]. For accurate system-wide CPU usage, it is recommended to use performance counter classes, such as Win32_PerfFormattedData_Counters_ProcessorInformation (specifically the _Total instance) or the Win32_PerfFormattedData_PerfOS_Processor class [1][6][5]. These classes are designed to provide aggregated performance data that correctly reflects the total system load [1][6].
Citations:
- 1: https://stackoverflow.com/questions/61956471/cpu-load-percentage-for-individual-processors
- 2: https://stackoverflow.com/questions/37006720/what-exactly-get-wmiobject-win32-processor-select-loadpercentage-returns
- 3: https://stackoverflow.com/questions/38370012/win32-processor-only-shows-cpu0
- 4: https://learn.microsoft.com/en-us/windows/win32/cimwin32prov/win32-processor
- 5: https://xkln.net/blog/analyzing-cpu-usage-with-powershell-wmi-and-excel/
- 6: https://learn.microsoft.com/en-us/archive/msdn-technet-forums/608a2bc0-3bf8-4845-b0d8-09f2624189f7
Consider using performance counters instead of Win32_Processor for system-wide CPU load.
Win32_Processor returns one instance per physical processor package, so this implementation reads only the first socket and misses data on multi-socket systems. However, aggregating LoadPercentage across Win32_Processor instances is not recommended—this approach is unreliable on modern multi-core architectures. Use Win32_PerfFormattedData_Counters_ProcessorInformation with the _Total instance instead, which provides accurate system-wide CPU load.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SysManager/SysManager/Services/SystemInfoService.cs` around lines 115 - 126,
QueryCpuLoad currently uses ManagementObjectSearcher("SELECT LoadPercentage FROM
Win32_Processor") and returns the first Win32_Processor instance, which misses
multi-socket/system-wide load; replace the WMI query to read from
Win32_PerfFormattedData_Counters_ProcessorInformation (select
PercentProcessorTime where Name = "_Total" or use the _Total instance) and parse
that value to a double, keeping the same method signature, disposing the
searcher/collection as before (referencing QueryCpuLoad,
ManagementObjectSearcher, and ManagementObjectCollection) and return 0 on
missing/null values.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (!string.IsNullOrEmpty(lastBootRaw)) | ||
| { | ||
| try { return DateTime.Now - ManagementDateTimeConverter.ToDateTime(lastBootRaw); } | ||
| catch (FormatException) { } |
| { | ||
| try { return DateTime.Now - ManagementDateTimeConverter.ToDateTime(lastBootRaw); } | ||
| catch (FormatException) { } | ||
| catch (InvalidCastException) { } |
…cy guard (#406) - PERF-M1: SystemInfoService caches static WMI data (OS, CPU, disks) - PERF-M2: NetworkSharedState.RecomputeStats uses manual loops (no LINQ allocs) - PERF-M4: TrayIconService skips overlapping WMI calls via Interlocked guard Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
3 participants
Summary
Addresses MEDIUM performance findings from the comprehensive code review.
Changes
Files changed (4)
Services: SystemInfoService, TrayIconService
ViewModels: NetworkSharedState
Docs: CHANGELOG.md
Build
0 errors, 0 new warnings.
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor