Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2328553002:
Make CopyOnWriteBuffer keep capacity (Closed)
|
Created:
4 years, 3 months ago by danilchap Modified:
4 years, 3 months ago Reviewers:
kwiberg-webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionMake CopyOnWriteBuffer keep capacity
for SetData and Clear functions too.
This way result of all functions is same
for shared and non-shared buffer cases
R=kwiberg@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/2b2779f3f185bd2a0346cc425405561030cf224c
Patch Set 1 #
Total comments: 18
Patch Set 2 : Feedback #Patch Set 3 : Guarantee capacity #
Total comments: 12
Patch Set 4 : Moved long non-template functions from .h into .cc #Patch Set 5 : Revert move implmentation to .cc #Patch Set 6 : Feedback #Patch Set 7 : Test Feedback #Patch Set 8 : heap_allocation -> original_allocation #
Messages
Total messages: 20 (7 generated)
danilchap@webrtc.org changed reviewers: + kwiberg@webrtc.org
While working with CopyOnWriteBuffer I run into issue where SetData may reduce capacity of the buffer, but in my case I want to keep capacity but avoid extra reallocation and memcpy . new function address that issue taking same set of parameters as one of the constructors.
Description was changed from ========== Added CopyOnWriteBuffer::SetData with capacity guarantees. ========== to ========== Add CopyOnWriteBuffer::SetData with capacity guarantees. ==========
https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer.h File webrtc/base/copyonwritebuffer.h (right): https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer... webrtc/base/copyonwritebuffer.h:157: buffer_ = size > 0 ? new RefCountedObject<Buffer>(data, size) 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. https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer... webrtc/base/copyonwritebuffer.h:169: void SetData(const T* data, size_t size, size_t capacity) { 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? https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer... File webrtc/base/copyonwritebuffer_unittest.cc (right): https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer... webrtc/base/copyonwritebuffer_unittest.cc:152: TEST(CopyOnWriteBufferTest, SetDataSmallerCapacityCauseNoReallocation) { "Causes". Or even better, "DoesntCause" https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer... webrtc/base/copyonwritebuffer_unittest.cc:154: const uint8_t* raw = buf1.cdata(); You can make the pointer const too. https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer... webrtc/base/copyonwritebuffer_unittest.cc:158: EXPECT_EQ(2U, buf1.size()); Lower-case "u". https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer... webrtc/base/copyonwritebuffer_unittest.cc:191: } In order to put the more fundamental tests first, reorder these from 1234 to 3412 or something like that?
https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer.h File webrtc/base/copyonwritebuffer.h (right): https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer... webrtc/base/copyonwritebuffer.h:157: buffer_ = size > 0 ? new RefCountedObject<Buffer>(data, size) On 2016/09/09 08:55:13, kwiberg-webrtc wrote: > 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. 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. https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer... webrtc/base/copyonwritebuffer.h:169: void SetData(const T* data, size_t size, size_t capacity) { On 2016/09/09 08:55:13, kwiberg-webrtc wrote: > 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? 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? https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer... File webrtc/base/copyonwritebuffer_unittest.cc (right): https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer... webrtc/base/copyonwritebuffer_unittest.cc:152: TEST(CopyOnWriteBufferTest, SetDataSmallerCapacityCauseNoReallocation) { On 2016/09/09 08:55:14, kwiberg-webrtc wrote: > "Causes". Or even better, "DoesntCause" Done. https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer... webrtc/base/copyonwritebuffer_unittest.cc:154: const uint8_t* raw = buf1.cdata(); On 2016/09/09 08:55:14, kwiberg-webrtc wrote: > You can make the pointer const too. Done. https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer... webrtc/base/copyonwritebuffer_unittest.cc:158: EXPECT_EQ(2U, buf1.size()); On 2016/09/09 08:55:13, kwiberg-webrtc wrote: > Lower-case "u". for local consistency, Done. but globally prefer uppercase just because one of examples in style guide use uppercase suffix. (none use lowercase there) https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer... webrtc/base/copyonwritebuffer_unittest.cc:191: } On 2016/09/09 08:55:13, kwiberg-webrtc wrote: > In order to put the more fundamental tests first, reorder these from 1234 to > 3412 or something like that? Done. Reordered 4312.
https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer.h File webrtc/base/copyonwritebuffer.h (right): https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer... webrtc/base/copyonwritebuffer.h:157: buffer_ = size > 0 ? new RefCountedObject<Buffer>(data, size) On 2016/09/09 11:21:17, danilchap wrote: > On 2016/09/09 08:55:13, kwiberg-webrtc wrote: > > 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. > > 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. 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. https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer... webrtc/base/copyonwritebuffer.h:169: void SetData(const T* data, size_t size, size_t capacity) { On 2016/09/09 11:21:17, danilchap wrote: > On 2016/09/09 08:55:13, kwiberg-webrtc wrote: > > 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? > > 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. 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... > 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? 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. https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer... File webrtc/base/copyonwritebuffer_unittest.cc (right): https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer... webrtc/base/copyonwritebuffer_unittest.cc:158: EXPECT_EQ(2U, buf1.size()); On 2016/09/09 11:21:17, danilchap wrote: > On 2016/09/09 08:55:13, kwiberg-webrtc wrote: > > Lower-case "u". > > for local consistency, Done. OK. > but globally prefer uppercase just because one of examples in style guide use > uppercase suffix. (none use lowercase there) "u" is better than "U" because it's more visually distinct from a digit. That the style guide happens to use "U" in an unrelated example isn't a good enough reason to use "U", IMO.
May be go for different 'simple' solution: Make it similar to Buffer by forcing CowBuffer to always keep capacity (sure except for copy/move/swap with another CopyOnWriteBuffer). I can argue that for class called SomethingBuffer capacity is too important property to let it shrink as a side effect. This way Buffer and CowBuffer function would have same functionality expectation and differ only in perfomance. In particluar any non-const function in CowBuffer might cause reallocation if buffer is shared. https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer.h File webrtc/base/copyonwritebuffer.h (right): https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer... webrtc/base/copyonwritebuffer.h:157: buffer_ = size > 0 ? new RefCountedObject<Buffer>(data, size) On 2016/09/09 12:59:13, kwiberg-webrtc wrote: > On 2016/09/09 11:21:17, danilchap wrote: > > On 2016/09/09 08:55:13, kwiberg-webrtc wrote: > > > 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. > > > > 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. > > 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. 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. https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer... webrtc/base/copyonwritebuffer.h:169: void SetData(const T* data, size_t size, size_t capacity) { On 2016/09/09 12:59:13, kwiberg-webrtc wrote: > On 2016/09/09 11:21:17, danilchap wrote: > > On 2016/09/09 08:55:13, kwiberg-webrtc wrote: > > > 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? > > > > 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. > > 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... > > > 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? > > 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. Yes, I'm wrong. it is not same. Buffer may benefit from SetData(data, size, capacity) too.
On 2016/09/09 16:00:56, danilchap wrote: > May be go for different 'simple' solution: > Make it similar to Buffer by forcing CowBuffer to always keep capacity (sure > except for copy/move/swap with another CopyOnWriteBuffer). Yes. That's not always optimal, but it's probably good enough for now. And very close to the current behavior. > I can argue that for class called SomethingBuffer capacity is too important > property to let it shrink as a side effect. Mmm. The whole point of tracking the capacity is to get good performance, and it is the case that maintaining the capacity when copying can cause bad performance if that capacity wasn't needed. > This way Buffer and CowBuffer function would have same functionality expectation > and differ only in perfomance. > > In particluar any non-const function in CowBuffer might cause reallocation if > buffer is shared. Yes. In other words, exactly like it is now except we make sure that *all* operations preserve capacity.
https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer.h File webrtc/base/copyonwritebuffer.h (right): https://codereview.webrtc.org/2328553002/diff/1/webrtc/base/copyonwritebuffer... webrtc/base/copyonwritebuffer.h:157: buffer_ = size > 0 ? new RefCountedObject<Buffer>(data, size) On 2016/09/09 16:00:56, danilchap wrote: > On 2016/09/09 12:59:13, kwiberg-webrtc wrote: > > On 2016/09/09 11:21:17, danilchap wrote: > > > On 2016/09/09 08:55:13, kwiberg-webrtc wrote: > > > > 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. > > > > > > 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. > > > > 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. > > 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. 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.
Description was changed from ========== Add CopyOnWriteBuffer::SetData with capacity guarantees. ========== to ========== Make CopyOnWriteBuffer keep capacity for SetSize and Clear functions too. This way result of all functions is same for shared and non-shared buffer cases ==========
Description was changed from ========== Make CopyOnWriteBuffer keep capacity for SetSize and Clear functions too. This way result of all functions is same for shared and non-shared buffer cases ========== to ========== Make CopyOnWriteBuffer keep capacity for SetData and Clear functions too. This way result of all functions is same for shared and non-shared buffer cases ==========
On 2016/09/10 00:12:11, kwiberg-webrtc wrote: > On 2016/09/09 16:00:56, danilchap wrote: > > May be go for different 'simple' solution: > > Make it similar to Buffer by forcing CowBuffer to always keep capacity (sure > > except for copy/move/swap with another CopyOnWriteBuffer). > > Yes. That's not always optimal, but it's probably good enough for now. And very > close to the current behavior. > If that is agreed, can you please take a look if changing SetData and Clear is enough?
Patch set #3 lgtm. Moving longer functions to the .cc file is also a good idea, but doing both in the same CL makes it much harder to review since the diffs are no longer readable. If patch set #4 does nothing else except move code from .h to .cc, then you can land this, but please consider doing this sort of thing in separate CLs next time. https://codereview.webrtc.org/2328553002/diff/40001/webrtc/base/copyonwritebu... File webrtc/base/copyonwritebuffer.h (right): https://codereview.webrtc.org/2328553002/diff/40001/webrtc/base/copyonwritebu... webrtc/base/copyonwritebuffer.h:160: data, size, std::max(size, buffer_->capacity())); I don't think you need the std::max---the Buffer constructor should do that for you. https://codereview.webrtc.org/2328553002/diff/40001/webrtc/base/copyonwritebu... webrtc/base/copyonwritebuffer.h:264: if (!buffer_->HasOneRef()) { You can eliminate the negation if you swap the if and else clauses. https://codereview.webrtc.org/2328553002/diff/40001/webrtc/base/copyonwritebu... File webrtc/base/copyonwritebuffer_unittest.cc (right): https://codereview.webrtc.org/2328553002/diff/40001/webrtc/base/copyonwritebu... webrtc/base/copyonwritebuffer_unittest.cc:122: buf.SetData(static_cast<const uint8_t*>(nullptr), 0u); This much shorter form ought to work: buf.SetData<uint8_t>(nullptr, 0); https://codereview.webrtc.org/2328553002/diff/40001/webrtc/base/copyonwritebu... webrtc/base/copyonwritebuffer_unittest.cc:133: EXPECT_EQ(raw, buf1.cdata()); Maybe a comment? EXPECT_EQ(raw, buf1.cdata()); // Still using the same heap allocation. https://codereview.webrtc.org/2328553002/diff/40001/webrtc/base/copyonwritebu... webrtc/base/copyonwritebuffer_unittest.cc:147: const uint8_t data[] = {'f', 'o', 'o', 0x0}; const uint8_t data[] = "foo";
Patchset #7 (id:120001) has been deleted
patch set #4 did just the move, but I still reverted it and will redo it in different CL for better history. https://codereview.webrtc.org/2328553002/diff/40001/webrtc/base/copyonwritebu... File webrtc/base/copyonwritebuffer.h (right): https://codereview.webrtc.org/2328553002/diff/40001/webrtc/base/copyonwritebu... webrtc/base/copyonwritebuffer.h:160: data, size, std::max(size, buffer_->capacity())); On 2016/09/13 09:24:30, kwiberg-webrtc wrote: > I don't think you need the std::max---the Buffer constructor should do that for > you. Done. It is nicer without extra max. https://codereview.webrtc.org/2328553002/diff/40001/webrtc/base/copyonwritebu... webrtc/base/copyonwritebuffer.h:264: if (!buffer_->HasOneRef()) { On 2016/09/13 09:24:30, kwiberg-webrtc wrote: > You can eliminate the negation if you swap the if and else clauses. Done. https://codereview.webrtc.org/2328553002/diff/40001/webrtc/base/copyonwritebu... File webrtc/base/copyonwritebuffer_unittest.cc (right): https://codereview.webrtc.org/2328553002/diff/40001/webrtc/base/copyonwritebu... webrtc/base/copyonwritebuffer_unittest.cc:122: buf.SetData(static_cast<const uint8_t*>(nullptr), 0u); On 2016/09/13 09:24:31, kwiberg-webrtc wrote: > This much shorter form ought to work: > > buf.SetData<uint8_t>(nullptr, 0); Done. https://codereview.webrtc.org/2328553002/diff/40001/webrtc/base/copyonwritebu... webrtc/base/copyonwritebuffer_unittest.cc:133: EXPECT_EQ(raw, buf1.cdata()); On 2016/09/13 09:24:30, kwiberg-webrtc wrote: > Maybe a comment? > > EXPECT_EQ(raw, buf1.cdata()); // Still using the same heap allocation. Prefer to comment with variable names. changed raw to heap_allocation. https://codereview.webrtc.org/2328553002/diff/40001/webrtc/base/copyonwritebu... webrtc/base/copyonwritebuffer_unittest.cc:147: const uint8_t data[] = {'f', 'o', 'o', 0x0}; On 2016/09/13 09:24:30, kwiberg-webrtc wrote: > const uint8_t data[] = "foo"; Done.
lgtm https://codereview.webrtc.org/2328553002/diff/40001/webrtc/base/copyonwritebu... File webrtc/base/copyonwritebuffer_unittest.cc (right): https://codereview.webrtc.org/2328553002/diff/40001/webrtc/base/copyonwritebu... webrtc/base/copyonwritebuffer_unittest.cc:133: EXPECT_EQ(raw, buf1.cdata()); On 2016/09/13 09:56:37, danilchap wrote: > On 2016/09/13 09:24:30, kwiberg-webrtc wrote: > > Maybe a comment? > > > > EXPECT_EQ(raw, buf1.cdata()); // Still using the same heap allocation. > > Prefer to comment with variable names. > changed raw to heap_allocation. Hmm. The point isn't that it's on the heap, but that it's still the same after the operation. "original_allocation" would be better.
https://codereview.webrtc.org/2328553002/diff/40001/webrtc/base/copyonwritebu... File webrtc/base/copyonwritebuffer_unittest.cc (right): https://codereview.webrtc.org/2328553002/diff/40001/webrtc/base/copyonwritebu... webrtc/base/copyonwritebuffer_unittest.cc:133: EXPECT_EQ(raw, buf1.cdata()); On 2016/09/13 10:01:18, kwiberg-webrtc wrote: > On 2016/09/13 09:56:37, danilchap wrote: > > On 2016/09/13 09:24:30, kwiberg-webrtc wrote: > > > Maybe a comment? > > > > > > EXPECT_EQ(raw, buf1.cdata()); // Still using the same heap allocation. > > > > Prefer to comment with variable names. > > changed raw to heap_allocation. > > Hmm. The point isn't that it's on the heap, but that it's still the same after > the operation. "original_allocation" would be better. Yes, that name is better, Thank you.
Description was changed from ========== Make CopyOnWriteBuffer keep capacity for SetData and Clear functions too. This way result of all functions is same for shared and non-shared buffer cases ========== to ========== Make CopyOnWriteBuffer keep capacity for SetData and Clear functions too. This way result of all functions is same for shared and non-shared buffer cases R=kwiberg@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/2b2779f3f185bd2a0346cc425... ==========
Message was sent while issue was closed.
Description was changed from ========== Make CopyOnWriteBuffer keep capacity for SetData and Clear functions too. This way result of all functions is same for shared and non-shared buffer cases R=kwiberg@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/2b2779f3f185bd2a0346cc425... ========== to ========== Make CopyOnWriteBuffer keep capacity for SetData and Clear functions too. This way result of all functions is same for shared and non-shared buffer cases R=kwiberg@webrtc.org Committed: https://crrev.com/2b2779f3f185bd2a0346cc425405561030cf224c Cr-Commit-Position: refs/heads/master@{#14196} ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001) manually as 2b2779f3f185bd2a0346cc425405561030cf224c (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
