io: make NopCloser forward WriterTo calls to underlying reader #51566

Jorropo · 2022-03-09T16:08:12Z

Because I belive that is clearer with code than words:
Replace:

func NopCloser(r Reader) ReadCloser {
  return nopCloser{r}
}

With something like this:

func NopCloser(r Reader) ReadCloser {
  c := nopCloser{r}
  if to, ok := r.(WriterTo); ok {
    return struct{
      nopCloser
      WriterTo
    }{c, to}
  }
  return c
}

That allows an about ~1.5X faster read time when wrapping bytes.NewReader or bytes.Buffer in io.NopCloser (actually it turns it into a constant time operation, where io.Reader has a linear time):

package bytes_test

import (
	"bytes"
	"io"
	"testing"
)

func BenchmarkForward4096(b *testing.B){benchmarkForward(4096,b)}
func BenchmarkForward16384(b *testing.B){benchmarkForward(16384,b)}
func BenchmarkForward65536(b *testing.B){benchmarkForward(65536,b)}
func BenchmarkForward262144(b *testing.B){benchmarkForward(262144,b)}
func BenchmarkForward1048576(b *testing.B){benchmarkForward(1048576,b)}

func BenchmarkSimple4096(b *testing.B){benchmarkSimple(4096,b)}
func BenchmarkSimple16384(b *testing.B){benchmarkSimple(16384,b)}
func BenchmarkSimple65536(b *testing.B){benchmarkSimple(65536,b)}
func BenchmarkSimple262144(b *testing.B){benchmarkSimple(262144,b)}
func BenchmarkSimple1048576(b *testing.B){benchmarkSimple(1048576,b)}

var zeros = [1048576]byte{}

func benchmarkForward(i int, b *testing.B) {
	for n := 0; n < b.N; n++ {
		_, err := io.Copy(io.Discard, nopCloserForward(bytes.NewReader(zeros[:i])))
		c(err)
	}
}

func benchmarkSimple(i int, b *testing.B) {
	for n := 0; n < b.N; n++ {
		_, err := io.Copy(io.Discard, nopCloserSimple(bytes.NewReader(zeros[:i])))
		c(err)
	}
}

func c(err error) {
	if err != nil {
		panic(err)
	}
}

type nopCloser struct {
	io.Reader
}

func (_ nopCloser) Close() error {
	return nil
}

func nopCloserSimple(r io.Reader) io.ReadCloser {
  return nopCloser{r}
}

func nopCloserForward(r io.Reader) io.ReadCloser {
  c := nopCloser{r}
  if to, ok := r.(io.WriterTo); ok {
    return struct{
      nopCloser
      io.WriterTo
    }{c, to}
  }
  return c
}

goos: linux
goarch: amd64
cpu: AMD Ryzen 5 3600 6-Core Processor              
BenchmarkForward4096-12       	 9195832	       136.1 ns/op
BenchmarkForward16384-12      	 8926760	       133.7 ns/op
BenchmarkForward65536-12      	 8760045	       135.2 ns/op
BenchmarkForward262144-12     	 8769903	       130.6 ns/op
BenchmarkForward1048576-12    	 8559370	       131.6 ns/op
BenchmarkSimple4096-12        	 6630315	       179.9 ns/op
BenchmarkSimple16384-12       	 3842174	       318.7 ns/op
BenchmarkSimple65536-12       	 1377452	       855.4 ns/op
BenchmarkSimple262144-12      	  405866	      2820 ns/op
BenchmarkSimple1048576-12     	  109400	     11025 ns/op

As you can see this is a surprisingly popular patern: https://github.com/golang/go/search?q=NopCloser+bytes.NewReader

The only issue I see with this, is that pure io.Reader are now ~1.5x time slower to NopCloser-ify. (that the cost of trying the cast that would fail). Since most NopCloser are created with a known type, I would hope that NopCloser gets inlined and the type cast is solved at compile time, but it seems that not happening.

The text was updated successfully, but these errors were encountered:

rsc · 2022-03-16T17:51:45Z

It's hard to argue with the performance win here.
The implementation is unfortunate.
One possibility would be to implement #41198 and then make io.Copy understand an ErrUnsupported coming back from WriterTo. Then there could be still just a single nopCloser implementation, and it would return ErrUnsupported if the underlying reader did not have the method.

rsc · 2022-03-16T18:01:07Z

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

rsc · 2022-03-23T17:30:08Z

It seems like we should make this change, which would mean two different possible underlying types for the interface returned by NopCloser. And then later we can decide whether to make io.Copy support ErrUnsupported, which would get us back to one underlying type.

