Repository navigation
peerconn: reuse chunk pool buffers for peer request data - #1082
AskAlexSharov wants to merge 7 commits into
Conversation
readPeerRequestData allocated r.Length bytes for every served peer request. Take the buffer from the Torrent chunk pool instead when it fits, and return it once the message writer has copied the piece data into its write buffer. Adds BenchmarkServePeerRequests: 17227 -> 852 B/op, 3.28us -> 1.84us per request.
5eb9721 to
9ea4239
Compare
…eTorrentInputNoPanic addTrackers mutates client-wide announce dispatcher state and must be called with the client lock held, like Torrent.AddTrackers does. The test called it unlocked, racing the dispatcher timer goroutine, which reads pendingTorrentInputUpdates in step(). go test -race -count 200 -run TestV2InfohashUpdateTorrentInputNoPanic: 9 races before, 0 after.
|
Interesting. I think I optimized the chunk allocation recently but it must have only been for the read path. I think the issue with using the pool on the seeding/write path is we can't predict the remote peer will request consistently sized chunks, or chunks that match our preferred read chunk size. I'll have to read more carefully. I'll update the branch to catch fixes that went in already too. |
There was a problem hiding this comment.
🟡 Changes recommended
Undersized requests can exceed the configured per-connection memory limit, and cleanup paths lack regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Reuses torrent chunk-pool buffers when serving peer requests to reduce upload allocations.
Changes:
- Adds pooled peer-request buffer lifecycle management.
- Releases buffers after sending, cancellation, errors, and connection closure.
- Adds an upload benchmark.
File summaries
| File | Description |
|---|---|
peerconn.go |
Implements pooled request-data ownership and cleanup. |
peerconn_test.go |
Adds upload storage fixture and benchmark. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: anacrolix <988750+anacrolix@users.noreply.github.com>
Co-authored-by: anacrolix <988750+anacrolix@users.noreply.github.com>
Charging pooled buffers at chunk capacity rejected peers that request smaller chunks than ours when MaxAllocPeerRequestDataPerConn is small (TestPeerConnEstablished hung).
readPeerRequestDataallocatedr.Lengthbytes for every served peer request, so seeding allocated ~16KiB per chunk uploaded. It now takes the buffer from theTorrentchunk pool (already used by the wire decoder) when the request is exactly one chunk, so pooled capacity equals the length charged againstMaxAllocPeerRequestDataPerConn, and returns it once the message writer has copied the piece data into its write buffer. Requests of any other size (peers do not always use our chunk size) still allocate.Release points: read error, request no longer wanted,
deleteReadyPeerRequest,deleteAllPeerRequests, and aftermsg()insendChunk.sendChunkuses a newtakeReadyPeerRequest(delete without release) so it can't free the buffer before the copy.PeerConn.onClosenow callsdeleteAllPeerRequestsso buffers go back to the pool andtorrentChunkBuffersOutstandingstays honest.BenchmarkServePeerRequests(new, drivesonReadRequest->servePeerRequest->sendChunk), n=8:-raceis clean on the root package,./test/...and./tests/...; a temporary check showedtorrentChunkBuffersOutstanding == 0after real seed/leech transfers.