Repository navigation
Conversation
5b00d8a to
286024f
Compare
Summary: When output was written outside of `Terminal` (e.g. `console.log`) in a burst, a status update could land between the burst's lines. In a TTY it can't be erased there, so copies of the status were left scattered in the output. - Before writing the status, `Terminal` waits until no output is written outside of it for 333ms (`EXTERNAL_WRITES_QUIET_MS`), in interactive and non-interactive terminals. Outside output is detected through the stream's `bytesWritten`. Without outside output, the status is written synchronously. `flush()` skips the wait. - Status writes wait on a delay promise set after each write: 33ms in a TTY, 3500ms otherwise. `flush()` ends the delay early. - Status delay timers are unreferenced, so they don't keep the process alive. An ongoing status update is flushed on `beforeExit`, so the latest status is still written and `waitForUpdates()` resolves. - `lodash.throttle` is removed from `metro-core` and `metro`: - `Terminal` limits non-TTY status lines through the same status delay. - `TerminalReporter` updates bundle progress on each `bundle_transform_progressed` event; `Terminal` limits the redraws. The `bundle_transform_progressed_throttled` event type is removed from `TerminalReportableEvent`. - The status is erased with ANSI escape sequences written through `Terminal`'s own write, so every write tracks its completion and the bytes written after it. `node:readline` is no longer used. - `isUpdating` and `hasPendingUpdate` are removed: an update is ongoing while `#updatePromise` is set, and pending while the written status differs from the latest one. `metro build` no longer waits for status timers before exiting: on a generated app of 5,000 modules whose build logs 3 lines per transformed module, wall time drops by 28% in prod and 37% in dev (about 2-3s, the trailing `lodash.throttle` timer on `main`), with no significant CPU change and byte-identical bundles. Changelog: [Breaking] Terminal no longer prints the status in the middle of bursts of output written outside of it, and `bundle_transform_progressed_throttled` is removed from `TerminalReportableEvent` Test Plan: - New test "writes the status after a burst of external writes" fails before this change, with a 33ms quiet period, and without non-TTY outside output detection, and passes after. - New test "flush does not wait for the next status update" fails when `flush()` doesn't end the status delay. - New test "flushes the pending status before the process exits" fails without the `beforeExit` listener. - New `TerminalReporter` test "shows the latest bundle transform progress" fails with the throttled reporter. - `Terminal-test.js` uses fake timers. - `yarn jest packages/metro-core packages/metro/src/lib` - `yarn flow check`, `yarn eslint packages/metro-core/src packages/metro/src/lib`, `yarn verify-api-snapshots` - `node scripts/benchmark-patches/benchmark-patches.js bundling_speed --runs=10`
286024f to
bd0c69d
Compare
Metro review: Changes suggestedRisk: medium · Commit: This PR holds Findings
Verified
Benchmarks (base vs this PR)
Method
Not run: No real TTY: running under a pty with Automated review by Claude. Advisory only: it never approves or blocks, and a maintainer makes the call. |
| return undefined; | ||
| } | ||
|
|
||
| return this.#delayStatus(EXTERNAL_WRITES_QUIET_MS).then(() => |
There was a problem hiding this comment.
should fix · The quiet wait has no upper bound, so sustained outside output hides the status indefinitely
#waitForExternalWritesToStop keeps re-arming the 333ms delay for as long as the byte count changes, with no ceiling. Any outside writer that logs more often than every 333ms therefore blocks all status updates until it stops or flush() is called.
Failure case: a TTY, with one outside line every 200ms and bundling progress updating alongside it. On base, the progress status is written throughout. On head, no progress is written at all during the 60s simulated run, so the terminal looks hung. The PR's own benchmark patch has this shape: it logs 3 lines per transformed module, so a build with that kind of logging shows no progress bar until the output pauses. On a long build, or with a middleware or plugin that logs steadily, that can last the whole session. unstable_server_menu_updated statuses go through the same path. A plain waitForUpdates() caller also stays blocked for that time, while flush() does not.
Suggested fix: cap the total wait, for example by writing the status anyway once it has been held for a few seconds. A stranded copy now and then is a better failure than no progress at all. Add a test with outside writes that never stop.
Reproduced: Repro packages/metro-core/src/__tests__/TerminalReviewRepro-test.js, 'sustained external writes': head fails with Expected: > 0, Received: 0 progress writes over 300 writes at 200ms intervals; the same scenario passes on base. A real process on a pipe (repro/scenario.js sustained) printed no progress line on head across 25 updates over 5s; base printed one. · id 5a6c4169
| } | ||
|
|
||
| return this.#delayStatus(EXTERNAL_WRITES_QUIET_MS).then(() => | ||
| this.#waitForExternalWritesToStop(currentBytesWritten), |
There was a problem hiding this comment.
should fix · Terminal's own writes count as outside output once the quiet wait has started
The first check compares against #bytesWrittenAfterLastWrite, which excludes Terminal's own writes. The recursive call instead compares against the currentBytesWritten captured before the delay, and that total includes anything terminal.log wrote during the delay. Once one outside write has started the wait, Terminal's own logs keep extending it, which contradicts the method's doc comment and the PR description ("no output is written outside of it").
Failure case: a TTY, one console.log line, then terminal.log every 100ms, as with client_log or worker_stdout_chunk traffic during a build. No status is written for the whole 10s run. The control without the outside line shows the status normally, so the single line is what hides it.
Suggested fix: measure outside bytes separately from Terminal's own. For example, in #write, add getBytesWritten() - #bytesWrittenAfterLastWrite to an #externalBytesWritten counter before writing, and have the wait compare that counter.
Reproduced: Repro TerminalReviewRepro-test.js, "Terminal's own logs don't extend the external quiet period": head fails with Expected: > 0, Received: 0, the control 'own logs alone' passes on head, and the same scenario passes on base. · id b4814541
| break; | ||
|
|
||
| case 'bundle_transform_progressed_throttled': | ||
| case 'bundle_transform_progressed': |
There was a problem hiding this comment.
nit · Expo CLI's bundling progress events depend on the removed throttled event
The [Breaking] changelog entry covers the type removal, but a known consumer depends on the runtime event too. @expo/cli@58.1.19 subclasses TerminalReporter, and its MetroTerminalReporter.#captureLog only updates its bundle spans and emits bundling:progress on bundle_transform_progressed_throttled. Metro no longer emits that event, so once Expo moves to this Metro version, those events stop without any error.
Nothing breaks today, because @expo/metro@latest pins metro 0.87.1, and the status bar itself should keep working through the base class. Subclass _log overrides also now run on every progress event instead of at most every 100ms.
Suggested fix: give the Expo maintainers a heads-up before this ships, or say in the changelog that subclasses should handle bundle_transform_progressed instead.
Evidence: Read build/src/start/server/metro/MetroTerminalReporter.js:364 and TerminalReporter.js:87 in the @expo/cli@58.1.19 tarball from npm, and checked the pin with npm view @expo/metro@latest dependencies. Not run against this PR's Metro. · id 2c40d144
Summary
When output is written outside of
Terminal(e.g.console.log) in a burst, a status update can land between the burst's lines. In a TTY it can't be erased there, so copies of the status are left scattered in the output.Terminalwaits until no output is written outside of it for 333ms (EXTERNAL_WRITES_QUIET_MS), in interactive and non-interactive terminals. Outside output is detected through the stream'sbytesWritten. Without outside output, the status is written synchronously.flush()skips the wait.flush()ends the delay early.beforeExit, so the latest status is still written andwaitForUpdates()resolves.lodash.throttleis removed frommetro-coreandmetro:Terminallimits non-TTY status lines through the same status delay.TerminalReporterupdates bundle progress on eachbundle_transform_progressedevent;Terminallimits the redraws. Thebundle_transform_progressed_throttledevent type is removed fromTerminalReportableEvent.Terminal's own write, so every write tracks its completion and the bytes written after it.node:readlineis no longer used.isUpdatingandhasPendingUpdateare removed: an update is ongoing while#updatePromiseis set, and pending while the written status differs from the latest one.The performance impact was measured with the benchmark in #2171, which builds a generated app and supports a base patch applied to every variant (results below).
Changelog: [Breaking] Terminal no longer prints the status in the middle of bursts of output written outside of it, and
bundle_transform_progressed_throttledis removed fromTerminalReportableEventTest plan
flush()doesn't end the status delay.beforeExitlistener.TerminalReportertest "shows the latest bundle transform progress" fails with the throttled reporter.Terminal-test.jsuses fake timers.yarn jest packages/metro-core packages/metro/src/libyarn flow check,yarn eslint packages/metro-core/src packages/metro/src/lib,yarn verify-api-snapshotsBenchmark
node scripts/benchmark-patches/benchmark-patches.js bundling_speed --runs=10, with abase.patchthat makesmetro buildlog 3 lines per transformed module (30,000 lines per build, interleaved with progress updates), and this PR'sTerminal.jsandTerminalReporter.jschanges as the variant patch. Measured on 94cc963 (these files are unchanged on the base of this PR):Values are mean ± sample sd over 10 runs per variant. t is Welch's t vs unpatched.
The saving is a roughly flat 2-3s per build, not faster bundling: the process no longer waits for status timers before exiting (on
main, the trailinglodash.throttletimer, up to 3.5s after the last status line in non-interactive output). The generated app builds in about 5s, so the percentages are large. Longer builds, like real apps, save about the same seconds, which is a much smaller share of their wall time. CPU time doesn't change significantly, and bundles are byte-identical.base.patch