Skip to content

Commit 4868125

Browse files
committed
perf(cli): keep node:http and ICU off the CLI startup path (-54% warm call)
A warm `appstate` cost 146ms; 79ms of that was startup work no local command needs. `node:http` was value-imported by four modules inside the eager import closure of `src/cli.ts`, so every invocation initialized undici -- and, under `NODE_USE_SYSTEM_CA=1`, the macOS trust store behind it. That is ~70ms of mostly off-CPU time (a blocking trustd lookup, invisible to --cpu-prof), paid by commands that never speak HTTP: the default daemon transport is a plain `node:net` socket. Cheap daemon-only commands paid it in full; only commands slow enough to absorb it, like `snapshot`, did not. Separately, `option-schema.ts` sorted its specs with `localeCompare` at module scope. The first `localeCompare` in a process costs ~5.4ms of ICU collator initialization (every later one is ~0.002ms), so the CLI paid ICU init purely to alphabetize an internal list. A case-insensitive comparator reproduces the exact same order for the ASCII flag-key vocabulary -- verified against all 159 keys. Both HTTP modules are now loaded on demand, via `.default` so they stay the same mutable module object a static import yields (the transport tests stub `http.request` on it). Behavior is unchanged: the HTTP transport, remote daemons and artifact upload/download all still work. Interleaved A/B, alternating prebuilt dists in one loop, 5-min load avg < 10, n=30 per arm: appstate 146.4ms -> 67.6ms -53.8% session list 137.9ms -> 57.7ms -58.2% snapshot -i 195.5ms -> 188.8ms -3.4% (device-work bound) Without NODE_USE_SYSTEM_CA the appstate win is 73.9ms -> 57.9ms (-22%), so this is not purely an artifact of that setting. Measured and rejected: splitting command metadata from runtime handlers (the facet restructure deferred by #1660). registry.js is 142kB of the 643kB eager closure but only ~1.6ms of module evaluation, and ALL module compilation is ~9.5ms -- the whole refactor could not clear the 20ms bar set for attempting it. A unit test walks the eager closure of src/cli.ts and fails if any file in it value-imports node:http/node:https again; type-only imports stay allowed.
1 parent 13bc70f commit 4868125

6 files changed

Lines changed: 137 additions & 15 deletions

File tree

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
import { expect, test } from 'vitest';
2+
import fs from 'node:fs';
3+
import path from 'node:path';
4+
5+
/**
6+
* Every CLI invocation eagerly evaluates the static import closure of
7+
* `src/cli.ts`. Importing `node:http` (or `node:https`) for a VALUE inside that
8+
* closure initializes undici and, under `NODE_USE_SYSTEM_CA=1`, the platform
9+
* trust store as well -- ~79ms added to every warm command on macOS, including
10+
* ones that never speak HTTP (the default daemon transport is a `node:net`
11+
* socket). The HTTP daemon transport, remote-artifact upload and download all
12+
* load these modules on demand instead.
13+
*
14+
* Type-only imports are free and stay allowed; this only rejects value imports.
15+
*/
16+
17+
const srcRoot = path.resolve(import.meta.dirname, '..');
18+
// `from './x.ts'` / `from "./x.ts"`, excluding `import type ... from`.
19+
const STATIC_IMPORT =
20+
/(?:^|\n)\s*(?:import|export)\s+(?!type\s)([^;]*?)\s*from\s*['"]([^'"]+)['"]/g;
21+
22+
function collectStaticImports(source: string): { clause: string; specifier: string }[] {
23+
const found: { clause: string; specifier: string }[] = [];
24+
STATIC_IMPORT.lastIndex = 0;
25+
let match: RegExpExecArray | null = null;
26+
while ((match = STATIC_IMPORT.exec(source)) !== null) {
27+
found.push({ clause: match[1] ?? '', specifier: match[2] ?? '' });
28+
}
29+
return found;
30+
}
31+
32+
function resolveRelative(fromFile: string, specifier: string): string | null {
33+
const candidate = path.resolve(path.dirname(fromFile), specifier);
34+
if (fs.existsSync(candidate) && fs.statSync(candidate).isFile()) return candidate;
35+
for (const suffix of ['.ts', '.tsx', '/index.ts']) {
36+
const withSuffix = `${candidate}${suffix}`;
37+
if (fs.existsSync(withSuffix)) return withSuffix;
38+
}
39+
return null;
40+
}
41+
42+
function eagerClosureOfCli(): string[] {
43+
const queue = [path.join(srcRoot, 'cli.ts')];
44+
const visited = new Set<string>();
45+
while (queue.length > 0) {
46+
const current = queue.pop();
47+
if (!current || visited.has(current)) continue;
48+
visited.add(current);
49+
for (const { specifier } of collectStaticImports(fs.readFileSync(current, 'utf8'))) {
50+
if (!specifier.startsWith('.')) continue;
51+
const resolved = resolveRelative(current, specifier);
52+
if (resolved) queue.push(resolved);
53+
}
54+
}
55+
return [...visited];
56+
}
57+
58+
test('the CLI startup import closure never value-imports node:http or node:https', () => {
59+
const offenders: string[] = [];
60+
for (const file of eagerClosureOfCli()) {
61+
for (const { clause, specifier } of collectStaticImports(fs.readFileSync(file, 'utf8'))) {
62+
if (specifier !== 'node:http' && specifier !== 'node:https') continue;
63+
// `import type http from` is caught by the regex's negative lookahead;
64+
// `import { type IncomingMessage } from` is a value import of nothing.
65+
const importsOnlyTypes = clause
66+
.replace(/^\{|\}$/g, '')
67+
.split(',')
68+
.every((binding) => binding.trim() === '' || binding.trim().startsWith('type '));
69+
if (importsOnlyTypes) continue;
70+
offenders.push(`${path.relative(srcRoot, file)} -> ${specifier}`);
71+
}
72+
}
73+
74+
expect(
75+
offenders,
76+
'Load node:http / node:https on demand instead: a value import here costs every warm CLI ' +
77+
'command ~79ms of undici + system-CA initialization.',
78+
).toEqual([]);
79+
});
80+
81+
test('the CLI startup import closure is reachable and non-trivial', () => {
82+
// Guards the test above from silently passing because the walk found nothing.
83+
expect(eagerClosureOfCli().length).toBeGreaterThan(50);
84+
});

src/cli-schema/option-schema.ts

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,20 @@ function buildOptionSpecs(): OptionSpec[] {
9191
return supported.has(command);
9292
},
9393
}))
94-
.sort((left, right) => left.key.localeCompare(right.key));
94+
.sort((left, right) => compareOptionKeys(left.key, right.key));
95+
}
96+
97+
/**
98+
* Case-insensitive-then-codepoint ordering, matching what `localeCompare`
99+
* produces for the ASCII flag-key vocabulary. This runs at module load on
100+
* every CLI invocation, and the FIRST `localeCompare` in a process pays ~5ms
101+
* of ICU collator initialization — a cost the CLI otherwise never incurs.
102+
*/
103+
function compareOptionKeys(left: string, right: string): number {
104+
const leftLower = left.toLowerCase();
105+
const rightLower = right.toLowerCase();
106+
if (leftLower !== rightLower) return leftLower < rightLower ? -1 : 1;
107+
return left < right ? -1 : left > right ? 1 : 0;
95108
}
96109

