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: make (*TCPConn).ReadFrom(r) use sendfile syscall when r is io.SectionReader of *os.File #6374

Closed
gopherbot opened this issue Sep 12, 2013 · 9 comments

Comments

Projects
None yet
3 participants
@gopherbot
Copy link

commented Sep 12, 2013

by valyala:

Currently net.TCPConn.ReadFrom() shortcuts to sendfile() syscall if the provided reader
implements os.File (possibly wrapped into io.LimitReader) - see
http://golang.org/src/pkg/net/sendfile_linux.go#L24 .

It would be great if it could shortcut to sendfile() syscall also if os.File is wrapped
into io.SectionReader. This will enable transparent sending arbitrary sections of the
same os.File object from multiple concurrent goroutines via sendfile().

See the following topic for details -
https://groups.google.com/d/msg/golang-nuts/mbnJjKLgYzU/vm4GHAQwfSsJ .
@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2013

Comment 1:

Not completely sure this is a good idea but it seems like it might be just a few lines.

Labels changed: added priority-later, go1.3, removed priority-triage.

Status changed to Accepted.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2013

Comment 2:

Labels changed: added release-go1.3.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2013

Comment 3:

Labels changed: removed go1.3.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2013

Comment 4:

Labels changed: added repo-main.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Mar 5, 2014

Comment 5:

Markus sent https://golang.org/cl/69870051/
@rsc

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2014

Comment 6:

My first summary was bogus. The new one captures the true complexity here, and also the
not-very-general nature of the proposed change. Let's postpone that CL until after Go
1.3 is out.

Labels changed: added release-go1.4, removed release-go1.3.

@gopherbot

This comment has been minimized.

Copy link
Author

commented Apr 6, 2014

Comment 7 by markus@distinctive.co:

So would the preference be to have a new type of reader, like FileSectionReader for
example, that would specifically address this network use-case rather than exposing more
of the SectionReader's internal state?
If there is question of the value of this feature, our specific case streams entries to
a file and then periodically ships file chunks to a remote server in a single process,
we have seen a noticeable increase in performance by deferring to the OS/sendfile under
strained workloads.
@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 6, 2014

Comment 8:

The goal is to have a consistent and simple API above all else. Performance is nice, but
not if it means adding new confusing types for rare cases. That means all readers of the
io package docs have the cognitive burden of seeing that new type and its docs and
understanding why they should or shouldn't use it.
If a new type is only for performance, and the performance is actually worth it in many
cases, and it's not something people can do on their own easily without forking lots of
code, then it's possible we could support it in more gross/magic ways.
Exposing io.LimitedReader (it used to be hidden) was already a bit of a compromise.
But now that LimitReader + sendfile works, it's unclear we need to make all the reader
wrapper types work with sendfile--- you can make a LimitReader now and use it with
ReadFrom.  If you made the SectionReader then you already have the *os.File.
@rsc

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2014

Comment 9:

Sounds like we don't need this.

Status changed to WontFix.

@rsc rsc added this to the Go1.4 milestone Apr 14, 2015

@rsc rsc removed the release-go1.4 label Apr 14, 2015

@golang golang locked and limited conversation to collaborators Jun 25, 2016

This issue was closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.