Skip to content

perf(core): reduce Git snapshot processes - #6784

Merged
wenshao merged 2 commits into
QwenLM:mainfrom
dexhunter:perf/git-snapshot-processes
Jul 14, 2026
Merged

perf(core): reduce Git snapshot processes#6784
wenshao merged 2 commits into
QwenLM:mainfrom
dexhunter:perf/git-snapshot-processes

Conversation

@dexhunter

@dexhunter dexhunter commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

What this PR does

This combines the branch and short-status reads used by the main-session system instruction into one git status --short --branch process. It parses only the added branch header, preserves the existing short-status path and color behavior, keeps git log live on every session, and fixes the command locale so detached and unborn branch labels are stable.

Why it's needed

getRecentGitStatus() runs while building every main-session system instruction. It previously launched three synchronous Git processes per session: branch, status, and log. In a long-lived qwen serve process, that work is repeated for each new session on the shared event loop. The new command returns branch and the same short-status remainder together, reducing the deterministic process count from 3 to 2 without caching Git output or reimplementing ref semantics.

Reviewer Test Plan

How to verify

  1. Run npx vitest run packages/core/src/utils/gitUtils.test.ts packages/core/src/core/client.test.ts --pool=threads --poolOptions.threads.minThreads=1 --poolOptions.threads.maxThreads=1; all 255 tests should pass.
  2. Run npm run build and npm run typecheck; both commands should complete successfully.
  3. Inspect getRecentGitStatus() and confirm it invokes git status --short --branch once plus git log once. The branch header is stripped from the status output, VT controls are removed only for branch parsing, and the status remainder is unchanged.
  4. Confirm the unit cases cover tracking details, forced status color, relative paths from a repository subdirectory, unborn branches, and detached HEAD.

The external strict evaluator imports the built candidate, traces real Git child processes over 31 warm session initializations, and compares the complete generated system instruction byte-for-byte with the prior three-command implementation. It also exercises tracked/index mutations, branch and commit changes, linked worktrees, separate repositories, metadata deletion, working-directory changes, reset, subdirectory renames, forced colors, and detached HEAD.

Evidence (Before & After)

This is not a UI change. The deterministic primary metric and same-machine timing diagnostics were:

Metric Before After Change
Git child processes per session 3.0 2.0 -33.3%
median system-instruction stage 26.01 ms 11.20 ms -56.9%
p95 system-instruction stage 26.94 ms 11.94 ms -55.7%

The two final candidate runs measured 11.53 ms and 11.20 ms median, and every warm sample executed exactly two Git processes. Timing is hardware-sensitive; the process count is the primary metric. Public autoresearch trajectory: Weco dashboard.

Tested on

OS Status
🍏 macOS ⚠️ not tested locally
🪟 Windows ⚠️ not tested locally
🐧 Linux ✅ tested

Environment (optional)

Ubuntu 24.04, Node.js 22.23.1. Validation included 255 focused tests, the full repository build, full workspace typecheck, changed-file ESLint and Prettier, two final strict evaluator runs at exactly 2.0, and git diff --check.

Risk & Scope

  • Main risk or tradeoff: branch parsing now depends on the stable --short --branch header. The command runs with LC_ALL=C, strips VT controls only from that header, and has regression coverage for tracking, color, unborn, detached, and subdirectory cases.
  • Not validated / out of scope: macOS and Windows were not run locally. This PR does not change later system-prompt construction or measure end-to-end daemon HTTP latency.
  • Breaking changes / migration notes: none. The generated Git snapshot text is required to remain byte-for-byte equivalent.

Linked Issues

Fixes #6312

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @dexhunter!

The body has solid technical content — good measurement table, correctness gates, and clear motivation. However, it's missing several required sections from the PR template:

  • Reviewer Test Plan — no "How to verify" steps, no Before/After evidence, no "Tested on" OS table. This is the most important missing section; it's what lets reviewers actually confirm the change works.
  • Risk & Scope — no risk/tradeoff, out-of-scope, or breaking-change notes.
  • Linked Issues — "Fixes #6312" appears in the body text but the section itself is absent.

Could you fill in the template sections? The measurement data and correctness gates you already have are great — they just need to live in the right structure so reviewers can find them.

中文说明

感谢 PR,@dexhunter

正文的技术内容很好——有详细的测量数据、正确性验证和清晰的动机说明。但缺少 PR 模板 中的几个必填部分:

  • Reviewer Test Plan — 缺少"如何验证"步骤、Before/After 证据、"测试平台" OS 表格。这是最重要的缺失部分,reviewer 需要它来确认改动有效。
  • Risk & Scope — 缺少风险/权衡、不在范围内、破坏性变更的说明。
  • Linked Issues — 正文中有"Fixes #6312"但缺少该部分。

