Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: support zero-copy send on TCPConn when reading from File via SectionReader #61727

Open
CAFxX opened this issue Aug 3, 2023 · 10 comments
Open
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone

Comments

@CAFxX
Copy link
Contributor

CAFxX commented Aug 3, 2023

Currently net.TCPConn has dedicated support for this, but only for LimitedReader. Having dedicated support also for SectionReader would allow using the same FD concurrently to send the same file to multiple TCP connections, as well as supporting other use-cases (e.g. range requests).

This is obviously currently possible but without zero-copy support, resulting in the fallback genericReadFrom being used (that, in addition to requiring more syscalls, also requires more allocations):

image

I have a PoC for this on deea4f6

@dr2chase dr2chase added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance labels Aug 3, 2023
@dr2chase
Copy link
Contributor

dr2chase commented Aug 3, 2023

@neild @ianlancetaylor

@ianlancetaylor
Copy link
Contributor

In principle this sounds good but in practice I think this will require exporting some fields from io.SectionReader, or adding at least one new method. And that technically requires a proposal, as described at https://go.dev/s/proposal. Can you look into what an implementation would require? Thanks.

@CAFxX
Copy link
Contributor Author

CAFxX commented Aug 4, 2023

@ianlancetaylor would a linkname-d unexported method be acceptable? That would sidestep the proposal process... (that we can still do if we want, but at least without blocking this)

@ianlancetaylor
Copy link
Contributor

Honestly I don't think this problem is important enough or urgent enough to reach for go:linkname, which should be avoided whenever possible.

@CAFxX
Copy link
Contributor Author

CAFxX commented Aug 7, 2023

@ianlancetaylor personally I feel the urgency a bit more, but OK, understood.

I don't really have a strong opinion about which way to go for the proposal, and it's not like the rest of package io has relevant examples to copy from for something like this (of the options below, there seem to be a general tendency toward option 1, but it's weak), so maybe you can offer your perspective on which one would be more desirable/in line with expectations:

  1. Turn all fields (r, base, off, limit) into exported ones (R, Base, Off, Limit), explaining how to (not) use them
  2. Same as 1. but make Offset=0 mean that we're reading from the first byte of the section (in line with Seek(offset, io.SeekStart))
  3. Create an Unwrap method (can't find a good name for this one) that returns a copy of all four fields
  4. Create Parent() ReaderAt, Base() int64 methods (off and limit can be obtained with a few calls to Seek)
  5. Create Parent() ReaderAt, Base() int64, Size() int64 methods (the offset can be obtained with a call to Seek)
  6. Create Parent() ReaderAt, Base() int64, Size() int64, Offset() int64 methods
  7. Same as one of options 3 to 6, but somehow expose it via an internal/* package (likely a bit of an hack, but would sidestep the proposal)
  8. go:linkname shenanigans (even bigger hack, sidesteps the proposal: already excluded, just for completeness)

(all names above are placeholders, feel free to suggest different ones)

Will take your preference and prepare the proposal based on it.

@ianlancetaylor
Copy link
Contributor

I don't see a problem with exporting the r and base fields. And then as you say we can get the current offset and the final offset using Seek, so maybe that is enough.

@AlexanderYastrebov
Copy link
Contributor

@CAFxX Discussion in #61530 might be related and interesting to you.

@gopherbot
Copy link

Change https://go.dev/cl/526855 mentions this issue: io: add (*SectionReader).Outer()

CAFxX added a commit to CAFxX/go that referenced this issue Sep 8, 2023
Fixes: golang#61870
Updates: golang#61727

Change-Id: Iaef9b59c402d68f6bf64be212db2b6746abe8900
@CAFxX
Copy link
Contributor Author

CAFxX commented Sep 8, 2023

Early results for sending a 1MB file:

goos: linux
goarch: amd64
pkg: test
cpu: Intel(R) Xeon(R) CPU @ 2.20GHz
        │ master.txt  │              patch.txt              │
        │   sec/op    │   sec/op     vs base                │
Serve-4   466.5µ ± 3%   268.5µ ± 5%  -42.43% (p=0.000 n=10)

        │  master.txt   │               patch.txt               │
        │     B/op      │     B/op       vs base                │
Serve-4   52.331Ki ± 3%   8.423Ki ± 26%  -83.90% (p=0.000 n=10)

        │ master.txt  │              patch.txt              │
        │  allocs/op  │  allocs/op   vs base                │
Serve-4   152.00 ± 7%   87.50 ± 10%  -42.43% (p=0.000 n=10)

master...CAFxX:go:cafxx-sendfile-sectionreader

CAFxX added a commit to CAFxX/go that referenced this issue Sep 10, 2023
Fixes: golang#61870
Updates: golang#61727

Change-Id: Iaef9b59c402d68f6bf64be212db2b6746abe8900
CAFxX added a commit to CAFxX/go that referenced this issue Sep 11, 2023
goos: linux
goarch: amd64
pkg: test
cpu: Intel(R) Xeon(R) CPU @ 2.20GHz
        │ master.txt  │              patch.txt              │
        │   sec/op    │   sec/op     vs base                │
Serve-4   466.5µ ± 3%   268.5µ ± 5%  -42.43% (p=0.000 n=10)

        │  master.txt   │               patch.txt               │
        │     B/op      │     B/op       vs base                │
Serve-4   52.331Ki ± 3%   8.423Ki ± 26%  -83.90% (p=0.000 n=10)

        │ master.txt  │              patch.txt              │
        │  allocs/op  │  allocs/op   vs base                │
Serve-4   152.00 ± 7%   87.50 ± 10%  -42.43% (p=0.000 n=10)

Updates golang#61727

Change-Id: Iede44e82cf56af81d75f4c5a024cb8a6d3dc5116
CAFxX added a commit to CAFxX/go that referenced this issue Sep 11, 2023
DO NOT REVIEW
DO NOT SUBMIT

goos: linux
goarch: amd64
pkg: test
cpu: Intel(R) Xeon(R) CPU @ 2.20GHz
        │ master.txt  │              patch.txt              │
        │   sec/op    │   sec/op     vs base                │
Serve-4   466.5µ ± 3%   268.5µ ± 5%  -42.43% (p=0.000 n=10)

        │  master.txt   │               patch.txt               │
        │     B/op      │     B/op       vs base                │
Serve-4   52.331Ki ± 3%   8.423Ki ± 26%  -83.90% (p=0.000 n=10)

        │ master.txt  │              patch.txt              │
        │  allocs/op  │  allocs/op   vs base                │
Serve-4   152.00 ± 7%   87.50 ± 10%  -42.43% (p=0.000 n=10)

Updates golang#61727

Change-Id: Iede44e82cf56af81d75f4c5a024cb8a6d3dc5116
@gopherbot
Copy link

Change https://go.dev/cl/527215 mentions this issue: net: support sendfile for SectionReader (linux)

@dmitshur dmitshur added this to the Backlog milestone Oct 9, 2023
gopherbot pushed a commit that referenced this issue Oct 9, 2023
Fixes #61870
Updates #61727

Change-Id: Iaef9b59c402d68f6bf64be212db2b6746abe8900
Reviewed-on: https://go-review.googlesource.com/c/go/+/526855
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
yunginnanet pushed a commit to yunginnanet/go that referenced this issue Oct 20, 2023
Fixes golang#61870
Updates golang#61727

Change-Id: Iaef9b59c402d68f6bf64be212db2b6746abe8900
Reviewed-on: https://go-review.googlesource.com/c/go/+/526855
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Development

No branches or pull requests

6 participants