feat(conn): add SetWriteDeadline to bound writes without allocating - #570
Closed
sztanpet wants to merge 1 commit into
Closed
feat(conn): add SetWriteDeadline to bound writes without allocating#570sztanpet wants to merge 1 commit into
sztanpet wants to merge 1 commit into
Conversation
Bounding a write requires a context, so a caller that wants a write deadline must allocate one per frame — a context.WithTimeout on its side and, for any cancelable context, a context.AfterFunc on ours. A server fanning one message out to many connections pays that per recipient rather than per message, where it is the dominant cost of sending: profiling a chat server holding 500 connections in one room at ~49,600 message deliveries a second attributed about 700 bytes of garbage per delivered message to it, and 99% of everything the server allocated under that load. A deadline is an instant, and an instant needs no allocation to express. SetWriteDeadline stores one in an atomic and writeFrame arms a timer created once per connection, using only Reset and Stop — the approach netconn.go already takes for net.Conn deadlines, exposed on Conn so a caller that can name its bound as a deadline does not have to buy a context to carry it. Paired with a non-cancelable context on Write, which coder#566 already made free, the write path allocates nothing. Existing callers are unaffected: the deadline is unset by default, and with none set writeFrame does one atomic load more than before. On the same server and load, replacing the per-frame context with a deadline: allocation over a 20s window falls from 689MB to zero sampled, CPU samples for identical delivered work from 14.33s to 12.50s, write(2) from 63.1% to 73.1% of the profile, runtime.mallocgc from 4.82% to below the reporting threshold, and mean delivery latency measured at the client from 4.20ms to 3.53ms. Two details worth noting for review. The timer is armed by writeFrame rather than by SetWriteDeadline, so that it is armed under writeFrameMu together with the frame it bounds and cannot disturb a frame already in flight. And armWriteDeadline must not close the connection itself: close takes writeFrameMu for a client connection via msgWriter.close, so closing inline deadlocks, which is why an expired deadline refuses the frame with os.ErrDeadlineExceeded and leaves the closing to the timer's goroutine. It reports bools rather than returning a stop closure for the reason coder#565 gives on the read path — a closure over the connection is an allocation per frame. ws_js.go gets the same method so the API does not differ by platform. Writes there are handed to the browser and never block, so the deadline is checked when a write is attempted rather than interrupting one. Refs coder#565, coder#566
Member
|
Closing as we don't have the capacity to review fully AI generated PRs in this repository at this time. Our guidelines need to be updated but some level of human involvement must be demonstrated, and the PR description written by a human demonstrating they understand the issue and solution, as well as having self-reviewed the full set of changes. Also, please try to avoid LLM minted terminology/jargon like "wedge" and "arm", to name two I happened to spot at a glance. If you want to work on this more, feel free to re-open once you've made amendments, or open a new one. |
Author
|
sure, will rewrite soon, thanks |
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.
A server writing to many connections needs every write bounded, so that one
client which has stopped reading cannot wedge the goroutine writing to it.
The only way to express that bound today is a context, and a context costs an
allocation per frame — which, for one message fanned out to a room, is an
allocation per recipient. This adds a way to say the same thing with a
deadline, which costs nothing.
Concretely:
Writetakes a context, so the bound is acontext.WithTimeoutin the caller plus, for any cancelable context, a
context.AfterFunchere.Profiling a chat server at ~49,600 deliveries/s across 500 connections put
that at about 700 bytes of garbage per delivered message, none of it the
message, and 99% of everything the server allocated.
A deadline is an instant, and an instant needs no allocation to express:
It stores the instant in an
atomic.Int64, andwriteFramearms a*time.Timercreated once per connection, using onlyResetandStop—netconn.goalready makes stopped timers this way fornet.Conndeadlines.Combined with a non-cancelable context on
Write, which #566 made free, thewrite path allocates nothing.
It is opt-in and fully backwards compatible. Nothing in the library calls
SetWriteDeadline; no existing signature, default or behaviour changes; andwith no deadline set
writeFramedoes one atomic load more than before.Callers that never touch it cannot tell the difference.
An expired deadline closes the connection, as an expired context does, and
refuses that frame with
os.ErrDeadlineExceeded. A zerotclears it. Thetimer is armed under
writeFrameMuso that setting a deadline cannot disturba frame in flight; it is also why an expired deadline cannot close inline and
leaves that to the timer's goroutine, since
closetakes the same lock viamsgWriter.close.Evidence
Same server and load, per-frame context replaced by a deadline, measured at
9f84ce4. Latency is measured at the client, the rest is pprof on theserver, built without
-race.write(2)share of CPU12% less CPU for identical delivered work and 15% off mean latency. Mean
latency held 3.53-3.55 ms across four deadline runs where every baseline run
of this load sat at 3.75-4.24 ms; the bands do not overlap.
TestSetWriteDeadlineAllocsasserts the allocation claim in the suiteitself, via
testing.AllocsPerRun.Tradeoff
The deadline belongs to the connection rather than to a call, so where
several goroutines write to one connection the last to set it wins. Frames
written under one deadline share that instant instead of restarting the
clock, which is usually what a caller batching frames into one wakeup wants.
It does not arise for callers whose writers share a single write timeout, but
it is a real narrowing against a per-call context.
Alternatives considered
Writecalls. Measured: it removesthe per-frame cost when a caller batches, nothing when each wakeup carries
one frame, and it must keep the deadline armed past the last frame.
Also free, but it aborts mid-frame and leaves the stream at an unknown
offset where this closes cleanly.
Not in this PR
SetReadDeadline. The symmetric change wants its own thought aboutCloseReadand ping handling.net.ConnonConn. This is one method, not a step toward that;NetConnstays the way to get it.Refs #565, #566