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: Buffers makes multiple Write calls on Writers that don't implement buffersWriter #21676

Open
zombiezen opened this issue Aug 29, 2017 · 20 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@zombiezen
Copy link
Contributor

The writev syscall is supposed to act like a single write. The WriteTo method of net.Buffers will make a single write syscall on writers that have the unexported writeBuffers method. However, for writers that do not have such a method, it will call Write multiple times. This becomes significant if you are wrapping a *net.TCPConn without embedding, for instance, since it has different performance characteristics with respect to Nagle's algorithm. Frustratingly, since the writeBuffers method is unexported, there's no way for the application to know the behavior of Buffers.WriteTo in order to work around the issue.

Repro case: https://play.golang.org/p/rF0JRZs8z8

@odeke-em
Copy link
Member

/cc @mikioh @ianlancetaylor @rsc

@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Aug 29, 2017
@ianlancetaylor
Copy link
Contributor

This is an interesting one. I think it's somewhat clear that calling the Write method should turn into a single write system call for low-level types. But this is the WriteTo method, and I don't think there has ever been such a guarantee for WriteTo. In general WriteTo writes all available data to the Writer argument, which can imply fetching more data. For example, the more-or-less canonical implementation of WriteTo, bufio.(*Reader).WriteTo, makes multiple Write calls. Similarly, the original implementation of WriteTo, in https://golang.org/cl/166041, used multiple Write calls.

But clearly for a low-level type it is desirable to minimize write calls, and when using net.Buffers it's inconvenient to not know whether you will get one Write call or several. So there does seem to be an argument that net.(*Buffers).WriteTo should copy the bytes and call Write once. But there is also a counter-argument that the whole point of net.Buffers is to avoid copying bytes, and so doing a copy anyhow seems like a bit of a trick.

@gobwas
Copy link

gobwas commented Sep 5, 2017

@ianlancetaylor hi! I think the problem is not in the WriteTo method, but in the writeBuffers. The point is that when some public function wants to cast its interface argument to some type that provides more efficient method to do same thing, that type and its method must be exported. I thing the good example of this is io.Copy function, that makes type assertion to io.WriterTo and io.ReaderFrom. net.Buffers.WriteTo does the similar thing, but with non-exported interface. This could bring some problems – I've tried to show them in duplicate of this issue. TL;DR: it is all about wrappers and method overloading.

@ianlancetaylor
Copy link
Contributor

@gobwas You are describing a general problem that Go code can run into, but as far as I can see this particular problem will not be helped by exporting the writeBuffers method. That will let the calling code see whether it gets a single write or not, but it doesn't solve the basic question of whether we should always be using a single write. Unless, I suppose, we want to restrict this problem to make it possible to wrap a net.TCPConn while still getting a single write.

@zombiezen
Copy link
Contributor Author

zombiezen commented Sep 8, 2017

I understand the concern. I think that users of the API want to make the choice: not allocating might be important or not issuing more than one write might be important, depending on context. Exporting the interface would be a means of allowing the caller to use a type-assertion to determine capabilities of the io.Writer, much like other I/O capabilities.

Could we perhaps introduce a general I/O interface (consider this a sketch, nothing more):

// Writever wraps the Writev method.
//
// Writev behaves identically to a Write where all of the byte slices in p are concatenated together.
type Writever interface {
  Writev(p [][]byte) (n int, err error)
}

Then in usage:

func singleWritev(w io.Writer, p [][]byte) (n int, err error) {
  if wv, ok := w.(Writever); ok {
    return wv.Writev(p)
  }
  pp := bytes.Join(p, nil)
  return w.Write(pp)
}

func noallocWritev(w io.Writer, p [][]byte) (n int, err error) {
  if wv, ok := w.(Writever); ok {
    return wv.Writev(p)
  }
  for _, pp := range p {
    nn, err := w.Write(pp)
    n += nn
    if err != nil {
      return n, err
    }
  }
  return n, nil
}

I could see this also being done as an alternate method on net.Buffers, but I still don't see why net.Buffers restricts to particular types, instead of allowing any type that implements the Writev semantics to benefit.

@ianlancetaylor ianlancetaylor added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 8, 2017
@rsc
Copy link
Contributor

rsc commented Oct 23, 2017

Wait, does anyone use Nagle's algorithm anymore? I thought we turned that off on all our file descriptors.

@zombiezen
Copy link
Contributor Author

@rsc It's off by default, but could be enabled by calling *TCPConn.SetNoDelay. It's not the only case where minimizing the number of Write calls is useful though.

@rsc
Copy link
Contributor

rsc commented Oct 30, 2017

