fix: optimize buffer trimming and executable path resolution (PERF-M3, M5)#414
Conversation
📝 WalkthroughWalkthroughThis PR introduces two performance optimizations released in version 0.48.29. ChangesPerformance optimizations
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
SysManager/SysManager/Services/IconExtractorService.cs (1)
361-371: ⚡ Quick winConsider adding cache size limits to
_pathCachefor consistency.The icon cache
_cachehas a configurableMaxCacheSizewith eviction logic (lines 29, 52-63), but_pathCachewill grow unbounded throughout the application lifetime. While process names are relatively limited, adding consistent cache management would prevent potential memory accumulation and improve maintainability.♻️ Suggested cache size management pattern
// PERF-M5: Cache resolved executable paths to avoid re-scanning Program Files // directories on every process list refresh (~100+ subdirs per scan). private static readonly ConcurrentDictionary<string, string?> _pathCache = new(StringComparer.OrdinalIgnoreCase); +private static readonly object _pathEvictionLock = new(); + +/// <summary>Maximum number of cached paths before eviction kicks in.</summary> +public static int MaxPathCacheSize { get; set; } = 500; private static string? FindExecutableByName(string processName) { var exeName = processName.EndsWith(".exe", StringComparison.OrdinalIgnoreCase) ? processName : processName + ".exe"; + // Evict oldest entries when cache exceeds the limit + if (_pathCache.Count >= MaxPathCacheSize) + { + lock (_pathEvictionLock) + { + if (_pathCache.Count >= MaxPathCacheSize) + { + var keys = _pathCache.Keys.Take(_pathCache.Count / 2).ToList(); + foreach (var k in keys) _pathCache.TryRemove(k, out _); + } + } + } + return _pathCache.GetOrAdd(exeName, static name => ResolveExecutablePath(name)); }🤖 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/IconExtractorService.cs` around lines 361 - 371, The _pathCache currently is an unbounded ConcurrentDictionary used by FindExecutableByName (which calls ResolveExecutablePath); add bounded-cache behavior by replacing or wrapping _pathCache with a thread-safe LRU or size-limited structure (or implement simple eviction logic) that enforces a configurable MaxPathCacheSize and evicts least-recently-used or oldest entries when the limit is exceeded; update FindExecutableByName to use the new capped cache and ensure ResolveExecutablePath remains the value factory for misses while preserving StringComparer.OrdinalIgnoreCase for keys.
🤖 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.
Nitpick comments:
In `@SysManager/SysManager/Services/IconExtractorService.cs`:
- Around line 361-371: The _pathCache currently is an unbounded
ConcurrentDictionary used by FindExecutableByName (which calls
ResolveExecutablePath); add bounded-cache behavior by replacing or wrapping
_pathCache with a thread-safe LRU or size-limited structure (or implement simple
eviction logic) that enforces a configurable MaxPathCacheSize and evicts
least-recently-used or oldest entries when the limit is exceeded; update
FindExecutableByName to use the new capped cache and ensure
ResolveExecutablePath remains the value factory for misses while preserving
StringComparer.OrdinalIgnoreCase for keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e32e664d-bd5f-49b4-8efe-21248998d37e
📒 Files selected for processing (3)
CHANGELOG.mdSysManager/SysManager/Services/IconExtractorService.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/IconExtractorService.cs (1)
373-415: LGTM!SysManager/SysManager/ViewModels/NetworkSharedState.cs (1)
468-496: LGTM!CHANGELOG.md (1)
9-17: LGTM!
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…route DNS (#431) ## Summary **Batch 10** — final batch of the QA bug fix series. ### Issue #414 — Service WaitForStatus can hang - Wrapped \WaitForStatus\ in try-catch for \TimeoutException\ - Converts to descriptive \InvalidOperationException\ with user-friendly message - No more unhandled crash when a service takes >30s to start/stop ### Issue #415 — Snapshot not persisted to disk - Added \SaveSnapshot\/\LoadSnapshot\ to \PerformanceService\ - Persists \OriginalSnapshot\ as JSON in \%LOCALAPPDATA%\\SysManager\\performance-snapshot.json\ - \EnsureSnapshotAsync\ now: loads from disk → falls back to live capture → saves to disk - Snapshot survives app restarts — Restore All works after reopen ### Issue #416 — Traceroute DNS fire-and-forget race condition - Replaced fire-and-forget \Task.Run\ DNS lookup with awaited \Dns.GetHostEntryAsync\ - Added 1.5s timeout via \CancellationTokenSource.CreateLinkedTokenSource\ - \hop.HostName\ is now populated BEFORE emission via \HopCompleted\ - No more race condition where UI renders '*' and never updates ### Testing - Build: 0 errors Closes #414, Closes #415, Closes #416 Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
Adds CHANGELOG entries for all 9 releases from the QA bug fix session: - **v0.28.16** — Dispose lifecycle (#395, #410) - **v0.28.17** — CTS disposal + bare catch (#396, #413) - **v0.28.18** — Input validation + null checks (#397, #398) - **v0.28.19** — JSON error handling (#400) - **v0.28.20** — Drive scanning + cache eviction + ConfigureAwait (#401, #402, #403) - **v0.28.21** — Audit logging + error messages (#405, #407) - **v0.28.22** — SHA256 verification (#408, #409) - **v0.28.23** — Service timeout + snapshot persist + traceroute DNS (#414, #415, #416) - **v0.28.24** — Accessibility (#411) 18 bugs fixed in total. Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
…, M5) (#414) Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
Summary
Resolves two remaining medium-priority performance findings.
Changes
PERF-M5: FindExecutableByName path caching
PERF-M3: TrimBuffer O(n) optimization
Testing
Summary by CodeRabbit