能否补充模板部分?你已有的测量数据和正确性验证非常好,只需要放到正确的结构里,方便 reviewer 查找。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — no blockers. Suggestions are inline.

Comment thread packages/core/src/utils/gitUtils.ts
Comment thread packages/core/src/utils/gitUtils.ts
Comment thread packages/core/src/utils/gitUtils.ts
Comment thread packages/core/src/utils/gitUtils.ts
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Qwen precheck requires maintainer approval before automated triage/review.

Head SHA: 364d928ae9b702fff1ee7ee533ef5a950fa1ef99

Reason:

  • prompt_injection:system_prompt

A maintainer with write access can inspect the PR and manually request a run with @qwen-code /triage or @qwen-code /review. A new push requires a fresh precheck.

@dexhunter

Copy link
Copy Markdown
Contributor Author

The current head only adds the two requested regression tests for malformed branch headers and the Initial commit on variant. All review threads are answered and resolved; 257 focused tests, ESLint, Prettier, and diff checks pass. The precheck now requires write access, so could a maintainer trigger @qwen-code /review when convenient?

@wenshao

wenshao commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

✅ Local verification report — PR #6784

I built and ran this PR locally on real hardware as a merge reference. Result: the change is functionally equivalent to main and delivers the claimed process reduction. Recommend merge.

Environment: macOS (darwin-arm64) · Node v22.23.1 · git 2.x · PR head 364d928a · scope packages/core


1. Static gate — tests · typecheck · lint · format · build

Check Command Result
Focused unit tests the exact command from the test plan, single-thread pool 257 passed (gitUtils.test.ts 11 + client.test.ts 246)
Typecheck tsc --noEmit ✅ exit 0
Lint eslint on both changed files ✅ 0 problems
Format prettier --check on both changed files ✅ clean
Build build_package.js (core) dist emits the new status --short --branch command + LC_ALL

Note: the head now carries 257 focused tests (not 255) — the two extra cover the malformed-header and Initial commit on variants added after the initial description.

2. Byte-for-byte equivalence (the core claim)

I compiled both implementations side by side — main (3 git processes) and this PR (2 git processes) — and ran each against 12 real temporary git repositories (no mocks), diffing the generated snapshot text character-for-character:

Scenario Match
clean repo on main (with commits)
dirty: staged + unstaged + untracked + MM
branch with upstream ahead/behind header
unborn branch / empty repo ✅ (both → null)
detached HEAD
branch name with slashes (feature/foo-bar)
renamed file (R status)
non-ASCII / quoted filename
deleted file (D status)
long status (>2000 chars, truncation path)
linked worktree
called from a subdirectory (relative ../ paths)

12/12 identical. The LC_ALL=C addition is applied only to the status call; since none of branch/status/log output is localized, the snapshot text is unaffected — confirmed on this locale-capable (zh-CN) machine.

3. Process-count reduction (primary metric)

I traced real git child processes via a PATH shim that logs every invocation. The deterministic count matches the PR's headline claim exactly:

  • OLD (main): 3 processes — branch --show-current, status --short, log
  • NEW (PR): 2 processes — status --short --branch, log
  • → −1 process per session (−33.3%)


Notes for the record

  • Timing numbers were not independently reproduced (hardware-sensitive); the PR itself designates the process count as the primary metric, and that is confirmed above.
  • Minor behavior change (safe): if git status --short --branch ever returns output without a ## header, the new code throws → getRecentGitStatus returns null. In practice --branch always emits the header, so this path is only reachable under mocking. Fail-closed and low-risk.
  • Not covered locally: Windows / non-macOS runtime (out of scope; the command avoids shell builtins so it stays cmd.exe-safe).

Verdict: LGTM — safe to merge.

🇨🇳 中文版本(点击展开)

✅ 本地验证报告 — PR #6784

作为合并参考,我在真实环境本地构建并运行了该 PR。结论:该改动与 main 功能等价,并实现了其声称的进程数削减。建议合并。

环境: macOS (darwin-arm64) · Node v22.23.1 · git 2.x · PR head 364d928a · 范围 packages/core

1. 静态门禁 — 测试 · 类型检查 · Lint · 格式化 · 构建

检查项 命令 结果
聚焦单测 测试计划中的原始命令,单线程池 257 通过gitUtils.test.ts 11 + client.test.ts 246)
类型检查 tsc --noEmit ✅ 退出码 0
Lint 对两个改动文件运行 eslint ✅ 0 问题
格式化 对两个改动文件运行 prettier --check ✅ 通过
构建 build_package.js(core) dist 产物包含新命令 status --short --branchLC_ALL