97110
function primaryFlagDefinition(spec: OptionSpec): FlagDefinition {

src/daemon/client/daemon-client-progress.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
import type { RequestProgressEvent, RequestProgressSink } from '@agent-device/contracts/progress';
2-
import http from 'node:http';
2+
// Type-only: importing `node:http` for a value eagerly initializes undici
3+
// (~9ms in a fresh process), which the default socket transport never needs.
4+
import type http from 'node:http';
35
import type { Socket } from 'node:net';
46
import { AppError } from '@agent-device/kernel/errors';
57
import type { DaemonRequest, DaemonResponse } from '../types.ts';

src/daemon/client/daemon-client-transport.ts

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
import type { RequestProgressSink } from '@agent-device/contracts/progress';
22
import net from 'node:net';
3-
import http from 'node:http';
4-
import https from 'node:https';
53
import { AppError } from '@agent-device/kernel/errors';
64
import { readNodeHttpResponseBody } from '../../utils/node-http.ts';
75
import type { DaemonRequest, DaemonResponse } from '../types.ts';
@@ -20,6 +18,22 @@ import { DAEMON_RPC_PROTOCOL_VERSION } from '../http-health.ts';
2018
import { readVersion } from '../../utils/version.ts';
2119

2220
type ResolvedDaemonTransport = 'socket' | 'http';
21+
type NodeHttpRequester = Pick<typeof import('node:http'), 'request'>;
22+
23+
/**
24+
* `node:http` eagerly initializes undici, which costs ~9ms of startup in a
25+
* fresh process. The default local transport is a plain `node:net` socket and
26+
* never issues an HTTP request, so both HTTP modules are loaded on demand —
27+
* only the HTTP transport, remote daemons, and HTTP health checks pay for them.
28+
*/
29+
async function loadHttpRequester(protocol: string): Promise<NodeHttpRequester> {
30+
// `.default` (not the namespace) so this stays the very same module object a
31+
// static `import http from 'node:http'` yields — its `request` property is
32+
// mutable, which the transport's tests rely on to stub outbound requests.
33+
return protocol === 'https:'
34+
? (await import('node:https')).default
35+
: (await import('node:http')).default;
36+
}
2337
type SendRequestOptions = {
2438
onProgress?: RequestProgressSink;
2539
};
@@ -102,19 +116,19 @@ export async function readRemoteDaemonHealth(info: DaemonInfo): Promise<RemoteDa
102116
return health;
103117
}
104118

