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

CC @nigeltao

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

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.

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

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

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.

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.

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? 🤔

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.

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)

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.

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

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.

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

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

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

@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