From 983532a5cb5fa026c7013bbb40f5cbb9797d99ab Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Sun, 11 Oct 2026 20:27:15 -0700 Subject: [PATCH] fix(bench): make bench:idle-cpu build, sample CPU and parse the process table on Windows (#27622) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(bench): make the idle-CPU benchmark run and report CPU on Windows Three independent Windows defects in the idle-CPU benchmark (its report is what Windows performance work would cite): - `buildAppIfNeeded` spawned `npx`, which is `npx.cmd` on Windows, so execFileSync failed with ENOENT before any sampling. Build through `config/scripts/run-electron-vite-build.mjs` instead — the repo's own Windows-safe wrapper, which also runs the boot-graph verification. - `readWindowsProcesses` returned `cpuTimeSeconds: null`, so the sampler's per-process CPU delta (idle-cpu-process-sampling.mjs:198) fell back to the hardcoded `percentCpu: 0` and every CPU KPI in the report was zero. Read `UserModeTime`/`KernelModeTime` from `Win32_Process` (100 ns units) and keep `null` when a host does not return them. Measured on Windows, same options before and after the CPU fix: | class | mean CPU before | mean CPU after | | --- | ---: | ---: | | renderer | 0 | 2.20 | | gpu | 0 | 2.08 | | utility | 0 | 0.11 | | agent-or-node | 0 | 0.06 | | total | 0 | 4.51 (p95 8.40) | RSS is unchanged (934 MiB -> 937 MiB), so only the CPU reporting changed. Refs #25235 * fix(bench): reject a missing Win32_Process time instead of reading it as zero Review follow-up (CodeRabbit). `Number(null)` is `0`, which is also a real counter value, so an absent `UserModeTime`/`KernelModeTime` passed the finite check and the sampler would report a kernel-only delta instead of taking the fallback. Require an actual number from the CIM payload. Verified on Windows that the real payload still reports CPU once the check is tight: total mean 4.40%, p95 14.48%, renderer 3.51%. * test(bench): cover the Windows CIM CPU time mapping The mapping was only reachable through Get-CimInstance, so no test ran it. Lift it into cimCpuSeconds and assert the 100ns-to-seconds sum plus the missing / string / non-finite cases that must stay null. * fix(bench): read the Windows process table as UTF-8 so JSON.parse survives PowerShell writes to a redirected stdout in the OEM code page (437 here), which best-fits characters such as U+2022 and U+2192 in any running process's command line to raw 0x07 / 0x1A bytes. ConvertTo-Json had left them unescaped because they are not control characters in UTF-16, so the sampler's JSON.parse failed with "Bad control character in string literal" whenever such a process happened to be alive -- hence intermittent. Set [Console]::OutputEncoding to UTF-8, as the app's own CIM scan does, and cover it with a Windows-only test that fails without the fix. Fixes #25236 * fix(bench): pass the local runtime to fetchRepos in the idle-CPU fixture fetchRepos now requires a runtimeEnvironmentId, so the fixture's bare call threw "Cannot read properties of undefined (reading 'runtimeEnvironmentId')" before sampling on every platform. null selects this app's own catalog, as the e2e helpers already do. * ci(bench): run the Windows process-table test in the Windows lane The UTF-8 regression test self-skips off win32, so it must be registered in both Windows lists or it runs on no machine. --------- Co-authored-by: Paolo Co-authored-by: Orca Worker --- .github/workflows/pr.yml | 1 + config/scripts/idle-cpu-process-sampling.mjs | 21 +++++++++- .../idle-cpu-process-sampling.test.mjs | 40 ++++++++++++++++++- config/scripts/pr-code-change-scope.mjs | 1 + config/scripts/run-idle-cpu-benchmark.mjs | 18 ++++++--- 5 files changed, 72 insertions(+), 9 deletions(-) diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index ba11ca658fd..e2fcfe1b0f1 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -1236,6 +1236,7 @@ jobs: run: >- pnpm exec vitest run --config config/vitest.config.ts config/scripts/nsis-process-check.test.mjs + config/scripts/idle-cpu-process-sampling.test.mjs - name: Test Windows-specific boundaries run: >- diff --git a/config/scripts/idle-cpu-process-sampling.mjs b/config/scripts/idle-cpu-process-sampling.mjs index 81f9218e5ad..883b3c566bb 100644 --- a/config/scripts/idle-cpu-process-sampling.mjs +++ b/config/scripts/idle-cpu-process-sampling.mjs @@ -55,9 +55,26 @@ function readUnixProcesses() { return parseUnixProcesses(stdout) } +// `Win32_Process` times are 100 ns counts. Not `Number(value)`: that turns a missing +// field into a real-looking 0, so the CPU delta would silently go kernel-only. +function cimTimeSeconds(value) { + return typeof value === 'number' && Number.isFinite(value) ? value / 10_000_000 : null +} + +export function cimCpuSeconds(entry) { + const userModeSeconds = cimTimeSeconds(entry.UserModeTime) + const kernelModeSeconds = cimTimeSeconds(entry.KernelModeTime) + return userModeSeconds !== null && kernelModeSeconds !== null + ? userModeSeconds + kernelModeSeconds + : null +} + function readWindowsProcesses() { + // Why UTF-8: the default OEM code page best-fits characters such as `•` or `→` in any + // process's command line to raw control bytes (0x07, 0x1A), which JSON.parse rejects. const script = - 'Get-CimInstance Win32_Process | Select-Object ProcessId,ParentProcessId,WorkingSetSize,CommandLine | ConvertTo-Json -Compress' + '[Console]::OutputEncoding = [System.Text.Encoding]::UTF8; ' + + 'Get-CimInstance Win32_Process | Select-Object ProcessId,ParentProcessId,WorkingSetSize,UserModeTime,KernelModeTime,CommandLine | ConvertTo-Json -Compress' const result = spawnSync('powershell.exe', ['-NoProfile', '-Command', script], { encoding: 'utf8', maxBuffer: 20 * 1024 * 1024 @@ -71,7 +88,7 @@ function readWindowsProcesses() { pid: Number(entry.ProcessId), ppid: Number(entry.ParentProcessId), percentCpu: 0, - cpuTimeSeconds: null, + cpuTimeSeconds: cimCpuSeconds(entry), rssBytes: Number(entry.WorkingSetSize) || 0, command: String(entry.CommandLine || '') })) diff --git a/config/scripts/idle-cpu-process-sampling.test.mjs b/config/scripts/idle-cpu-process-sampling.test.mjs index d452c6e3ce0..2c25187ea9f 100644 --- a/config/scripts/idle-cpu-process-sampling.test.mjs +++ b/config/scripts/idle-cpu-process-sampling.test.mjs @@ -1,5 +1,10 @@ +import { spawn } from 'node:child_process' import { describe, expect, it } from 'vitest' -import { sampleProcessTreeUntilWorkloadsComplete } from './idle-cpu-process-sampling.mjs' +import { + cimCpuSeconds, + readProcessRows, + sampleProcessTreeUntilWorkloadsComplete +} from './idle-cpu-process-sampling.mjs' function createClock(workloadCompletesAt) { let currentMs = 0 @@ -27,6 +32,39 @@ function createClock(workloadCompletesAt) { return { now: () => currentMs, readRows, wait, workloadPromise } } +describe('Windows CIM CPU time mapping', () => { + it('converts the 100ns user and kernel times to a summed second count', () => { + // 10_000_000 and 25_000_000 units of 100ns are 1s and 2.5s. + expect(cimCpuSeconds({ UserModeTime: 10_000_000, KernelModeTime: 25_000_000 })).toBe(3.5) + }) + + it('maps a missing or non-numeric time to null instead of a kernel-only reading', () => { + expect(cimCpuSeconds({ UserModeTime: null, KernelModeTime: 10_000_000 })).toBeNull() + expect(cimCpuSeconds({ KernelModeTime: 10_000_000 })).toBeNull() + expect(cimCpuSeconds({ UserModeTime: '10000000', KernelModeTime: 10_000_000 })).toBeNull() + expect(cimCpuSeconds({ UserModeTime: Number.NaN, KernelModeTime: 10_000_000 })).toBeNull() + expect( + cimCpuSeconds({ UserModeTime: Number.POSITIVE_INFINITY, KernelModeTime: 10_000_000 }) + ).toBeNull() + }) +}) + +describe.skipIf(process.platform !== 'win32')('Windows process table read', () => { + it('parses a command line whose characters the OEM code page maps to control bytes', () => { + const marker = 'idle-cpu-oem • → ♪' + const child = spawn(process.execPath, ['-e', 'setTimeout(() => {}, 30_000)', marker], { + stdio: 'ignore' + }) + try { + const row = readProcessRows().find((candidate) => candidate.pid === child.pid) + expect(row?.command).toContain(marker) + expect(row?.cpuTimeSeconds).toEqual(expect.any(Number)) + } finally { + child.kill() + } + }, 30_000) +}) + describe('idle CPU process sampling window', () => { it('extends through a slow workload and captures a final CPU delta', async () => { const clock = createClock(40) diff --git a/config/scripts/pr-code-change-scope.mjs b/config/scripts/pr-code-change-scope.mjs index 149c49a2d11..e04946456e4 100644 --- a/config/scripts/pr-code-change-scope.mjs +++ b/config/scripts/pr-code-change-scope.mjs @@ -372,6 +372,7 @@ const WINDOWS_PACKAGE_TESTS = [ 'config/scripts/rebuild-native-deps-windows-process-tree.test.mjs', 'config/scripts/rebuild-native-deps-node-pty.test.mjs', 'config/scripts/nsis-process-check.test.mjs', + 'config/scripts/idle-cpu-process-sampling.test.mjs', 'config/scripts/ensure-native-runtime-job-ownership.test.mjs', 'config/scripts/verify-packaged-node-pty-job-ownership.test.mjs', 'config/scripts/windows-pe-machine.test.mjs', diff --git a/config/scripts/run-idle-cpu-benchmark.mjs b/config/scripts/run-idle-cpu-benchmark.mjs index 3892de8845d..eb3ebd3ea31 100644 --- a/config/scripts/run-idle-cpu-benchmark.mjs +++ b/config/scripts/run-idle-cpu-benchmark.mjs @@ -159,11 +159,17 @@ function buildAppIfNeeded(root, skipBuild) { throw new Error(`--skip-build requested, but ${mainPath} does not exist`) } console.log('[idle-cpu] building Electron app with electron-vite --mode e2e') - run('npx', ['electron-vite', 'build', '--mode', 'e2e'], { - cwd: root, - stdio: 'inherit', - env: { ...process.env, VITE_EXPOSE_STORE: 'true' } - }) + // Why through the wrapper: `npx` only resolves to an executable on POSIX; on Windows it is + // `npx.cmd`, which execFileSync cannot run, and the wrapper also verifies the boot graph. + run( + process.execPath, + [path.join(root, 'config', 'scripts', 'run-electron-vite-build.mjs'), '--mode', 'e2e'], + { + cwd: root, + stdio: 'inherit', + env: { ...process.env, VITE_EXPOSE_STORE: 'true' } + } + ) return mainPath } @@ -348,7 +354,7 @@ async function main() { return { error: added.error } } const store = window.__store - await store?.getState().fetchRepos() + await store?.getState().fetchRepos({ runtimeEnvironmentId: null }) const repo = store?.getState().repos.find((candidate) => candidate.id === added.repo.id) if (repo) { const detected = await store