perf: skip loadCliConfig + refreshAuth in parent for non-sandbox startup#24804
perf: skip loadCliConfig + refreshAuth in parent for non-sandbox startup#24804bzqzheng wants to merge 2 commits intogoogle-gemini:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello, 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 optimizes the CLI startup sequence by deferring heavy initialization tasks to the child process in non-sandbox environments. By eliminating redundant configuration loading and network-bound authentication calls in the parent process, the application achieves a faster and more efficient launch. The logic remains unchanged for sandbox and command-mode execution paths to ensure compatibility. 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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request restructures the CLI startup logic to optimize performance by skipping redundant configuration and authentication during app relaunch. However, the changes introduce significant code duplication and a performance regression where initialization tasks are executed twice when already inside a sandbox. Feedback highlights the need to refactor shared logic into a reusable function to maintain consistency and avoid unnecessary overhead.
packages/cli/src/gemini.tsx
Outdated
| } else { | ||
| // Non-relaunch path (sandbox env or command mode): auth is needed here | ||
| // because we won't be relaunching. | ||
| const partialConfig = await loadCliConfig( | ||
| settings.merged, | ||
| sessionId, | ||
| argv, | ||
| { | ||
| projectHooks: settings.workspace.settings.hooks, | ||
| }, | ||
| ); | ||
| adminControlsListner.setConfig(partialConfig); | ||
|
|
||
| if (!settings.merged.security.auth.useExternal && !argv.isCommand) { | ||
| try { | ||
| if ( | ||
| partialConfig.isInteractive() && | ||
| settings.merged.security.auth.selectedType | ||
| ) { | ||
| const err = validateAuthMethod( | ||
| settings.merged.security.auth.selectedType, | ||
| ); | ||
| if (err) { | ||
| throw new Error(err); | ||
| } | ||
|
|
||
| await partialConfig.refreshAuth( | ||
| settings.merged.security.auth.selectedType, | ||
| ); | ||
| } else if (!partialConfig.isInteractive()) { | ||
| const authType = await validateNonInteractiveAuth( | ||
| settings.merged.security.auth.selectedType, | ||
| settings.merged.security.auth.useExternal, | ||
| partialConfig, | ||
| settings, | ||
| ); | ||
| await partialConfig.refreshAuth(authType); | ||
| } | ||
| } catch (err) { | ||
| if (err instanceof ValidationCancelledError) { | ||
| await runExitCleanup(); | ||
| process.exit(ExitCodes.SUCCESS); | ||
| } | ||
|
|
||
| if (!(err instanceof ValidationRequiredError)) { | ||
| debugLogger.error('Error authenticating:', err); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const remoteAdminSettings = partialConfig.getRemoteAdminSettings(); | ||
| if (remoteAdminSettings) { | ||
| settings.setRemoteAdminSettings(remoteAdminSettings); | ||
| } | ||
|
|
||
| await runDeferredCommand(settings.merged); | ||
| } |
There was a problem hiding this comment.
This else block introduces a performance regression for the sandbox environment. When process.env['SANDBOX'] is true (meaning we are already executing inside the sandbox), this block performs a full loadCliConfig and refreshAuth. However, the execution then continues to line 508, where loadCliConfig is called a second time, and later to initializeApp (line 602) which performs authentication again. Since this PR's objective is to optimize startup time, performing these expensive operations twice inside the sandbox is counterproductive. This block should be skipped if process.env['SANDBOX'] is true, as the main initialization starting at line 507 handles this correctly.
References
- When the application relaunches into a sandbox, it re-executes the main function. This re-execution includes the logic for fetching and applying remote admin settings, making it unnecessary to perform these steps manually before the main initialization.
There was a problem hiding this comment.
Not a new regression. The double load when SANDBOX=true is pre-existing behavior. The old code ran loadCliConfig unconditionally at line ~327 (before the sandbox branch), then again at line ~507. Both old and new code execute it twice in this path. Skipping the else block would break things: it also calls getRemoteAdminSettings() and runDeferredCommand() which are needed before the main init at line 507.
| const partialConfig = await loadCliConfig( | ||
| settings.merged, | ||
| sessionId, | ||
| argv, | ||
| { | ||
| projectHooks: settings.workspace.settings.hooks, | ||
| }, | ||
| ); | ||
| adminControlsListner.setConfig(partialConfig); | ||
|
|
||
| if (!settings.merged.security.auth.useExternal && !argv.isCommand) { | ||
| try { | ||
| if ( | ||
| partialConfig.isInteractive() && | ||
| settings.merged.security.auth.selectedType | ||
| ) { | ||
| const err = validateAuthMethod( | ||
| settings.merged.security.auth.selectedType, | ||
| ); | ||
| if (err) { | ||
| throw new Error(err); | ||
| } | ||
|
|
||
| await partialConfig.refreshAuth( | ||
| settings.merged.security.auth.selectedType, | ||
| ); | ||
| } else if (!partialConfig.isInteractive()) { | ||
| const authType = await validateNonInteractiveAuth( | ||
| settings.merged.security.auth.selectedType, | ||
| settings.merged.security.auth.useExternal, | ||
| partialConfig, | ||
| settings, | ||
| ); | ||
| await partialConfig.refreshAuth(authType); | ||
| } | ||
| } catch (err) { | ||
| if (err instanceof ValidationCancelledError) { | ||
| await runExitCleanup(); | ||
| process.exit(ExitCodes.SUCCESS); | ||
| } | ||
|
|
||
| if (!(err instanceof ValidationRequiredError)) { | ||
| debugLogger.error('Error authenticating:', err); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const remoteAdminSettings = partialConfig.getRemoteAdminSettings(); | ||
| if (remoteAdminSettings) { | ||
| settings.setRemoteAdminSettings(remoteAdminSettings); | ||
| } | ||
|
|
||
| await runDeferredCommand(settings.merged); |
There was a problem hiding this comment.
The logic for loading configuration, refreshing authentication, and handling remote admin settings is now duplicated between the if (sandboxConfig) block (lines 345-400) and this else block. This duplication increases maintenance overhead and has already introduced inconsistencies, such as the missing initialAuthFailed check in this second block. Please refactor this shared initialization logic into a reusable function or restructure the control flow to avoid duplication.
There was a problem hiding this comment.
Good catch on the duplication — addressing in a follow-up commit by extracting a performEarlyInit() function. Re: the missing initialAuthFailed: this is intentional, not a bug. The sandbox path needs a hard exit (FATAL_AUTHENTICATION_ERROR) before entering the sandbox because it blocks OAuth redirects. The else path falls through to the full init at line ~507 which handles auth independently, so an early fatal exit would be wrong.
In the common non-sandbox case, the parent process now skips expensive initialization (loadCliConfig, refreshAuth) and relaunches immediately. The child process handles all initialization on its own. Previously, the parent loaded full config (extensions, hierarchical memory search, policy engine) and made 3-5 network calls (OAuth, quota, experiments, admin controls) only to discard everything when spawning the child — which then repeated the same work. This eliminates ~500-1000ms of duplicated work on every startup. The sandbox path is completely unchanged. Closes google-gemini#24776
The sandbox and non-relaunch code paths had identical ~40-line blocks
for loadCliConfig, refreshAuth, getRemoteAdminSettings, and
runDeferredCommand. Extract into performEarlyInit() which returns
{ partialConfig, authFailed } so callers can choose their error
handling strategy.
Behavioral change: the else block (SANDBOX env / command mode) now
tracks authFailed (previously it silently swallowed auth errors).
This is safe because the fallback path at line ~507 handles auth
independently — the early failure just means the pre-init attempt
didn't succeed.
bzqzheng
force-pushed
the
perf/skip-parent-config-auth
branch
from
2b9a365 to
51865fd
Compare
|
@jacob314 @scidomino — PTAL. This is a perf optimization that skips loadCliConfig + refreshAuth in the parent process for the non-sandbox startup path (~500-1000ms improvement). Addresses the duplication concern from automated review in a follow-up commit (extracted |
1 participant
Summary
Skip
loadCliConfigandrefreshAuthin the parent process for the non-sandbox startup path, eliminating ~500-1000ms of duplicated work on every startup.Details
In the common non-sandbox case, the parent process previously ran the full
loadCliConfig(extension loading, hierarchical memory search, policy engine construction) andrefreshAuth(3-5 network calls) before relaunching the child — which then repeated all of the same work from scratch.This PR restructures
gemini.tsxso that:loadCliConfig/refreshAuth, relaunches immediately. The child handles all initialization.The key insight: the parent only needs
settingsandmemoryArgsto callrelaunchAppInChildProcess. ThepartialConfig,refreshAuth,remoteAdminSettings, andrunDeferredCommandare only consumed by the sandbox branch or are re-done by the child.Closes #24776
How to Validate
npm run build— should pass (verified)geminiinteractively — should start normally, no auth issues--sandboxflag — sandbox path should be unchanged-p "hello") — non-interactive mode should workPre-Merge Checklist