net: Buffers makes multiple Write calls on Writers that don't implement buffersWriter

[zombiezen]

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]

/cc @mikioh @ianlancetaylor @rsc

[ianlancetaylor]

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]

@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]

@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]

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.

[rsc]

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

[zombiezen]

@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]

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.