Add (*Reader).CopyNext(w) (int64, error)

[pwaller]

It is useful to be able to efficiently copy objects without
decoding them.

My use case is filtering when I already know the indices of
the objects I want to keep, and for rewriting a dictionary
of objects as a column of objects.

This commit:

1. Exports GetNextSize.
2. Adds a method ReadNext(p) to *Reader.

I wasn't sure about exporting GetNextSize, but I can't see the
harm in it, and it may be useful for finer-grained control in
the copying.

I also experimented with a NextReader() io.Reader method which
returned io.LimitReader(m.R, GetNextSize()), but for my use case
this was twice as slow and also did not handle nested objects.
In principle that might be useful for very large simple objects,
but is not included in this PR.

---

This change is

[philhofer]

I think the errors returned from ReadNext need to implement msgp.Error to preserve that API contract. Beyond that, 👍

[philhofer]

Actually, as I squint harder at this, it's not clear that ReadNext does what you want it to do. The returned size value from getNextSize for an object encoded in a fixmap, for instance, is 1 byte; you don't get the size of the actual object.

[pwaller]

Ah. Any ideas on how to fix this or otherwise do it correctly?

[pwaller]

My implementation was inspired by Skip(), so, does skip have that problem or does it avoid that somehow?

[philhofer]

Skip operates recursively. You'd have to do the same thing.

[pwaller]

Oh, crap, I have not pushed the latest commit. Oops.

[pwaller]

Force pushed.

[pwaller]

Will fix Error return, too.

[philhofer]

Ok cool; this looks correct.

I might suggest ReadNext(w io.Writer) (int, error), though. That may make the API easier to use (e.g. buf := bytes.NewBuffer(...); r.ReadNext(buf); ...)

[philhofer]

Also, given the non-obvious nature of GetNextSize, perhaps it shouldn't be exported as-is.

[pwaller]

Nice idea for ReadNext would suit me well. How about CopyNext as a name? ReadNext as a thing taking a writer seems confusing.

For GetNextSize I agree, though I may re-propose that later if I find I do need it.

Still fixing the error contract.

[philhofer]

CopyNext sounds good to me.

[pwaller]

1. On second thought, Copy/io.Writer-style rather than Read means that the caller loses control of the buffer which is not good for allocation unless we do something ala io.CopyBuffer. Can we reconsider ReadNext as it is? Thoughts?
2. I pushed Error interface just now.
3. I am also unsure about whether the additional size check is necessary, do you have an opinion?

    n, err := m.R.ReadFull(p[:sz])
    if err != nil {
        return 0, err
    }
    if uintptr(n) != sz {
        return 0, fmt.Errorf("wrong # bytes read (%d != %d)", n, int64(sz))
    }

[pwaller]

(If (3) is necessary, does it want to be yet another named error in errors.go, or something else?)

Thanks for the rapid review by the way.

[philhofer]

The caller doesn't necessarily lose control of the buffer. bytes.NewBuffer(buf) will do the 'right' thing in the case where your buffer is large enough; you just need to call Reset() before copying the next object.

The size check should be unnecessary; it's a bug if ReadFull returns without error and doesn't fill the buffer.

[philhofer]

(The Copy API also doesn't require that you hold the object in memory, either; it could be used to proxy objects or sub-objects of arbitrary size.)

[pwaller]

The caller doesn't necessarily lose control of the buffer.

But in order to call the w.Write from inside CopyNext(w), I need to call ReadFull with a byte slice. From whence does this byte slice come?

[philhofer]

Yeah; you'd need to refactor the code a bit. (*Reader).Next() gives you a pointer directly into the reader's buffer.

[philhofer]

Also, *fwd.Reader implements WriteTo, so io.CopyN() shouldn't allocate a buffer.
EDIT: Never mind, io.CopyN doesn't use io.WriterTo. But still, if it were a bytes.Buffer, for instance, you'd still get the ReadFrom optimization.

[pwaller]

That was the bit I was missing. That is freaking awesome:

--- a/msgp/read.go
+++ b/msgp/read.go
@@ -180,6 +180,32 @@ func (m *Reader) ReadNext(p []byte) (int, error) {
    return tot, nil
 }

+// CopyNext reads the next object from m without decoding it and writes it to w.
+// It avoids unnecessary copies internally.
+func (m *Reader) CopyNext(w io.Writer) (int64, error) {
+   sz, o, err := getNextSize(m.R)
+   if err != nil {
+       return 0, err
+   }
+
+   // avoids allocating because m.R implements WriteTo.
+   n, err := io.CopyN(w, m.R, int64(sz))
+   if err != nil {
+       return 0, err
+   }
+
+   // for maps and slices, read elements
+   for x := uintptr(0); x < o; x++ {
+       var n2 int64
+       n2, err = m.CopyNext(w)
+       if err != nil {
+           return n, err
+       }
+       n += n2
+   }
+   return n, nil
+}
+
 // ReadFull implements `io.ReadFull`
 func (m *Reader) ReadFull(p []byte) (int, error) {
    return m.R.ReadFull(p)

I have fully squashed my patches and force pushed.

[pwaller]

Sorry, please hold, I forgot to remove ReadNext correctly.

Also, do we need to revisit in light of your strikeout in the above comment?

[philhofer]

I would use (*fwd.Reader).Next(), and then fall back to CopyN when that fails. The reasoning here is that very frequently, Next() will work, and it will only fail when you ask for more bytes than can fit in the read buffer, in which case the cost of CopyN will amortize well, even if it does have to allocate a buffer.

[pwaller]

Pushed. My current status: happy. Taking a break for a bit.

I learned some neat tricks tonight, thanks for that 😎

[philhofer]

@pwaller hey, did you want to finish this off? If not, I'm happy to do so myself.

[pwaller]

AFAIK, I think it's done? I've ticked "Allow edits from maintainers" if you would like to make any additional changes.

[philhofer]

Cool. Pushed a small change in the error handling to make it a little more consistent with the rest of the code.

Thanks again!

[pwaller]

👍