105-
function readDaemonHttpHealth(info: DaemonInfo): Promise<RemoteDaemonHealth> {
119+
async function readDaemonHttpHealth(info: DaemonInfo): Promise<RemoteDaemonHealth> {
106120
const endpoint = info.baseUrl
107121
? buildDaemonHttpUrl(info.baseUrl, 'health')
108122
: info.httpPort
109123
? `http://127.0.0.1:${info.httpPort}/health`
110124
: null;
111-
if (!endpoint) return Promise.resolve({ reachable: false });
125+
if (!endpoint) return { reachable: false };
112126
const url = new URL(endpoint);
113-
const transport = url.protocol === 'https:' ? https : http;
127+
const transport = await loadHttpRequester(url.protocol);
114128
const timeoutMs = info.baseUrl
115129
? REMOTE_DAEMON_HEALTHCHECK_TIMEOUT_MS
116130
: LOCAL_DAEMON_HEALTHCHECK_TIMEOUT_MS;
117-
return new Promise((resolve) => {
131+
return await new Promise((resolve) => {
118132
const headers = info.baseUrl ? buildDaemonHttpAuthHeaders(info.token) : {};
119133
const req = transport.request(
120134
{
@@ -380,9 +394,9 @@ async function sendHttpRequest(
380394
if (info.baseUrl) {
381395
Object.assign(headers, buildDaemonHttpAuthHeaders(info.token));
382396
}
397+
const transport = await loadHttpRequester(rpcUrl.protocol);
383398

384399
return await new Promise((resolve, reject) => {
385-
const transport = rpcUrl.protocol === 'https:' ? https : http;
386400
const request = transport.request(
387401
{
388402
protocol: rpcUrl.protocol,

src/remote/daemon-artifacts.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,4 @@
11
import fs from 'node:fs';
2-
import http from 'node:http';
3-
import https from 'node:https';
42
import path from 'node:path';
53
import { pipeline } from 'node:stream/promises';
64
import { AppError } from '@agent-device/kernel/errors';
@@ -349,7 +347,13 @@ type DownloadRemoteArtifactParams = {
349347

350348
export async function downloadRemoteArtifact(params: DownloadRemoteArtifactParams): Promise<void> {
351349
const artifactUrl = new URL(buildDaemonArtifactUrl(params.baseUrl, params.artifactId));
352-
const transport = artifactUrl.protocol === 'https:' ? https : http;
350+
// Loaded on demand: `prepareRemoteRequestArtifacts` runs on every CLI
351+
// request, but only a remote daemon ever downloads an artifact, and
352+
// importing `node:http` for a value costs ~9ms of undici init.
353+
const transport =
354+
artifactUrl.protocol === 'https:'
355+
? (await import('node:https')).default
356+
: (await import('node:http')).default;
353357
await fs.promises.mkdir(path.dirname(params.destinationPath), { recursive: true });
354358
await new Promise<void>((resolve, reject) => {
355359
let settled = false;

src/remote/upload-stream.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
import fs from 'node:fs';
2-
import http, { type IncomingHttpHeaders } from 'node:http';
3-
import https from 'node:https';
2+
import type { IncomingHttpHeaders } from 'node:http';
43
import path from 'node:path';
54
import { pipeline } from 'node:stream/promises';
65
import { AppError } from '@agent-device/kernel/errors';
@@ -65,7 +64,13 @@ async function streamFileToHttpRequestAttempt(options: {
6564
startOffset: number;
6665
progress?: UploadStreamProgressOptions;
6766
}): Promise<UploadStreamResponse> {
68-
const transport = options.url.protocol === 'https:' ? https : http;
67+
// Loaded on demand: this module sits in the CLI's eager import closure via
68+
// the remote-artifact upload client, but only a remote daemon ever uploads.
69+
// Importing `node:http` for a value costs ~9ms of undici init per process.
70+
const transport =
71+
options.url.protocol === 'https:'
72+
? (await import('node:https')).default
73+
: (await import('node:http')).default;
6974
const payloadSize = fs.statSync(options.payloadPath).size;
7075
const headers = buildUploadRequestHeaders(options.headers, options.startOffset, payloadSize);
7176
emitUploadAttemptStarted(options.progress, options.startOffset, payloadSize);

0 commit comments

Comments
 (0)