说明:当前 head 实际有 257 个聚焦测试(而非 255)——新增的两个覆盖了畸形 header 与 Initial commit on 变体。

2. 逐字节等价性(核心主张)

我将 两个实现并排编译——main(3 个 git 进程)与本 PR(2 个 git 进程)——并在 12 个真实临时 git 仓库(无 mock)上分别运行,对生成的快照文本逐字符比对:

涵盖场景:干净仓库、暂存+未暂存+未跟踪+MM、带上游 ahead/behind header、未初始化/空仓库(两者均返回 null)、detached HEAD、带斜杠的分支名、重命名(R)、非 ASCII/带引号文件名、删除(D)、超长状态(>2000 字符触发截断)、关联 worktree、从子目录调用(相对 ../ 路径)。

12/12 完全一致。 LC_ALL=C 仅作用于 status 调用;由于 branch/status/log 的输出均不本地化,快照文本不受影响——已在本机(中文 zh-CN 环境)确认。

3. 进程数削减(主指标)

我通过 PATH 垫片(shim)记录每次 git 调用,追踪真实的 git 子进程。确定性计数与 PR 的主张完全一致:

  • 旧(main): 3 个进程 — branch --show-currentstatus --shortlog
  • 新(PR): 2 个进程 — status --short --branchlog
  • → 每个会话减少 1 个进程(−33.3%)

备注

  • 计时数据未独立复现(依赖硬件);PR 本身也将进程数定为主指标,该项已在上文确认。
  • 一处小的行为变化(安全):git status --short --branch 返回的输出不含 ## header,新代码会抛错 → getRecentGitStatus 返回 null。实际上 --branch 总会输出该 header,故该分支仅在 mock 下可达。为 fail-closed,风险低。
  • 本地未覆盖:Windows/非 macOS 运行时(超出范围;该命令不依赖 shell 内建命令,故对 cmd.exe 安全)。

结论:LGTM — 可安全合并。

@wenshao

wenshao commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — all required headings present with detailed evidence.

Problem: Observed optimization with metrics. getRecentGitStatus() runs on the hot path for every session creation, spawning 3 synchronous git processes. Issue #6312 documents this, and the PR provides before/after measurements (3→2 processes, ~57% median latency reduction). Real overhead, real numbers.

Direction: Aligned. Reducing process spawning on the per-session hot path is a clear performance win. The approach — combining git branch --show-current and git status --short into a single git status --short --branch — is the simplest possible way to eliminate one process call without caching or reimplementing ref semantics.

Size: 2 files changed, ~44 production logic lines + ~99 test lines in packages/core/src/utils/. Well under any threshold.

Approach: Scope is minimal — one function modified, regression tests added for every new edge case (tracking details, VT control stripping, unborn branches, initial commits, detached HEAD, malformed output). No drive-by refactors, no scope creep. This is how a focused perf PR should look.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ — 所有必要标题齐全,附有详细证据。

问题:有指标支撑的实际性能优化。getRecentGitStatus() 在每次会话创建的热路径上运行,原先启动 3 个同步 git 进程。Issue #6312 记录了此问题,PR 提供了 before/after 测量(3→2 进程,中位延迟降低约 57%)。真实的开销,真实的数据。

方向:对齐。减少每个会话热路径上的进程创建是明确的性能收益。将 git branch --show-currentgit status --short 合并为单个 git status --short --branch 是最简方案——无需缓存或重新实现引用语义。

规模:2 个文件变更,约 44 行生产逻辑 + 约 99 行测试,位于 packages/core/src/utils/。远低于任何阈值。

方案:范围最小化——修改一个函数,为每个新的边界情况添加回归测试(跟踪详情、VT 控制剥离、未出生分支、初始提交、detached HEAD、格式异常输出)。无附带重构,无范围蔓延。这是一个聚焦的性能 PR 应有的样子。

进入代码审查 🔍

Qwen Code · qwen3.7-max

Reviewed at 364d928ae9b702fff1ee7ee533ef5a950fa1ef99 · re-run with @qwen-code /triage

@wenshao

wenshao commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

Focused two-file change in packages/core/src/utils/gitUtils.ts:

  • Replaces git branch --show-current + git status --short with a single git status --short --branch, then parses the ## branch header from the combined output
  • Adds LC_ALL: 'C' to ensure stable locale for parsing across environments
  • Uses stripVTControlCharacters from node:util on the branch header only — status remainder is untouched, preserving existing color behavior
  • Branch parsing covers all edge cases: tracking info (...origin/feature [ahead 2]), unborn (No commits yet on new-branch), initial commit (Initial commit on new-branch), detached (HEAD (no branch)), and malformed output (throws, caught by outer try/catch → returns null)