Jorropo · 2022-03-23T18:11:11Z

And then later we can decide whether to make io.Copy support ErrUnsupported, which would get us back to one underlying type.

I completely agree that this is a good idea.
However I think it will would have less of an impact than we can think.

Because we would still do the cast -> fail overhead when io.Copying, at least it's free for Read only based workloads.

rsc · 2022-03-30T18:13:29Z

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

rsc · 2022-04-13T18:00:19Z

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

andig · 2022-04-13T18:51:35Z

One possibility would be to implement #41198 and then make io.Copy understand an ErrUnsupported coming back from WriterTo.

Seems that approved proposal is getting nowhere. Use and io/fs/os ErrUnsupported instead?

Jorropo · 2022-04-13T20:02:57Z

Seems that approved proposal is getting nowhere. Use and io/fs/os ErrUnsupported instead?

@andig My current understanding is that we will implement this using reflection in io.NopCloser for now, and probably update to use ErrUnsupported once #41198 is implemented (if that is faster, idealy the compiler would inline and optimise the reflection away instead).

This patch also include related fixes to net/http's NopCloser unwrapping logic. The io_test.go don't bother to test actually reading or WritingTo of the NopCloser because the logic is simple. NopCloser didn't even had direct tests before. Fixes golang#51566

gopherbot · 2022-04-13T23:39:55Z

Change https://go.dev/cl/400236 mentions this issue: io: NopCloser forward WriterTo implementations if the reader supports it

This patch also include related fixes to net/http's NopCloser unwrapping logic. The io_test.go don't bother to test actually reading or WritingTo of the NopCloser because the logic is simple. NopCloser didn't even had direct tests before. Fixes golang#51566

The io_test.go don't bother to test actually reading or WritingTo of the NopCloser because the logic is simple. It didn't even had direct tests before. Fixes golang#51566

Jorropo added the Proposal label Mar 9, 2022

gopherbot added this to the Proposal milestone Mar 9, 2022

Jorropo changed the title ~~proposal: io: make NopCloser forward io.WriterTo too~~ proposal: io: make NopCloser forward io.WriterTo Mar 9, 2022

ianlancetaylor added this to Incoming in Proposals (old) Mar 9, 2022

seankhliao added the Performance label Mar 9, 2022

rsc changed the title ~~proposal: io: make NopCloser forward io.WriterTo~~ proposal: io: make NopCloser forward WriterTo calls to underlying reader Mar 16, 2022

rsc moved this from Incoming to Active in Proposals (old) Mar 16, 2022

rsc moved this from Active to Likely Accept in Proposals (old) Mar 30, 2022

rsc added the Proposal-FinalCommentPeriod label Mar 30, 2022

rsc moved this from Likely Accept to Accepted in Proposals (old) Apr 13, 2022

rsc changed the title ~~proposal: io: make NopCloser forward WriterTo calls to underlying reader~~ io: make NopCloser forward WriterTo calls to underlying reader Apr 13, 2022

rsc added the Proposal-Accepted label Apr 13, 2022

rsc modified the milestones: Proposal, Backlog Apr 13, 2022

Jorropo mentioned this issue Apr 13, 2022

io: NopCloser forward WriterTo implementations if the reader supports it #52340

Closed

dmitshur modified the milestones: Backlog, Go1.19 May 3, 2022

gopherbot closed this as completed in 0537a74 May 3, 2022

golang locked and limited conversation to collaborators May 3, 2023

gopherbot added the FrozenDueToAge label May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

io: make NopCloser forward WriterTo calls to underlying reader #51566

io: make NopCloser forward WriterTo calls to underlying reader #51566

Jorropo commented Mar 9, 2022

rsc commented Mar 16, 2022

rsc commented Mar 16, 2022

rsc commented Mar 23, 2022

Jorropo commented Mar 23, 2022

rsc commented Mar 30, 2022

rsc commented Apr 13, 2022

andig commented Apr 13, 2022

Jorropo commented Apr 13, 2022

gopherbot commented Apr 13, 2022

io: make NopCloser forward WriterTo calls to underlying reader #51566

io: make NopCloser forward WriterTo calls to underlying reader #51566

Comments

Jorropo commented Mar 9, 2022

rsc commented Mar 16, 2022

rsc commented Mar 16, 2022

rsc commented Mar 23, 2022

Jorropo commented Mar 23, 2022

rsc commented Mar 30, 2022

rsc commented Apr 13, 2022

andig commented Apr 13, 2022

Jorropo commented Apr 13, 2022

gopherbot commented Apr 13, 2022