Repository navigation
perf(cli): code-split lowlight to cut startup V8 parse cost #4070
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
c201ba2
d581da0
872d971
fd9da1a
3a97c45
d5acc21
ffd7f8c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,9 +74,12 @@ const external = [ | |
|
|
||
| esbuild | ||
| .build({ | ||
| entryPoints: ['packages/cli/index.ts'], | ||
| entryPoints: { cli: 'packages/cli/index.ts' }, | ||
| bundle: true, | ||
| outfile: 'dist/cli.js', | ||
| outdir: 'dist', | ||
| entryNames: '[name]', | ||
| chunkNames: 'chunks/[name]-[hash]', | ||
| splitting: true, | ||
| platform: 'node', | ||
| format: 'esm', | ||
| target: 'node22', | ||
|
|
@@ -103,6 +106,11 @@ esbuild | |
| 'process.env.CLI_VERSION': JSON.stringify(pkg.version), | ||
| // Make global available for compatibility | ||
| global: 'globalThis', | ||
| // Redirect free __dirname/__filename references to the shim so that | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] — DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a contributor-facing warning to the Left the ESLint rule out for now — adding a custom rule (no plugin handles this exact pattern, since the rule has to be "bare reference not shadowed by a local same-name binding") felt heavier than the bug count justifies. If we hit this trap a third time I'd happily land a small custom rule under |
||
| // vendored libraries that emit their own `var __dirname` locals don't | ||
| // collide with our injected bindings when code-splitting is enabled. | ||
| __dirname: '__qwen_dirname', | ||
| __filename: '__qwen_filename', | ||
| }, | ||
| loader: { '.node': 'file' }, | ||
| plugins: [wasmBinaryPlugin, wasmLoader({ mode: 'embedded' })], | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,7 +6,6 @@ | |||||||||||||||||
|
|
||||||||||||||||||
| import React from 'react'; | ||||||||||||||||||
| import { Text, Box } from 'ink'; | ||||||||||||||||||
| import { common, createLowlight } from 'lowlight'; | ||||||||||||||||||
| import type { | ||||||||||||||||||
| Root, | ||||||||||||||||||
| Element, | ||||||||||||||||||
|
|
@@ -22,11 +21,17 @@ import { | |||||||||||||||||
| } from '../components/shared/MaxSizedBox.js'; | ||||||||||||||||||
| import type { LoadedSettings } from '../../config/settings.js'; | ||||||||||||||||||
| import { createDebugLogger } from '@qwen-code/qwen-code-core'; | ||||||||||||||||||
| import { getLowlightInstance, loadLowlight } from './lowlightLoader.js'; | ||||||||||||||||||
|
|
||||||||||||||||||
| // Configure theming and parsing utilities. | ||||||||||||||||||
| const lowlight = createLowlight(common); | ||||||||||||||||||
| const debugLogger = createDebugLogger('CODE_COLORIZER'); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Lowlight is heavy (~1.5 MB bundled, ~36–60 ms V8 parse). It's loaded lazily | ||||||||||||||||||
| // from `./lowlightLoader.js` via dynamic import so it lives in a separate | ||||||||||||||||||
| // esbuild chunk that's only parsed once a code block actually needs | ||||||||||||||||||
| // highlighting. Callers see plain text for the very first render and the | ||||||||||||||||||
| // highlighted version once React next re-renders the surrounding subtree | ||||||||||||||||||
| // (typically on the next user keystroke or message). | ||||||||||||||||||
|
|
||||||||||||||||||
| function renderHastNode( | ||||||||||||||||||
| node: Root | Element | HastText | RootContent, | ||||||||||||||||||
| theme: Theme, | ||||||||||||||||||
|
|
@@ -97,11 +102,21 @@ function highlightAndRenderLine( | |||||||||||||||||
| language: string | null, | ||||||||||||||||||
| theme: Theme, | ||||||||||||||||||
| ): React.ReactNode { | ||||||||||||||||||
| // Trigger the lazy load on first use; until it resolves, fall back to a | ||||||||||||||||||
| // plain-text rendering of the line. The next React render of the | ||||||||||||||||||
| // surrounding subtree will pick up the highlighted version. | ||||||||||||||||||
| const ll = getLowlightInstance(); | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] On failure,
Suggested change
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 3a97c45 with the exact pattern you suggested: extracted Side benefit: |
||||||||||||||||||
| if (!ll) { | ||||||||||||||||||
| void loadLowlight().catch((err) => { | ||||||||||||||||||
| debugLogger.error('[CodeColorizer] failed to load lowlight:', err); | ||||||||||||||||||
| }); | ||||||||||||||||||
| return line; | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The header comment promises that the highlighted version arrives "once React next re-renders the surrounding subtree (typically on the next user keystroke or message)." That holds for items still in the pending area, but not for items committed to ink's So a code block that lands in The simplest fix is to fire — claude-opus-4-7[1m] via Qwen Code /qreview
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch on the Also updated the |
||||||||||||||||||
| } | ||||||||||||||||||
| try { | ||||||||||||||||||
| const getHighlightedLine = () => | ||||||||||||||||||
| !language || !lowlight.registered(language) | ||||||||||||||||||
| ? lowlight.highlightAuto(line) | ||||||||||||||||||
| : lowlight.highlight(language, line); | ||||||||||||||||||
| !language || !ll.registered(language) | ||||||||||||||||||
| ? ll.highlightAuto(line) | ||||||||||||||||||
| : ll.highlight(language, line); | ||||||||||||||||||
|
|
||||||||||||||||||
| const renderedNode = renderHastNode(getHighlightedLine(), theme, undefined); | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,53 @@ | ||||||||||||||||||
| /** | ||||||||||||||||||
| * @license | ||||||||||||||||||
| * Copyright 2025 Google LLC | ||||||||||||||||||
| * SPDX-License-Identifier: Apache-2.0 | ||||||||||||||||||
| */ | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Standalone loader for the lowlight syntax-highlight engine. | ||||||||||||||||||
| * | ||||||||||||||||||
| * Kept in its own module — with zero imports beyond `lowlight` itself — so | ||||||||||||||||||
| * that priming the cache from `test-setup.ts` does not transitively pull | ||||||||||||||||||
| * `themeManager`, settings, or `@qwen-code/qwen-code-core` into every test | ||||||||||||||||||
| * file's module graph. That cascade was observed to alter theme/config test | ||||||||||||||||||
| * outcomes (e.g. theme-manager auto-detection and QWEN_HOME env tests). | ||||||||||||||||||
| */ | ||||||||||||||||||
|
|
||||||||||||||||||
| import type { Root } from 'hast'; | ||||||||||||||||||
|
|
||||||||||||||||||
| export type Lowlight = { | ||||||||||||||||||
| registered(language: string): boolean; | ||||||||||||||||||
| highlight(language: string, value: string): Root; | ||||||||||||||||||
| highlightAuto(value: string): Root; | ||||||||||||||||||
| }; | ||||||||||||||||||
|
|
||||||||||||||||||
| let lowlightInstance: Lowlight | null = null; | ||||||||||||||||||
| let lowlightLoad: Promise<Lowlight> | null = null; | ||||||||||||||||||
|
|
||||||||||||||||||
| export function getLowlightInstance(): Lowlight | null { | ||||||||||||||||||
| return lowlightInstance; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Kicks off (or returns the in-flight) load of the lowlight chunk. Exported | ||||||||||||||||||
| * for two callers: | ||||||||||||||||||
| * 1. `CodeColorizer.tsx` — fires the load on first colorize call so the | ||||||||||||||||||
| * next React commit picks up the highlighted output. | ||||||||||||||||||
| * 2. `test-setup.ts` — awaits this once to keep snapshot tests | ||||||||||||||||||
| * deterministic without dragging more modules into the test graph. | ||||||||||||||||||
| */ | ||||||||||||||||||
| export function loadLowlight(): Promise<Lowlight> { | ||||||||||||||||||
| if (lowlightInstance) return Promise.resolve(lowlightInstance); | ||||||||||||||||||
| if (lowlightLoad) return lowlightLoad; | ||||||||||||||||||
| lowlightLoad = import('lowlight') | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion]
Suggested change
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in d5acc21 — added the three-method shape check before the cast. A mismatched object now throws a descriptive Error which routes through the existing |
||||||||||||||||||
| .then((mod) => { | ||||||||||||||||||
| lowlightInstance = mod.createLowlight(mod.common) as Lowlight; | ||||||||||||||||||
| return lowlightInstance; | ||||||||||||||||||
| }) | ||||||||||||||||||
| .catch((err) => { | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The Consider adding a permanent-failure sentinel so once
Suggested change
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 872d971. Added a The |
||||||||||||||||||
| lowlightLoad = null; | ||||||||||||||||||
| throw err; | ||||||||||||||||||
| }); | ||||||||||||||||||
| return lowlightLoad; | ||||||||||||||||||
| } | ||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,3 +16,13 @@ if (process.env['QWEN_DEBUG_LOG_FILE'] === undefined) { | |||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| import './src/test-utils/customMatchers.js'; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Lowlight is loaded asynchronously in production to keep it out of the | ||||||||||||||||||||||||
| // startup-critical bundle chunk. Snapshot tests render synchronously via | ||||||||||||||||||||||||
| // `lastFrame()` and would otherwise capture the plain-text fallback before | ||||||||||||||||||||||||
| // the dynamic import resolves. Prime the cache once here so every test sees | ||||||||||||||||||||||||
| // the fully-highlighted output. The loader is intentionally a tiny standalone | ||||||||||||||||||||||||
| // module (no transitive imports of themeManager / settings / core) so this | ||||||||||||||||||||||||
| // prime does not perturb any other test's module graph. | ||||||||||||||||||||||||
| import { loadLowlight } from './src/ui/utils/lowlightLoader.js'; | ||||||||||||||||||||||||
| await loadLowlight(); | ||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The top-level Wrap in try/catch to degrade gracefully:
Suggested change
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 872d971. The top-level |
||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -39,6 +39,7 @@ const TARGETS = new Map([ | |||||||||||
| const DIST_REQUIRED_PATHS = ['cli.js', 'vendor', 'bundled/qc-helper/docs']; | ||||||||||||
| const DIST_ALLOWED_ENTRIES = new Set([ | ||||||||||||
| 'cli.js', | ||||||||||||
| 'chunks', | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion]
Suggested change
— claude-opus-4-7[1m] via Qwen Code /qreview
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in d581da0. Verified on a fresh |
||||||||||||
| 'vendor', | ||||||||||||
| 'bundled', | ||||||||||||
| 'package.json', | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,25 +5,27 @@ | |||||||||||||||||||||||
| */ | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||
| * Shims for esbuild ESM bundles to support require() calls | ||||||||||||||||||||||||
| * This file is injected into the bundle via esbuild's inject option | ||||||||||||||||||||||||
| * Shims for esbuild ESM bundles. | ||||||||||||||||||||||||
| * | ||||||||||||||||||||||||
| * With code-splitting enabled, the inject is applied per-chunk and the | ||||||||||||||||||||||||
| * exported bindings cannot collide with `var __dirname` polyfills that | ||||||||||||||||||||||||
| * vendored libraries (e.g. yargs) emit in their own ESM compat layers. | ||||||||||||||||||||||||
| * To stay collision-free, this file exposes prefixed names; the build | ||||||||||||||||||||||||
| * config uses esbuild `define` to rewrite free `__dirname` / `__filename` | ||||||||||||||||||||||||
| * references in source to these prefixed identifiers, while leaving | ||||||||||||||||||||||||
| * vendor-declared locals untouched. | ||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| import { createRequire } from 'node:module'; | ||||||||||||||||||||||||
| import { fileURLToPath } from 'node:url'; | ||||||||||||||||||||||||
| import { dirname } from 'node:path'; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Create require function for the current module and make it global | ||||||||||||||||||||||||
| const _require = createRequire(import.meta.url); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Make require available globally for dynamic requires | ||||||||||||||||||||||||
| if (typeof globalThis.require === 'undefined') { | ||||||||||||||||||||||||
| globalThis.require = _require; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Export for esbuild injection | ||||||||||||||||||||||||
| export const require = _require; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Setup __filename and __dirname for compatibility | ||||||||||||||||||||||||
| export const __filename = fileURLToPath(import.meta.url); | ||||||||||||||||||||||||
| export const __dirname = dirname(__filename); | ||||||||||||||||||||||||
| export const __qwen_filename = fileURLToPath(import.meta.url); | ||||||||||||||||||||||||
| export const __qwen_dirname = dirname(__qwen_filename); | ||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The header comment explains why the rename was needed (vendor Existing first-party files survive only because they happen to declare a local shadow ( Please add a contributor-facing note here (and adjacent to the
Suggested change
— claude-opus-4-7[1m] via Qwen Code /qreview
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 872d971. Added a contributor-facing block comment in
Cross-referenced from the |
||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -159,6 +159,7 @@ const distPackageJson = { | |
| }, | ||
| files: [ | ||
| 'cli.js', | ||
| 'chunks', | ||
| 'vendor', | ||
| '*.sb', | ||
| 'README.md', | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Critical] Enabling
splitting: truesilently breaks three internal modules that derive runtime paths fromimport.meta.url. Each one assumed it would be co-located withcli.jsindist/; once esbuild hoists them intodist/chunks/chunk-*.js,path.dirname(fileURLToPath(import.meta.url))resolves todist/chunks/, but the assets they look up still live one level above. Verified empirically against the bundle this PR produces:packages/core/src/skills/skill-manager.ts:88-92—bundledSkillsDirends up asdist/chunks/bundled. The actual skills directory isdist/bundled/{batch,loop,qc-helper,review}(created bycopy_bundle_assets.js). TheexistsSyncguard atskill-manager.ts:896swallows the miss into adebugLogger.warn, so all four bundled skills (/review,/qc-helper,/batch,/loop) silently disappear from any package built from this branch — no startup error, no user-visible signal.packages/core/src/utils/ripgrepUtils.ts:111-122—getBuiltinRipgrep()computesdist/chunks/vendor/ripgrep/<arch>-<plat>/rg, which doesn't exist. The bundled binary atdist/vendor/ripgrep/...becomes unreachable.resolveRipgrepthen falls back to systemrgif installed, or returnsnullon minimal hosts (CI runners, fresh containers) — makinggrepfall back to a slow internal scanner on the very environments the vendored binary was shipped for.packages/cli/src/i18n/index.ts:45-48— same pattern;getBuiltinLocalesDir()returnsdist/chunks/locales. User-visible behavior survives becausetryImportBundledTranslations(i18n/index.ts:114) loads via static glob import, but the loose-on-disk override path is now dead code.Existing unit tests don't catch any of this — they all run in source mode where
import.meta.urlresolves to the real source file and the heuristics work. Suggested fixes (any one is enough):packages/cli/index.tsso esbuild keeps them incli.js). Smallest change, preserves the lowlight win.cli.js(whoseimport.meta.urlisdist/cli.js) and use that as the anchor in each module instead ofimport.meta.url. Most robust...until the expected sibling (bundled/,vendor/ripgrep/,locales/) is found.At minimum, please add a smoke test that runs the built
dist/cli.jsand asserts each of the three is reachable, otherwise the next code-splitting change will hit the same trap.— claude-opus-4-7[1m] via Qwen Code /qreview
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified the bug empirically against the bundle this PR produces and fixed in 8d1da04d. All three modules now strip a trailing
chunkssegment whenpath.basename(moduleDir) === 'chunks', so the sibling-asset lookups resolve underdist/rather thandist/chunks/. In source / transpiled modes the basename is neverchunks, so the fallback is a no-op.Sanity check after rebuild:
Also added
chunkstoDIST_REQUIRED_PATHSincreate-standalone-package.jsso a regressed bundle without chunks fails the packager check (separate from the [Suggestion] thread on line 42).Re: smoke test for the built
dist/cli.js— agree this is the right long-term gate. Not adding it in this round to keep the diff focused on the three concrete misses; will file a follow-up so the next code-splitting change has CI cover.