Skip to content

SerialMonitor.read_lines takes &mut self, so a write cannot overlap a read — costs ~220 ms per RPC in callers #1431

Description

@zackees

SerialMonitor.read_lines takes &mut self while write takes &self, so a caller cannot issue a write while a read is in flight. Callers that drive request/reply traffic therefore have to serialise the two, and each request ends up waiting out the tail of the previous reply's read before it can even be written.

Measured cost

FastLED's adapter offloads both calls to a single-worker ThreadPoolExecutor for exactly this reason, and caps each inner read_lines so the producer re-checks its cancel flag. Instrumenting a ping round trip against an RP2350W:

[trace] write took   8.5ms     <- first call of the session, no producer in flight
[trace] write took 321.0ms
[trace] write took 321.0ms
[trace] write took 320.9ms
n=4  median=330.6ms

The write is ~91% of the latency, and the first call of each session is fast because there is no read to queue behind — the signature of queueing rather than of waiting on the device. Dropping the inner cap 0.25 → 0.05 takes the write to ~100 ms and the round trip to ~110 ms, which is the mitigation FastLED is landing (FastLED#4343). Both points fit write ≈ 1.1 × cap + ~45 ms.

A ping is ~4 ms of wire time, and read_lines' own submit -> first-item is a steady 51 ms regardless of cap, so none of this is transport or device cost.

Why the caller cannot fix it

Raising the executor to two workers so writes stop queueing breaks the transport — the session stops answering its liveness probe:

RpcError: Connected to /dev/ttyACM0 but it never answered a liveness probe (ping, 5s),
or the request could not be written.

I want to be careful about the mechanism: the signatures predict a concurrent write during a read should be rejected outright, but what I observed was a dead probe rather than an Already borrowed, and I did not separate pyo3 borrow semantics from monitor state shared across threads. The empirical result is solid; the explanation is not mine to assert.

Suggested direction

read_lines_inner(&self, timeout) already exists in crates/fbuild-python/src/serial_monitor.rs, commented "for write_json_rpc which has &self" — so an &self read path is already viable, and the shared state it touches is behind a Mutex (ws_read.lock()) rather than depending on the &mut. If the public read_lines could take &self as well, a write would not have to wait for a read to finish, and callers could stop trading latency against cancellation responsiveness via a polling cap.

I have not attempted the change — whatever the public read_lines mutates beyond the shared read half (hook dispatch, last_line, pending buffers) needs an owner's judgement on which of those genuinely need exclusivity.

Environment

  • fbuild 2.5.23, Linux/NixOS
  • RP2350W 2DCB876B587EA334 over the fbuild serial monitor
  • Measurements: 10–20 ping calls per data point, medians quoted

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions