Perf/cli startup - #41
Merged
Merged
Conversation
emb ps takes ~1.7s to wrap a ~0.45s docker compose ps call, and profiling attributes 86% of that to compiling JavaScript rather than running it. Optimising this needs measurement: the machine has ~20% run-to-run variance, and several plausible-sounding fixes turned out to be worth nothing. npm run bench:startup [dir] wall-clock ladder npm run bench:modules -- <cmd> [--cwd] modules compiled, bytes parsed The ladder isolates each stage (node floor, oclif bootstrap, config+monorepo init, docker subprocess) so a change can be attributed to a layer. Timing is amortized over batches and reported as a median, since single runs on a loaded machine are unusable. The module counter instruments both loader pipelines. A CJS-only count is badly misleading here: it reports 836 modules / 4.3 MB for emb ps, where the true figure is 2116 / 21.2 MB. The gap is @kubernetes/client-node, which is ESM, and which alone accounts for two thirds of everything emb ps parses. ESM is counted via a module.register() hook reporting over a MessagePort, and deduplicated against the CJS hook since a CommonJS file imported from ESM passes through both. register() must be installed from a `node --import` preload; during a --require preload the ESM loader is not yet initialized and it fails with a misleading "Missing internal module" error. Both scripts warn when oclif.manifest.json is absent. Without one, oclif loads every command module at startup to read its metadata, which inflates emb --version roughly 5x and describes a configuration no user runs. The manifest is also now gitignored — prepack generates it and postpack removes it, so it should never be committed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
src/index.ts re-exports everything, so importing from '@' makes the entire
codebase reachable from any entry point. That is why emb ps parses 21 MB of
JavaScript to shell out to docker compose ps.
26 of the 38 files importing '@' needed only getContext (or getContext plus
setContext, or plus the EmbContext type). All of those live in src/context.ts,
which has no runtime dependencies at all. Import them from there instead.
AbstractOperation also takes an explicit `import type` for EmbContext.
12 root-barrel imports remain; they pull real values (Monorepo, ResourceInfo,
CommandExecError) and need untangling separately.
No change to module count (2116) or wall-clock — expected, since the graph
stays fully connected until the last edge into each subtree is cut. The gain
is structural, and visible in the trace. The route from ps to Kubernetes was
ps.js -> docker/index.js -> docker/resources/index.js
-> DockerImageResource.js -> monorepo/index.js
-> monorepo/operations/index.js -> RunTasksOperation.js
-> kubernetes/operations/index.js -> @kubernetes/client-node
and is now
ps.js -> cli/index.js -> cli/abstract/index.js -> BaseCommand.js
-> kubernetes/client.js -> @kubernetes/client-node
That whole barrel route is severed, leaving a single edge — the eager
createKubernetesClient() call in BaseCommand.init() — between emb ps and
14.5 MB of Kubernetes ESM. Cutting it is now the highest-value remaining fix.
678/678 unit tests pass; adds no new lint problems (34 pre-exist on master).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@kubernetes/client-node is 826 files and 14.5MB of ESM — two thirds of everything emb ps had to parse. BaseCommand.init() built a client on every command, and RunTasksOperation imported the Kubernetes operations statically for a branch only its `kubernetes` executor reaches. EmbContext.kubernetes is now optional and populated on first use via getKubernetesClient(), which memoises on the context so a client injected by a test or a plugin wins over building a real one. The seven call sites that read context.kubernetes now await it; all were already async. RunTasksOperation imports the Kubernetes operations inside runKubernetes(). emb ps 1683ms -> 1125ms (-33%) emb tasks 1267ms -> 922ms modules 2116 / 21.20MB -> 1171 / 5.30MB (-75% source parsed) The import in createKubernetesClient() is dynamic, so the accessor is async. require(esm) does work for this package on Node 22.22 and would have allowed a synchronous accessor with no call-site changes, but it only landed unflagged in 22.12 while engines allows >=22.0.0. Two notes for whoever picks up the remaining phases: Phase 3 alone changed nothing — 2116 modules before and after. A second route to the SDK was still live through RunTasksOperation, and the import tracer had hidden it by reporting only the shortest path. A package is only gone when every edge to it is cut; verify with bench:modules, not with a path trace. tests/setup/set.context.ts built a real client purely to give vi.mockObject a shape. Once the import went dynamic that cost 337ms per call and pushed 19 tests past their 5s timeout. It is now left undefined, since every test that exercises Kubernetes injects its own mock. The unit suite went 39s -> 21s. 678/678 unit tests and the integration-features suite pass. The docker integration suite fails 16 tests both on this branch and on master (network creation and an amd64/arm64 mismatch on this machine) — pre-existing, verified by running the same spec on both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
BaseCommand.init() built a Dockerode client on every command. dockerode brings
ssh2 (592KB) and @grpc/grpc-js (652KB) with it for transports emb never uses,
and commands like `emb ps` shell out to docker compose without touching the
Engine API at all.
EmbContext.docker is now optional and populated on first use via
getDockerClient(). dockerode is CommonJS, so createRequire keeps the accessor
synchronous — no await, and the 17 call sites changed shape only.
emb ps 1125ms -> 1046ms
emb tasks 922ms -> 883ms
modules 1171 / 5.30MB -> 1004 / 3.53MB
Against the original baseline: emb ps 1683ms -> 1046ms (-38%), and 83% less
JavaScript parsed.
The plan called for a memoising getter on the context object, needing no
call-site changes. That is a trap worth recording: BaseCommand.init() does
setContext({ ...partialContext, monorepo }), and a spread invokes getters —
silently restoring the eager load. Any {...context} anywhere would do the same.
An explicit accessor cannot be defeated that way, and matches the Kubernetes
one.
Two type fixes fell out. retagIfNecessary/pushImage took
ReturnType<typeof getContext>['docker'], which became nullable — now typed
Docker directly. Five specs reading context.docker.x needed a non-null
assertion, matching existing test style.
678/678 unit tests and integration-features pass. `emb images` and
`emb containers` verified against the real daemon.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.