Make CopyOnWriteBuffer keep capacity

webrtc/base/copyonwritebuffer.h

 /*
  *  Copyright 2016 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 136 matching lines @@
   }
 
   // Replace the contents of the buffer. Accepts the same types as the
   // constructors.
   template <typename T,
             typename std::enable_if<
                 internal::BufferCompat<uint8_t, T>::value>::type* = nullptr>
   void SetData(const T* data, size_t size) {
     RTC_DCHECK(IsConsistent());
     if (!buffer_ || !buffer_->HasOneRef()) {
       buffer_ = size > 0 ? new RefCountedObject<Buffer>(data, size)

[kwiberg-webrtc, 2016/09/09 08:55:13]

Here the new buffer gets capacity size, instead of max(size, old_capacity). As
far as I can tell, this is the only place where CopyOnWriteBuffer doesn't
preserve the existing capacity.

[danilchap, 2016/09/09 11:21:17]

Yes, that the only place I found too,
but it might be desirable to allocate smaller buffer here, so I choose to add
similar function instead modifying existent.

for SetSize it would make sense to reduce capacity sometimes too.

[kwiberg-webrtc, 2016/09/09 12:59:13]

Hmm. Right now, no one can look at CopyOnWriteBuffer calls and tell
which ones are going to cause reallocations without first checking the
implementation. It'd be very nice if we could (1) define a suitable
and simple behavior, and (2) stick to it.

The thing that makes CopyOnWriteBuffer trickier than plain old Buffer
is that allocations can happen as a result of a write, even if the
capacity is sufficient, so preserving the capacity (like the current
implementation goes to some pains to do) isn't always enough, or even
a good idea.

I wonder if we couldn't do something simpler, that would also be
easier to understand. Consider this, for example:

  class CowBuffer {
    const Buffer& get_buffer();     // read-only access to shared buffer
    Buffer steal_buffer(size_t capacity = 0);  // return buffer, leaving the
                                               // CowBuffer empty; will make a
                                               // copy if there are multiple
                                               // owners
    void set_buffer(Buffer buf);  // replace buffer
  };

The get_buffer() method would provide read-only access to a possibly
shared Buffer, while set_buffer() would replace the current Buffer
with one that the caller provides. To modify a CowBuffer, you'd do
something like

  Buffer buf(cowbuf.steal_buffer(4711));
  buf.AppendData(...);
  cowbuf.set_buffer(std::move(buf));

or

  cowbuf.set_buffer(Buffer(...));

or several other variations.

This would have two advantages over the current situation:

  1. Much simpler. For almost everything, we just let our user use
     Buffer directly. It'll be slightly more typing, but less magic,
     since it'll always be easy to see where the extra copying may
     happen.

  2. We wouldn't have an API that's slightly different from Buffer's,
     because our users use the Buffer methods directly.

[danilchap, 2016/09/09 16:00:56]

This solution wouldn't cover my user case:
Buffer buf(cowbuf.steal_buffer(4711));
buf.SetSize(10);
cowbuf.set_buffer(std::move(buf));

would first make a full copy of 4711 bytes before leaving 10 bytes that matter.
Something I would like to avoid.

[kwiberg-webrtc, 2016/09/10 00:12:18]

Right. So we'd need steal_buffer to be able to do any operation that can be done
cheaply if a copy isn't needed, and that would make the copy cheaper. The
problem is, this could be anything. Consider the operation of reversing the
buffer and keeping the 20 first bytes of the result, for example. You can easily
put that result in a new buffer while reading from the original, and you could
with just a little bit more effort modify a buffer in-place to accomplish it.
But hiding the difference and making those operations look the same? I don't
have a pretty solution to that. Everything I can think of boils down to the
caller having to treat the two cases separately, and that'll lead to untested
code paths and bugs.

                          : nullptr;
     } else {
       buffer_->SetData(data, size);
     }
     RTC_DCHECK(IsConsistent());
   }
 
+  // Replace the contents of the buffer. Ensure capacity is at least |capacity|.
+  template <typename T,
+            typename std::enable_if<
+                internal::BufferCompat<uint8_t, T>::value>::type* = nullptr>
+  void SetData(const T* data, size_t size, size_t capacity) {

[kwiberg-webrtc, 2016/09/09 08:55:13]

Isn't this equivalent to calling

  x.EnsureCapacity(foo);
  x.SetData(bar, baz);

Oh, no, because that'll copy the old data if .SetCapacity() needs to
reallocate. Got it.

However,

  1. All the SetData functions should get optional capacity arguments,
     right? (But the AppendData functions don't need them, because
     there it *is* OK to do x.EnsureCapacity(); x.AppendData();.)

  2. Buffer needs the extra arguments just as much as
     CopyOnWriteBuffer, right?

[danilchap, 2016/09/09 11:21:17]

1. SetData signatures mirror constructor signatures. There is only one
constructor that take capacity argument explicitly.
So other SetData probably do no need version with capacity.

2. Buffer probably doesn't need extra argument because Buffer doesn't reduce
capacity (except for move assign operator).
Buffer b; b.EnsureCapacity(capacity); b.SetData(data, size);
would do same as b.SetData(data, size, capacity);

consistency with CopyOnWriteBuffer is the only reason to add it there too. You
think it is strong enough reason?

[kwiberg-webrtc, 2016/09/09 12:59:13]

I'm not sure. You can always replace

  Buffer buf(...);

with

  Buffer buf;
  buf.SetData(...);

with essentially zero performance penalty, so you could argue that the
constructors don't need to be as general as SetData. But you could also argue
that the constructors also ought to have those extra capacity arguments...
No, "b.EnsureCapacity(capacity); b.SetData(data, size);" would copy all the old
data. You'd need "b.Clear(); b.EnsureCapacity(capacity); b.SetData(data,
size);", and no callers are going to get that right.

[danilchap, 2016/09/09 16:00:56]

Yes, I'm wrong. it is not same. Buffer may benefit from SetData(data, size,
capacity) too.

+    RTC_DCHECK(IsConsistent());
+    capacity = std::max(size, capacity);
+    if (!buffer_ || !buffer_->HasOneRef() || buffer_->capacity() < capacity) {
+      buffer_ = capacity > 0
+                    ? new RefCountedObject<Buffer>(data, size, capacity)
+                    : nullptr;
+    } else {
+      buffer_->SetData(data, size);
+    }
+    RTC_DCHECK(IsConsistent());
+  }
+
   template <typename T,
             size_t N,
             typename std::enable_if<
                 internal::BufferCompat<uint8_t, T>::value>::type* = nullptr>
   void SetData(const T (&array)[N]) {
     SetData(array, N);
   }
 
   void SetData(const CopyOnWriteBuffer& buf) {
     RTC_DCHECK(IsConsistent());
@@ skipping 106 matching lines @@
     return (!buffer_ || buffer_->capacity() > 0);
   }
 
   // buffer_ is either null, or points to an rtc::Buffer with capacity > 0.
   scoped_refptr<RefCountedObject<Buffer>> buffer_;
 };
 
 }  // namespace rtc
 
 #endif  // WEBRTC_BASE_COPYONWRITEBUFFER_H_