perf(core): optimize Windows IDE process detection from O(N) to O(1) #11048
Conversation
Summary of ChangesHello @abhipatel12, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a critical performance enhancement for Windows users by overhauling the IDE process detection mechanism. Previously, the system suffered from significant delays and instability due to an inefficient, iterative method of querying process information. The new implementation introduces a highly optimized, single-query snapshot approach, which dramatically cuts down the time required to identify the IDE process, leading to a more responsive application and more reliable integration tests. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Size Change: +93 B (0%) Total Size: 17.8 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Code Review
This pull request introduces a significant performance optimization for IDE process detection on Windows by switching from an iterative, multi-process approach to a single process snapshot. This is a great improvement that should resolve the long delays and test flakiness. My review focuses on increasing the robustness of this new implementation. I've suggested adding logging to error-handling blocks that currently fail silently, which will make debugging much easier. I also found a small piece of unreachable code in the new process traversal logic and suggested a simplification. The updated tests look thorough and cover the new logic well.
| } catch (_e) { | ||
| // Fallback or error handling if PowerShell fails | ||
| } |
There was a problem hiding this comment.
Similar to the JSON parsing, this catch block silences errors from executing the PowerShell command. Logging these errors is crucial for diagnosing environment-specific problems, such as PowerShell not being in the PATH or permission issues. Please log the error to aid in troubleshooting.
| } catch (_e) { | |
| // Fallback or error handling if PowerShell fails | |
| } | |
| } catch (e) { | |
| // Fallback or error handling if PowerShell fails | |
| console.debug('Failed to get process list from PowerShell:', e); | |
| } |
| } catch (_e) { | ||
| console.debug(`Failed to get process info for pid ${pid}:`, _e); | ||
| return { parentPid: 0, name: '', command: '' }; | ||
| } |
There was a problem hiding this comment.
The console.debug call was removed from this catch block. While this function is expected to fail on Windows when used as a fallback, it's the primary method for Unix systems. Silently catching errors on Unix could make it difficult to debug why IDE detection is failing. Please consider re-adding the error logging.
} catch (e) {
console.debug(`Failed to get process info for pid ${pid}:`, e);
return { parentPid: 0, name: '', command: '' };
}| } else if (ancestors.length > 0) { | ||
| const target = ancestors[ancestors.length - 1]; | ||
| return { pid: target.pid, command: target.command }; | ||
| } | ||
|
|
||
| return { pid: myPid, command: myProc.command }; |
There was a problem hiding this comment.
The final return statement on line 200 appears to be unreachable. If myProc is defined, the ancestors array will always have at least one element, so the condition ancestors.length > 0 on line 195 will always be true, causing one of the branches in the if/else if to execute and return. This can be simplified by removing the unreachable code and the redundant check.
} else {
// For shorter chains, fall back to the root-most ancestor.
const target = ancestors[ancestors.length - 1];
return { pid: target.pid, command: target.command };
}
TLDR
This PR optimizes the IDE process detection logic on Windows, reducing CLI slash command loading time from ~30 seconds to ~1.6 seconds. It replaces an iterative, multi-process approach with a single snapshot approach, resolving a bottleneck that caused flaky integration tests (fixes #11037, contributes to #10769). This also helps alleviate the long delay Windows users faced when attempting to quit the application (due to slash commands not being loaded).
Dive Deeper
The Problem
On Windows,$O(N)$ operation taking roughly 30 seconds for a typical process depth. This delay caused race conditions in integration tests where commands were not fully loaded before tests began.
getIdeProcessInfopreviously attempted to find the IDE process by walking up the process tree, spawning a new PowerShell instance for every ancestor to queryWin32_Process. Since PowerShell startup is relatively slow, this resulted in anThe Solution
The implementation in$O(1)$ snapshot approach for Windows:
packages/core/src/ide/process-utils.tshas been changed to use anProcessId,ParentProcessId,Name, andCommandLinefor all system processes as JSON.Performance Impact
Based on local profiling:
Unit tests in
process-utils.test.tshave been updated to mock the single snapshot output rather than individual process lookups.Reviewer Test Plan
npm run test --workspace=@google/gemini-cli-coreto ensure the new logic and mocks are correct./ide status./ide status.Testing Matrix