I think it's probably too late for this as an API change in Go 1.10. Let's leave this for Go 1.11 and be able to discuss with @bradfitz. I think maybe a more compelling motivation than a custom TCP wrapper would be letting os.File implementations get the writev optimization too.

@rsc rsc modified the milestones: Go1.10, Go1.11 Oct 30, 2017
@odeke-em
Copy link
Member

How's it going @bradfitz? I am just pinging you here as per @rsc's last comment :)

@ianlancetaylor
Copy link
Contributor

See also #21756.

@egorse
Copy link

egorse commented May 11, 2018

Step into this problem. net.Pipe impossible to use for tests with net.Buffers where there are multiple writers and single reader. Wire data became interleaved :(

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jun 27, 2018
@rsc rsc modified the milestones: Go1.12, Go1.13 Nov 14, 2018
@rsc rsc added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Nov 14, 2018
@rsc
Copy link
Contributor

rsc commented Nov 14, 2018

For Go 1.13 we should look at this earlier in the cycle. The part about os.File maybe implementing this suggests that if we do expose an API it should not refer to types in package net. Perhaps just [][]byte directly.

@noblehng
Copy link

noblehng commented Dec 13, 2018

I think the bigger problem here is for datagram sockets.

The underlying system call guarantee a writev call will generate a single datagram, so (*net.Buffers).WriteTo will have different behavior when using bare *net.UDPConn vs wrapped, that is single datagram vs multiple.

Edit:
Even for stream sockets, it is not atomic as the underlying system call does. If there are concurrent writers, the result will be interleaved.

As for os.File usage, I think the Buffers type and related interfaces should be in the io package, os.File should also implement syscall.Conn interface.

The current net.Buffers is also a little hard for reuse, because it modify the slice directly, caller have to keep a own copy for reuse. Use bare [][]byte and let caller do the consume tracking, or use a ring buffer could be easier and less copying.

Edit2:
Forget about the implement syscall.Conn interface part. I am thinking about wiring to the runtime poller, but it doesn't register to the runtime poller to begin with, except for those sockets already in net.conn.

@Merovius
Copy link
Contributor

As another voice: I'm particularly interested in what @rsc mentioned - having Writev for *os.File. My use-case is a write-ahead log, which adds a header and footer to some user-provided data. File is opened with O_APPEND, so there is a semantic difference between one and multiple writes. Currently I allocate a buffer and copy everything, but I'd like to avoid that.

Personally I like the interface @zombiezen wrote down above and I would put it in io. This would be akin to io.StringWriter/WriterTo/WriterAt/… where the io package defines multiple interfaces to make an io.Writer more efficient in some circumstances and then also offers functions like io.WriteString with the natural fallbacks.

@seebs
Copy link
Contributor

seebs commented Dec 18, 2019

I would love to have access to readv and writev from Go, but I would point out that there's no way to guarantee those semantics generically across arbitrary operating systems. But the ability to get those atomic writes from multiple buffers is a HUGE performance win for a lot of applications.

@ianlancetaylor
Copy link
Contributor

Although this issue only has 15 comments they all seem to head in different directions. I think that if the request is a Writev method for *os.File, perhaps only available on systems that support writev, then we should open a new proposal issue for that.

@tv42
Copy link

tv42 commented Dec 19, 2019

I'm not convinced there's any scenario in which concurrent writers without holding a mutex is safe. The write(2) syscall can make short writes on e.g. interrupts, and the Go Write implementation will need a for loop around that -> concurrent Writes can interleave anyway.

Writev matters for datagrams and performance.

@gobwas
Copy link

gobwas commented Mar 3, 2020

It didn't shipped with Go 1.14, right? Any plans to include that Writever interface by @zombiezen ?

@stokito
Copy link

stokito commented Jan 8, 2022

@tv42 "I'm not convinced there's any scenario in which concurrent writers without holding a mutex is safe. The write(2) syscall can make short writes on e.g. interrupts"
I'm not an expert but it looks like the writev is atomic by design
https://en.wikipedia.org/wiki/Vectored_I/O

@Merovius
Copy link
Contributor

Merovius commented Jan 8, 2022

@stokito ISTM that the meaning of "atomic" is under-specified there. Both the text in the wikipedia and the information from the manpage seem to suggest that it doesn't mean "either all writes succeed or none of them", but just that writes by different processes are not interleaved. i.e. it seems it refers to the isolation of ACID, not the atomicity. Also, from what I can tell, the actual POSIX standard does not guarantee even that, unless writes are less than PIPE_BUF in size ([1] [2]).

In my experience, interpreting what guarantees the POSIX standard really makes is very subtle. And what is actually implemented even more so. Personally, I got convinced by the argument that short writes can happen, at least for my usecase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests