Use CopyOnWriteBuffer instead of Buffer to avoid unnecessary copies.

Use CopyOnWriteBuffer instead of Buffer to avoid unnecessary copies.

This CL removes copy and assign support from Buffer and changes various
parameters from Buffer to CopyOnWriteBuffer so they can be passed along
and copied without actually copying the underlying data.

With this changed some parameters to be "const" and fixed an issue when
creating a CopyOnWriteBuffer with empty data.

BUG=webrtc:5155
Committed: https://crrev.com/944c39006f1c52aee20919676002dac7a42b1c05
Cr-Commit-Position: refs/heads/master@{#12058}

[joachim, 2016-03-11 22:59:57]

jbauch@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[joachim, 2016-03-11 22:59:58]

Ptal, this is a follow up to https://codereview.webrtc.org/1697743003

As suggested in #15 in that CL, I started by removing the copy and assign
functionality from Buffer and went from there to updated code/parameters where
buffers were copied.

[kwiberg-webrtc, 2016-03-14 11:53:49]

Did you change Buffer -> CopyOnWriteBuffer only where you had to? (That is,
where the compiler complained that the Buffer was beinbg copied?)

https://codereview.webrtc.org/1785713005/diff/20001/webrtc/base/copyonwritebu...
File webrtc/base/copyonwritebuffer.h (right):

https://codereview.webrtc.org/1785713005/diff/20001/webrtc/base/copyonwritebu...
webrtc/base/copyonwritebuffer.h:47: }
The reason for having the conditional is clearer if you write the (equivalent)
if (buffer_) instead.

https://codereview.webrtc.org/1785713005/diff/20001/webrtc/base/copyonwritebu...
webrtc/base/copyonwritebuffer.h:138: RTC_DCHECK(IsConsistent());
Hmm. This is still wrong: if size == 0 and !buffer_->HasOneRef(), you'll modify
the shared buffer. I think it should be something like

if (!buffer_ || !buffer_->HasOneRef()) {
  buffer_ = size > 0 ? new RefCountedObject<Buffer>(data, size)
                     : nullptr;
} else {
  buffer_->SetData(data, size);
}

https://codereview.webrtc.org/1785713005/diff/20001/webrtc/base/copyonwritebu...
File webrtc/base/copyonwritebuffer_unittest.cc (right):

https://codereview.webrtc.org/1785713005/diff/20001/webrtc/base/copyonwritebu...
webrtc/base/copyonwritebuffer_unittest.cc:45: CopyOnWriteBuffer
buf(static_cast<const uint8_t*>(nullptr), 0);
Do you need the static_cast here?

https://codereview.webrtc.org/1785713005/diff/20001/webrtc/base/copyonwritebu...
webrtc/base/copyonwritebuffer_unittest.cc:139: buf.SetData(static_cast<const
uint8_t*>(nullptr), 0u);
...and here?

[joachim, 2016-03-17 20:27:21]

Yes, I only changed what I needed to make it compile again. However at some
point that required changing the parameters of "On[Packet|Rtcp]Received" /
"Send[Packet|Rtcp]" (and the methods called by them), therefore a lot of files
needed the same change.

https://codereview.webrtc.org/1785713005/diff/20001/webrtc/base/copyonwritebu...
File webrtc/base/copyonwritebuffer.h (right):

https://codereview.webrtc.org/1785713005/diff/20001/webrtc/base/copyonwritebu...
webrtc/base/copyonwritebuffer.h:47: }
On 2016/03/14 11:53:49, kwiberg-webrtc wrote:
> The reason for having the conditional is clearer if you write the (equivalent)
> if (buffer_) instead.

Done.

https://codereview.webrtc.org/1785713005/diff/20001/webrtc/base/copyonwritebu...
webrtc/base/copyonwritebuffer.h:138: RTC_DCHECK(IsConsistent());
On 2016/03/14 11:53:49, kwiberg-webrtc wrote:
> Hmm. This is still wrong: if size == 0 and !buffer_->HasOneRef(), you'll
modify
> the shared buffer. I think it should be something like
> 
> if (!buffer_ || !buffer_->HasOneRef()) {
>   buffer_ = size > 0 ? new RefCountedObject<Buffer>(data, size)
>                      : nullptr;
> } else {
>   buffer_->SetData(data, size);
> }

Thanks. Changed and added a test.

https://codereview.webrtc.org/1785713005/diff/20001/webrtc/base/copyonwritebu...
File webrtc/base/copyonwritebuffer_unittest.cc (right):

https://codereview.webrtc.org/1785713005/diff/20001/webrtc/base/copyonwritebu...
webrtc/base/copyonwritebuffer_unittest.cc:45: CopyOnWriteBuffer
buf(static_cast<const uint8_t*>(nullptr), 0);
On 2016/03/14 11:53:49, kwiberg-webrtc wrote:
> Do you need the static_cast here?

Yes, otherwise compilation fails with

error: no matching constructor for initialization of 'rtc::CopyOnWriteBuffer'
note: candidate constructor not viable: no known conversion from 'nullptr_t' to
'size_t' (aka 'unsigned long') for 1st argument
  CopyOnWriteBuffer(size_t size, size_t capacity);
  ^
note: candidate template ignored: could not match 'const T *' against
'nullptr_t'
  CopyOnWriteBuffer(const T* data, size_t size)
  ^

...and similar errors...

https://codereview.webrtc.org/1785713005/diff/20001/webrtc/base/copyonwritebu...
webrtc/base/copyonwritebuffer_unittest.cc:139: buf.SetData(static_cast<const
uint8_t*>(nullptr), 0u);
On 2016/03/14 11:53:49, kwiberg-webrtc wrote:
> ...and here?

see above

[kwiberg-webrtc, 2016-03-18 05:27:11]

lgtm