No correctness issues found. The only downstream consumer is client.ts line 777 (this.cachedGitStatus = getRecentGitStatus(...)) — signature and output format are unchanged, so no downstream impact.

Reuse check: stripVTControlCharacters is imported from node:util (Node.js built-in) — correct choice, no need for a third-party dependency.

Test Results

gitUtils tests — 11/11 passed ✓

client tests — 246/246 passed ✓ (total 257, matching the PR's claim)

Build — passed ✓

Typecheck — passed ✓

Real-Scenario Testing

This is a non-UI internal optimization (system prompt construction). The tmux test verifies the CLI starts correctly and the system prompt (including git snapshot) is built without errors.

$ cd /home/github-runner/actions-runner-22/_work/qwen-code/qwen-code/.qwen/worktrees/triage && npm run dev -- -p 'what is 1+1? Reply with just the number.'

> @qwen-code/qwen-code@0.19.9 dev
> node scripts/dev.js -p what is 1+1? Reply with just the number.

2

CLI starts, builds the system instruction (which internally calls getRecentGitStatus with the new combined command), and responds correctly. No errors or warnings from the git snapshot path. The function is called during session initialization in client.ts — if it returned null unexpectedly, the system prompt would simply omit the git snapshot section (graceful degradation already in place).

中文说明

代码审查

聚焦的两文件变更,位于 packages/core/src/utils/gitUtils.ts

  • git branch --show-current + git status --short 替换为单个 git status --short --branch,然后从合并输出中解析 ## 分支头
  • 添加 LC_ALL: 'C' 确保跨环境解析的稳定性
  • 仅对分支头使用 stripVTControlCharacters(来自 node:util)——状态其余部分不变,保留现有颜色行为
  • 分支解析覆盖所有边界情况:跟踪信息、未出生分支、初始提交、detached HEAD、格式异常(抛出异常,被外层 try/catch 捕获 → 返回 null)

未发现正确性问题。唯一的下游消费者是 client.ts 第 777 行——函数签名和输出格式未变,无下游影响。

复用检查:stripVTControlCharacters 来自 node:util(Node.js 内置模块)——正确选择,无需第三方依赖。

测试结果

gitUtils 测试 — 11/11 通过 ✓
client 测试 — 246/246 通过 ✓(共 257 个,与 PR 声明一致)
构建 — 通过 ✓
类型检查 — 通过 ✓

真实场景测试

这是非 UI 的内部优化(系统提示构建)。tmux 测试验证 CLI 正常启动且系统提示(含 git 快照)构建无误。CLI 正确响应 "1+1" 为 "2",git 快照路径无错误或警告。

Qwen Code · qwen3.7-max

Reviewed at 364d928ae9b702fff1ee7ee533ef5a950fa1ef99 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 5/5 — Clean, focused perf PR that does exactly what it says.

This is a textbook optimization: reduce 3 synchronous git processes to 2 by combining two of them into a single git status --short --branch call. The implementation is minimal (~44 production lines), the edge case coverage is thorough (6 new test cases for branch header parsing), and the output format is preserved byte-for-byte so no downstream consumer is affected.

I independently considered how to eliminate one git process here — combining the branch and status reads is the obvious and correct approach. The PR's implementation matches that expectation and adds LC_ALL: 'C' for locale stability, which I wouldn't have thought of but is the right call.

All 257 tests pass, build and typecheck are clean, and the CLI runs correctly in tmux. The author has been responsive to review feedback (added regression tests for malformed headers and the Initial commit on variant). Maintainer @wenshao has already approved after local verification.

Approving. ✅

中文说明

信心度: 5/5 — 干净、聚焦的性能 PR,完全符合其声明。

这是一个教科书式的优化:将两个 git 命令合并为单个 git status --short --branch 调用,从而将 3 个同步 git 进程减少到 2 个。实现最小化(约 44 行生产代码),边界情况覆盖充分(6 个新的分支头解析测试用例),输出格式逐字节保持一致,不影响任何下游消费者。

我独立考虑了如何消除一个 git 进程——合并分支和状态读取是显而易见且正确的方案。PR 的实现符合预期,并添加了 LC_ALL: 'C' 确保区域稳定性,这是一个我可能不会想到但正确的决定。

全部 257 个测试通过,构建和类型检查正常,CLI 在 tmux 中运行正确。作者积极响应审查反馈。维护者 @wenshao 已在本地验证后批准。

批准。✅

Qwen Code · qwen3.7-max

Reviewed at 364d928ae9b702fff1ee7ee533ef5a950fa1ef99 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@wenshao
wenshao added this pull request to the merge queue Jul 14, 2026
Merged via the queue into QwenLM:main with commit aa00774 Jul 14, 2026
117 of 119 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tracking(serve): reduce per-session overhead on the daemon session-creation path

3 participants