perf: read the pty in 64 KB chunks instead of 1 KB - #25
Open
lordspace wants to merge 1 commit into
Open
Conversation
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
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.
Fixes #24
What this changes
read_loopreads with a 1 KB stack buffer and posts one Dart port message perread, on both the POSIX and the Windows reader. This raises the buffer to 64 KB
behind a named
PTY_READ_BUFFER_SIZE, in both files.Two lines of behaviour change, no API change.
Why
One read is one port message, and the message is what costs — a typed-data
allocation, a stream event, Future propagation on the Dart side, and the GC
after it. At 1 KB, 15 MB of program output becomes ~15,000 messages, and the UI
isolate spends its time on message machinery rather than on the terminal.
Measured in a real Flutter terminal app (Linux, release build,
find /usr=15.2 MB), timing how long the app stays busy after the command finishes —
i.e. how long the UI is unresponsive:
~5.8x faster, and the difference between a terminal that stalls on a noisy
build and one that does not.
Rendering was ruled out before touching this: 7.6x the terminal cells cost only
12% more time, so the cost is per-byte, not per-cell. A CPU profile agreed —
the hot leaves were typed-data allocation, GC (
__munmap), and_propagateToListeners/_scheduleMicrotask, i.e. the per-message path ratherthan parsing or painting.
Things a reviewer would reasonably ask
Does this add latency to interactive typing? No.
read()returns as soon asany data is available and never waits to fill the buffer, so an echoed keystroke
still arrives in one small read. The buffer size caps a read; it does not set one.
Is 64 KB on the stack safe? It is a stack array on the dedicated reader
thread, against a default 8 MB thread stack. If you would rather not spend 64 KB
of frame, a smaller value captures much of the win, or it can move to a single
mallocper reader — happy to change it either way.Why both files? The Windows reader carries the same 1 KB buffer, so a
POSIX-only change would leave Windows on the slow path.
Why not flow control instead? The existing
ackReadpath is untouched andstill works exactly as before; this is orthogonal and helps whether or not a
consumer opts into acking.