proposal: x/exp/mmap: implement io.WriterTo and io.ReadSeeker

[costela]

A naive implementation of io.WriterTo in mmap.ReaderAt seems to bring the following performance improvement when using code that relies on io.Copy (e.g. http.ServeContent).

name        old time/op    new time/op    delta
MmapCopy-8     126µs ± 7%     106µs ±16%  -16.09%  (p=0.000 n=8+10)

name        old speed      new speed      delta
MmapCopy-8  16.9GB/s ± 6%  20.4GB/s ±15%  +20.30%  (p=0.000 n=8+10)

name        old alloc/op   new alloc/op   delta
MmapCopy-8    33.7kB ± 0%     0.2kB ±14%  -99.40%  (p=0.000 n=9+10)

name        old allocs/op  new allocs/op  delta
MmapCopy-8      2.00 ± 0%      1.00 ± 0%  -50.00%  (p=0.000 n=10+10)

Benchmark code

func BenchmarkMmapCopy(b *testing.B) {
	// mmap some big-ish file; will only work on unix-like OSs.
	f, err := Open("/proc/self/exe")
	if err != nil {
		b.Fatalf("Open: %v", err)
	}

	type sectionReaderWithWriterTo struct {
		*io.SectionReader
		*ReaderAt
	}
	reader := &sectionReaderWithWriterTo{
		SectionReader: io.NewSectionReader(f, 0, int64(f.Len())),
		ReaderAt:      f,
	}

	// Sanity check: ensure we will run into the io.Copy optimization when using the NEW code above.
	var _ io.WriterTo = (*sectionReaderWithWriterTo)(nil)

	buf := &bytes.Buffer{}
	// "Hide" the ReaderFrom interface by wrapping the writer.
	// Otherwise we skew the results by optimizing the wrong side.
	writer := struct{ io.Writer }{buf}

	b.ReportAllocs()
	b.ResetTimer()

	for i := 0; i < b.N; i++ {
		_, _ = reader.Seek(0, io.SeekStart)
		buf.Reset()

		n, _ := io.Copy(writer, reader)
		b.SetBytes(n)
	}
}

This proposal would probably also solve #20642, as suggested.

Update 2022-10-27: since repeated uses of WriteTo() should behave like other readers (i.e.: keep state to avoid re-writing the whole mmap.ReaderAt), this proposal also implies an implementation of io.ReadSeeker.

[costela]

Is there maybe still a mistake in the benchmark that might be skewing the results?

[ianlancetaylor]

CC @nigeltao

[nigeltao]

Yeah, I'm OK with adding io.WriterTo, and since we'd now have a file position, adding io.ReadSeeker methods too.

[nigeltao]

performance improvement when using code that relies on io.Copy (e.g. http.ServeContent).

If you're talking about http.ServeContent, though, it may perform better to os.Open an os.File (because of the sendfile syscall) instead of to mmap.Open an mmap.ReaderAt (even with a WriteTo method). In which case there might be less motivation to change package mmap.

[costela]

@nigeltao

If you're talking about http.ServeContent, though, it may perform better to os.Open an os.File (because of the sendfile syscall) instead.

