Avoid copying the (potentially large) rest of the decoder buffer

[ankon]

This aligns 'readAll' with other 'read' methods, which do not produce a copy unless needed.
This avoids a couple of likely very expensive copy operations in the network receive path.

Signed-off-by: Andreas Kohn andreas.kohn@gmail.com

[Nevon]

While I applaud the initiative (I desperately want to fix the performance of our encoder/decoder), I don't think this actually changes anything. Buffer.slice returns a new copy of the buffer, so either way you are copying the data.

Also, your new implementation changes the offset of the decoder when reading. While this makes sense, I believe the original intent was to be able to read the entire remaining content of the buffer without affecting the offset (at least that's what the old implementation does).

[ankon]

Buffer.slice returns a new copy of the buffer, so either way you are copying the data.

Maybe I'm misreading the docs then, but https://nodejs.org/api/buffer.html#buffer_buf_slice_start_end says:

Returns a new Buffer that references the same memory as the original, but offset and cropped by the start and end indices.

As to changing the offset: Indeed, it does, to the end of the buffer of the decoder. In the places where readAll is used that isn't a problem, as the decoder is afterwards thrown away. Changing the offset explicitly here was to protect future users from using readAll multiple times and getting the same content back. That's what I meant with aligning the behavior: All read-ish methods move the offset, i.e. they consume the specific length of bytes from the decoder.

[Nevon]

You're completely correct about the copying behavior. I must have been thinking of the behavior of Uint8Array, where slicing does create a new copy.

Since that's the case, I'm all for getting this merged. Modifying the offset is a "breaking" change, but if none of the call sites are really affected, then I agree that it makes sense to align the behavior of the read methods. If we really do need a "read without moving the offset", we can create a method that's more explicit about that.

[ankon]

Thanks!

[tulios]

Pre-release 1.11.0-beta.6 was published with the change.