fix: reduce CPU load from file watchers and improve performance #7596
Conversation
- Increased debounce delays to reduce frequency of file system events - Fixed MCP Hub file watcher cleanup to prevent duplicate watchers - Optimized ripgrep process handling with early termination - Improved timeout handling in list-files scanning - Updated tests to match new debounce timings Fixes #7595
dosubot
bot
added
size:M
hannesrudolph
added
the
Issue/PR - Triage
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| this.initializeFilePaths() | ||
| } | ||
| }, 300) // Debounce for 300ms | ||
| }, 500) // Increased debounce to 500ms to reduce CPU load |
There was a problem hiding this comment.
The debounce values here (500ms) differ from other components (1000ms in file-watcher and McpHub). Could we consider making these configurable through VS Code settings? This would allow users to tune the delays based on their system performance - slower systems might benefit from longer delays, while faster systems could use shorter ones.
| if (results.length >= limit) { | ||
| rgProcess.kill() | ||
| clearTimeout(timeoutId) // Clear the timeout when we kill the process due to reaching the limit | ||
| if (results.length >= limit && !processKilled) { |
There was a problem hiding this comment.
Is this race condition handling intentional? The processKilled flag is checked and set in multiple places (lines 633, 635, 644, 650-653) without synchronization. While JavaScript is single-threaded, the async callbacks could potentially lead to edge cases where the process is killed multiple times or results are processed after kill. Consider using a more robust state machine or ensuring atomic flag operations.
| const newNames = new Set(Object.keys(newServers)) | ||
|
|
||
| // Delete removed servers | ||
| // Delete removed servers (this will also clean up their file watchers) |
There was a problem hiding this comment.
I notice the removeAllFileWatchers() call was removed here. The comment says "Delete removed servers (this will also clean up their file watchers)" and I see that deleteConnection() should handle the cleanup via removeFileWatchersForServer(). Just wanted to confirm this doesn't leave any orphaned watchers in edge cases?
| private accumulatedEvents: Map<string, { uri: vscode.Uri; type: "create" | "change" | "delete" }> = new Map() | ||
| private batchProcessDebounceTimer?: NodeJS.Timeout | ||
| private readonly BATCH_DEBOUNCE_DELAY_MS = 500 | ||
| private readonly BATCH_DEBOUNCE_DELAY_MS = 1000 // Increased from 500ms to reduce CPU load |
There was a problem hiding this comment.
Consider extracting these timing constants (500ms, 1000ms) to named constants at the top of the file. This would make them easier to find and modify, and the names could document their purpose (e.g., BATCH_DEBOUNCE_DELAY_MS, FILE_PROCESSING_TIMEOUT_MS).
|
Closing see #7595 (comment) |
Description
This PR addresses Issue #7595 regarding high CPU load in the VS Code extension.
Changes Made
Performance Optimizations
Increased debounce delays across multiple components to reduce frequency of file system events:
Improved process management:
Test Updates
Impact
These changes should significantly reduce CPU usage by:
Testing
Fixes #7595
Important
Reduce CPU load by increasing debounce delays and improving process management in file watchers and ripgrep handling.
WorkspaceTracker.ts,file-watcher.ts, andMcpHub.tsto reduce CPU load.list-files.tswith early termination and increased timeout from 10s to 15s.list-files.ts.WorkspaceTracker.spec.tsto match new debounce timings.This description was created by
for 34b81fa. You can customize this summary. It will automatically update as commits are pushed.