Absolutely, that's exactly what we're currently doing (plus some hacks to work around #16474). The problem with that is that we have multiple concurrent readers of large(-ish) files, in which case the overhead of multiple independent os.Open + sendfile turns out to be greater than (or least comparable to; difficult to measure reliably) a single mmap + sharing the []byte.

and since we'd now have a file position

I'm not sure that we must have a file position to implement io.WriterTo? Since the io.ReaderAt interface explicitly mentions the following:

If ReadAt is reading from an input source with a seek offset, ReadAt should not affect nor be affected by the underlying seek offset.

That reads to me like there is currently no way to move the the seek offset in mmap.ReaderAt, so WriteTo could always write from the beginning until io.EOF?

This doesn't mean we couldn't implement it, but AFAICT these two are orthogonal. What am I missing?

[nigeltao]

I don't think it's orthogonal. The io.WriterTo doc comment mentions io.Copy and io.Copy works with an io.Reader (not an io.ReaderAt) and I'd expect Read to advance the seek offset.

In any case, I started a golang-nuts discussion:
https://groups.google.com/g/golang-nuts/c/cHOwJJMZfyo

[nigeltao]

code that relies on io.Copy (e.g. http.ServeContent).

Separately, your benchmark code above uses io.Copy but if you're ultimately concerned about http.ServeContent performance, that calls io.CopyN, not io.Copy, and io.CopyN doesn't honor io.WriterTo. The WriteTo method cannot take the n argument passed to io.CopyN.

[costela]

if you're ultimately concerned about http.ServeContent performance, that calls io.CopyN, not io.Copy, and io.CopyN doesn't honor io.WriterTo

oh boy, you're right, of course 😞

In any case, I started a golang-nuts discussion

We can wait a bit for any counter-arguments, but yours and @ianlancetaylor's arguments make sense. I'll include it in the proposal.

[costela]

if you're ultimately concerned about http.ServeContent performance, that calls io.CopyN, not io.Copy, and io.CopyN doesn't honor io.WriterTo

oh, wait, not so sure: io.CopyN uses io.Copy+io.LimitReader. So AFAICT this change should still trickle down to http.ServeContent? 🤔

[nigeltao]

io.LimitReader wraps another io.Reader. The outer reader (the io.LimitReader) does not implement io.WriterTo even if the inner reader does. Hence, in combination, io.CopyN doesn't honor io.WriterTo.

See https://go.dev/play/p/LybHg59d4KU

I'll repeat: how would it work otherwise? The WriteTo method cannot take the n argument passed to io.CopyN.

[costela]

The WriteTo method cannot take the n argument passed to io.CopyN.

🤦‍♂️ yes, of course, sorry for the noise.

(feel free to mark the last couple of msgs as off-topic; they don't really contribute to the actual proposal discussion)

[nigeltao]

OK, I'll repeat that, for the actual proposal, I'm OK with adding io.WriterTo and io.ReadSeeker methods.

Still, if you're ultimately concerned about http.ServeContent performance, I don't think it's going to help you.

[gopherbot]

Change https://go.dev/cl/555796 mentions this issue: mmap: implement io.WriterTo and io.ReadSeeker

[costela]

After a regretable long absence, I finally came back to this.

Relatedly: #56480 made this relevant for http.ServeContent, but that has since been unfortunately reverted. I'll create a second CL attempt to reinstate that change, since significant work has been done on linux/splice since.

[sbinet]

wouldn't the io.ReadSeeker and io.Reader interfaces be available via composition with io.SectionReader ?
(that was the reply I got when I initially proposed to add these interfaces :})

see: #39683

[costela]

@sbinet true, but then we'd need something self-referential like this:

r := &ReaderAt{
	data: data,
}
r.SectionReader = io.NewSectionReader(r, 0, size)

and WriteTo gets a bit more error handling:

func (r *ReaderAt) WriteTo(w io.Writer) (int64, error) {
	pos, err := r.Seek(0, io.SeekCurrent)
	if err != nil {
		return 0, fmt.Errorf("mmap: WriteTo: %w", err)
	}
	n, err := w.Write(r.data[pos:])
	_, seekErr := r.Seek(int64(n), io.SeekCurrent)
	if seekErr != nil {
		return 0, fmt.Errorf("mmap: WriteTo: %w", err)
	}
	return int64(n), err
}

That didn't feel quite clean and I felt "a bit of copying is better than a bit dependency" (but applying that to the stdlib itself may be a bit too much 😅)

I'm totally ok going that way if that's preferred!

[sbinet]

I was just digging up the previous consensus about this issue :)
if the consensus changed, then go for it.

[costela]

@sbinet just changed the implementation to use io.SectionReader for all but the _other case, since that one is basically just a os.File passthrough anyway.

PTAL