(Note that you'll also need lgtm from OWNER(s) of all these directories.)

https://codereview.webrtc.org/1785713005/diff/20001/webrtc/base/copyonwritebu...
File webrtc/base/copyonwritebuffer_unittest.cc (right):

https://codereview.webrtc.org/1785713005/diff/20001/webrtc/base/copyonwritebu...
webrtc/base/copyonwritebuffer_unittest.cc:45: CopyOnWriteBuffer
buf(static_cast<const uint8_t*>(nullptr), 0);
On 2016/03/17 20:27:21, joachim wrote:
> On 2016/03/14 11:53:49, kwiberg-webrtc wrote:
> > Do you need the static_cast here?
> 
> Yes, otherwise compilation fails with
> 
> error: no matching constructor for initialization of 'rtc::CopyOnWriteBuffer'
> note: candidate constructor not viable: no known conversion from 'nullptr_t'
to
> 'size_t' (aka 'unsigned long') for 1st argument
>   CopyOnWriteBuffer(size_t size, size_t capacity);
>   ^
> note: candidate template ignored: could not match 'const T *' against
> 'nullptr_t'
>   CopyOnWriteBuffer(const T* data, size_t size)
>   ^
> 
> ...and similar errors...

Right. Thanks.

https://codereview.webrtc.org/1785713005/diff/20001/webrtc/base/copyonwritebu...
webrtc/base/copyonwritebuffer_unittest.cc:139: buf.SetData(static_cast<const
uint8_t*>(nullptr), 0u);
On 2016/03/17 20:27:21, joachim wrote:
> On 2016/03/14 11:53:49, kwiberg-webrtc wrote:
> > ...and here?
> 
> see above

Acknowledged.

[joachim, 2016-03-18 08:28:55]

jbauch@webrtc.org changed reviewers:
+ pthatcher@webrtc.org, tkchin@webrtc.org, tommi@webrtc.org

[joachim, 2016-03-18 08:28:56]

+tkchin for talk/app/webrtc/objc and webrtc/api/objc

+tommi for webrtc/api, webrtc/base and webrtc/pc

+pthatcher for webrtc/media

[tommi, 2016-03-18 10:54:37]

This is pretty awesome :D  alas no good deed goes unpunished... There are a few
places where we're copying data seemingly for no good reason.  There are a
couple of places that I'm wondering if we can fix that right now.  I and/or Karl
can probably also help with fixing some of these things, so feel free to put
down TODOs for me if you want.

https://codereview.webrtc.org/1785713005/diff/40001/webrtc/api/sctputils.cc
File webrtc/api/sctputils.cc (right):

https://codereview.webrtc.org/1785713005/diff/40001/webrtc/api/sctputils.cc#n...
webrtc/api/sctputils.cc:39: rtc::ByteBuffer buffer(payload.data<char>(),
payload.size());
is this really just to check a single uint8?  Can we fix this now by checking
payload.size() >= 1 and checking the value of the first uint8?

https://codereview.webrtc.org/1785713005/diff/40001/webrtc/api/sctputils.cc#n...
webrtc/api/sctputils.cc:55: rtc::ByteBuffer buffer(payload.data<char>(),
payload.size());
This is a more complex case of course, maybe file a bug for tracking?

From looking at the code, I think it could be simplified quite a bit too.  Just
read all the required bytes into a struct on the stack that we can then check -
or, better yet, check the size, cast the buffer to a struct ptr and no need to
copy anything. (I'm guessing you're already thinking this)

https://codereview.webrtc.org/1785713005/diff/40001/webrtc/api/sctputils.cc#n...
webrtc/api/sctputils.cc:127: rtc::ByteBuffer buffer(payload.data<char>(),
payload.size());
so much copy :(

if we can fix this case now (since it seems very simple), that would be good.

https://codereview.webrtc.org/1785713005/diff/40001/webrtc/api/sctputils.cc#n...
webrtc/api/sctputils.cc:182: payload->SetData(buffer.Data(), buffer.Length());
Hmm... there's no error checking for any of the preceding write operations and
no checking if Length() is the expected value.  I think there's room for
improvement.  Can you put down a TODO for me to take a look at this?

https://codereview.webrtc.org/1785713005/diff/40001/webrtc/api/sctputils.cc#n...
webrtc/api/sctputils.cc:187: rtc::ByteBuffer
buffer(rtc::ByteBuffer::ORDER_NETWORK);
I'm curious - do you know what's going on here?
a single byte buffer in network byte order?

I'm wondering what's the difference is between this function and doing:

uint8_t data = DATA_CHANNEL_OPEN_ACK_MESSAGE_TYPE;
payload->SetData(&data, sizeof(data));

https://codereview.webrtc.org/1785713005/diff/40001/webrtc/base/buffer.h
File webrtc/base/buffer.h (right):

https://codereview.webrtc.org/1785713005/diff/40001/webrtc/base/buffer.h#newc...
webrtc/base/buffer.h:277: RTC_DISALLOW_COPY_AND_ASSIGN(Buffer);
nice!

[joachim, 2016-03-18 12:18:08]

Thanks for your feedback.

https://codereview.webrtc.org/1785713005/diff/40001/webrtc/api/sctputils.cc
File webrtc/api/sctputils.cc (right):

https://codereview.webrtc.org/1785713005/diff/40001/webrtc/api/sctputils.cc#n...
webrtc/api/sctputils.cc:39: rtc::ByteBuffer buffer(payload.data<char>(),
payload.size());
On 2016/03/18 10:54:37, tommi-webrtc wrote:
> is this really just to check a single uint8?  Can we fix this now by checking
> payload.size() >= 1 and checking the value of the first uint8?

Done, added bracket operators to CopyOnWriteBuffer for this.

https://codereview.webrtc.org/1785713005/diff/40001/webrtc/api/sctputils.cc#n...
webrtc/api/sctputils.cc:55: rtc::ByteBuffer buffer(payload.data<char>(),
payload.size());
On 2016/03/18 10:54:37, tommi-webrtc wrote:
> This is a more complex case of course, maybe file a bug for tracking?

Added https://bugs.chromium.org/p/webrtc/issues/detail?id=5670

> From looking at the code, I think it could be simplified quite a bit too. 
Just
> read all the required bytes into a struct on the stack that we can then check
-
> or, better yet, check the size, cast the buffer to a struct ptr and no need to
> copy anything. (I'm guessing you're already thinking this)

I would defer this to a separate CL. Accessing the data directly might need to
convert from network byte order to host byte order.

https://codereview.webrtc.org/1785713005/diff/40001/webrtc/api/sctputils.cc#n...
webrtc/api/sctputils.cc:127: rtc::ByteBuffer buffer(payload.data<char>(),
payload.size());
On 2016/03/18 10:54:37, tommi-webrtc wrote:
> so much copy :(
> 
> if we can fix this case now (since it seems very simple), that would be good.

Done.

https://codereview.webrtc.org/1785713005/diff/40001/webrtc/api/sctputils.cc#n...
webrtc/api/sctputils.cc:182: payload->SetData(buffer.Data(), buffer.Length());
On 2016/03/18 10:54:37, tommi-webrtc wrote:
> Hmm... there's no error checking for any of the preceding write operations and
> no checking if Length() is the expected value.  I think there's room for
> improvement.  Can you put down a TODO for me to take a look at this?

Done.

https://codereview.webrtc.org/1785713005/diff/40001/webrtc/api/sctputils.cc#n...
webrtc/api/sctputils.cc:187: rtc::ByteBuffer
buffer(rtc::ByteBuffer::ORDER_NETWORK);
On 2016/03/18 10:54:37, tommi-webrtc wrote:
> I'm curious - do you know what's going on here?
> a single byte buffer in network byte order?
> 
> I'm wondering what's the difference is between this function and doing:
> 
> uint8_t data = DATA_CHANNEL_OPEN_ACK_MESSAGE_TYPE;
> payload->SetData(&data, sizeof(data));

Yeah, that looks overly complicated - changed to your suggestion.

[tommi, 2016-03-18 13:22:28]

lgtm! thanks very much for improving sctputils.cc along the way.

[tommi, 2016-03-18 13:22:44]

The CQ bit was checked by tommi@webrtc.org

[tommi, 2016-03-18 13:22:45]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1785713005/#ps60001 (title:
"Feedback from tommi.")

[tommi, 2016-03-18 13:22:51]

The CQ bit was unchecked by tommi@webrtc.org

[tkchin_webrtc, 2016-03-18 17:57:50]

lgtm Thanks!

[tommi, 2016-03-19 07:57:13]

Committing since I think Peter is ooo.

[tommi, 2016-03-19 07:57:20]

The CQ bit was checked by tommi@webrtc.org

[commit-bot: I haz the power, 2016-03-19 07:57:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1785713005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1785713005/60001

[commit-bot: I haz the power, 2016-03-19 08:57:37]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-19 08:57:46]

Description was changed from

==========
Use CopyOnWriteBuffer instead of Buffer to avoid unnecessary copies.

This CL removes copy and assign support from Buffer and changes various
parameters from Buffer to CopyOnWriteBuffer so they can be passed along
and copied without actually copying the underlying data.

With this changed some parameters to be "const" and fixed an issue when
creating a CopyOnWriteBuffer with empty data.

BUG=webrtc:5155
==========

to

==========
Use CopyOnWriteBuffer instead of Buffer to avoid unnecessary copies.

This CL removes copy and assign support from Buffer and changes various
parameters from Buffer to CopyOnWriteBuffer so they can be passed along
and copied without actually copying the underlying data.

With this changed some parameters to be "const" and fixed an issue when
creating a CopyOnWriteBuffer with empty data.

BUG=webrtc:5155

Committed: https://crrev.com/944c39006f1c52aee20919676002dac7a42b1c05
Cr-Commit-Position: refs/heads/master@{#12058}
==========

[commit-bot: I haz the power, 2016-03-19 08:57:47]

Patchset 4 (id:??) landed as
https://crrev.com/944c39006f1c52aee20919676002dac7a42b1c05
Cr-Commit-Position: refs/heads/master@{#12058}

[kjellander_webrtc, 2016-03-19 19:12:37]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.webrtc.org/1817753003/ by kjellander@webrtc.org.

The reason for reverting is: I'm really sorry for having to revert this but it
seems this hit an unexpected compile error downstream:

webrtc/media/sctp/sctpdataengine.cc: In function 'void
cricket::VerboseLogPacket(const void*, size_t, int)':
webrtc/media/sctp/sctpdataengine.cc:172:37: error: invalid conversion from
'const void*' to 'void*' [-fpermissive]
              data, length, direction)) != NULL) {
                                     ^
In file included from webrtc/media/sctp/sctpdataengine.cc:20:0:
third_party/usrsctp/usrsctplib/usrsctp.h:964:1: error:   initializing argument 1
of 'char* usrsctp_dumppacket(void*, size_t, int)' [-fpermissive]
 usrsctp_dumppacket(void *, size_t, int);
 ^

I'm sure you can fix this easily and just re-land this CL, while I'm going to
look into how to add this warning at the public bots (on Monday)..

[joachim, 2016-03-20 10:14:34]

On 2016/03/19 19:12:37, kjellander (webrtc) wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.webrtc.org/1817753003/ by mailto:kjellander@webrtc.org.
> 
> The reason for reverting is: I'm really sorry for having to revert this but it
> seems this hit an unexpected compile error downstream:
> 
> webrtc/media/sctp/sctpdataengine.cc: In function 'void
> cricket::VerboseLogPacket(const void*, size_t, int)':
> webrtc/media/sctp/sctpdataengine.cc:172:37: error: invalid conversion from
> 'const void*' to 'void*' [-fpermissive]
>               data, length, direction)) != NULL) {
>                                      ^
> In file included from webrtc/media/sctp/sctpdataengine.cc:20:0:
> third_party/usrsctp/usrsctplib/usrsctp.h:964:1: error:   initializing argument
1
> of 'char* usrsctp_dumppacket(void*, size_t, int)' [-fpermissive]
>  usrsctp_dumppacket(void *, size_t, int);
>  ^
> 
> I'm sure you can fix this easily and just re-land this CL, while I'm going to
> look into how to add this warning at the public bots (on Monday)..

I don't think that could have been triggered here. The version of usrsctp that
is used by WebRTC expects a "const void*" when dumping the packet.

Anyway, just created https://codereview.webrtc.org/1823503002/ to reland.