Support for Buffer slices & copyless methods in RepeatedBytes

[cmuramoto]

With the current API, it is 'tricky' to append byte[]s to an instance of RepeatedBytes without a copy.

The pattern is more or less like

void transfer(RepeatedBytes rb, Collection<byte[]> values) {
  rb.reserve(values.size());
  for(var value: values) {
    rb.next().setInternalArray(value).
  }
}

It would by nice to have something like

public final void addInternal(RepeatedByte value) {
  reserve(1);
  array[length++] = value;
}

public final void addInternal(byte[] value) {
  addInternal(RepeatedByte.newEmptyInstance().setInternalArray(value));
}

public final void setInternal(int index, RepeatedByte value) {
  checkIndex(index);
  array[index] = value;
}

public final void setInternal(int index, byte[] value) {
  setInternal(index,RepeatedByte.newEmptyInstance().setInternalArray(value)); 
}

When working with direct buffers, we must manifest a byte[] copy to transfer contents to a Message instance. Even though it is possible to avoid a second copy in RepeatedByte via setInternalArray, the same cannot be done in RepeatedBytes. Perhaps some helpers such as

static final byte[] copy(ByteBuffer value) {
  var rv = new byte[value.remaining()];
  buffer.duplicate().get(rv);
  return rv; 
}

public final void add(ByteBuffer value) {
  addInternal(copy(value));
}

public final void setInternal(int index, ByteBuffer value) {
  setInternal(index, copy(value)); 
}

could be useful too.

[ennerf]

The setInternal API was supposed to be a hack for rare use cases, but I should've expected it to become a desired feature :sweat_smile:

What is the problem with the first pattern you posted? To me that seems cleaner than the proposed helpers.

Should RepeatedByte have an add(ByteBuffer src) method so it can copy from a direct byte buffer without the extra conversions?

[cmuramoto]

@ennerf Personally, I don't dislike the pattern.

I just stumbled upon this during a profiling session. My team was used to the builder-style of protobuf-java and during migration to quickbuffers they ended up mapping message.addValue(ByteString.wrap(value)) to message.addValues(value).

Still, there are some scenarios where we have a template instance and we used to modify it via a shallow clone message.toBuilder().setValue(index, newValue).build(). With quickbuffers, since clone() is deep, I'm trying to avoid it for messages holding a few KBs of byte arrays, but not having random access capability to write without copy makes merging these template instances with new values using the cursor pattern a bit cumbersome.

As for add(ByteBuffer src) it would be nice to have it in RepeatedByte!