Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1697743003:
Add CopyOnWriteBuffer class (Closed)
DescriptionAdd CopyOnWriteBuffer class
This CL introduces a new class CopyOnWriteBuffer that holds data in a
refcounted Buffer which is shared between copied CopyOnWriteBuffer to avoid
unnecessary allocations / memory copies.
BUG=webrtc:5155
Committed: https://crrev.com/13041cf11f1c2691bb3ddc7d28e74cae9333b685
Cr-Commit-Position: refs/heads/master@{#11767}
Patch Set 1 #Patch Set 2 : Readded more IsConsistent checks. #Patch Set 3 : Fixed indent #Patch Set 4 : Move constructor code to .cc (fixes GN builds) #
Total comments: 7
Patch Set 5 : Feedback from Tommi #Patch Set 6 : Removed atomicops.h #
Total comments: 10
Patch Set 7 : Feedback from kwiberg. #
Total comments: 8
Patch Set 8 : Added CopyOnWriteBuffer class #
Total comments: 23
Patch Set 9 : Feedback from kwiberg + more tests. #
Total comments: 8
Patch Set 10 : More feedback from kwiberg #
Messages
Total messages: 32 (8 generated)
The CQ bit was checked by jbauch@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1697743003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1697743003/60001
jbauch@webrtc.org changed reviewers: + tommi@webrtc.org
Ptal https://codereview.webrtc.org/1697743003/diff/60001/webrtc/base/buffer.h File webrtc/base/buffer.h (right): https://codereview.webrtc.org/1697743003/diff/60001/webrtc/base/buffer.h#newc... webrtc/base/buffer.h:126: // Reimplement parts of RefCountedObject which is not in rtc_base_approved. Are there plans to move "refcount.h" into "rtc_base_approved"? This would allow removing this code duplication.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
tommi@webrtc.org changed reviewers: + kwiberg@webrtc.org
Nice - Adding kwiberg as well. https://codereview.webrtc.org/1697743003/diff/60001/webrtc/base/buffer.h File webrtc/base/buffer.h (right): https://codereview.webrtc.org/1697743003/diff/60001/webrtc/base/buffer.h#newc... webrtc/base/buffer.h:51: class BufferData { should this be in the 'internal' namespace? I'm assuming it's not supposed to be used outside this header. https://codereview.webrtc.org/1697743003/diff/60001/webrtc/base/buffer.h#newc... webrtc/base/buffer.h:66: assert(IsConsistent()); use RTC_DCHECK instead of assert() https://codereview.webrtc.org/1697743003/diff/60001/webrtc/base/buffer.h#newc... webrtc/base/buffer.h:126: // Reimplement parts of RefCountedObject which is not in rtc_base_approved. On 2016/02/13 16:08:46, joachim wrote: > Are there plans to move "refcount.h" into "rtc_base_approved"? This would allow > removing this code duplication. Yes, that should be trivial. Btw, does the refcount always have to be thread safe? Depending on how we'll use this code, we might want to have thread safety abstracted away in a traits class. That way we could have one that uses atomic ops, and another that DCHECKs correct thread usage and is single threaded.
Updated, assuming https://codereview.webrtc.org/1701533002/ has landed and "scoped_ref_ptr.h" also moved to "rtc_base_approved". https://codereview.webrtc.org/1697743003/diff/60001/webrtc/base/buffer.h File webrtc/base/buffer.h (right): https://codereview.webrtc.org/1697743003/diff/60001/webrtc/base/buffer.h#newc... webrtc/base/buffer.h:51: class BufferData { On 2016/02/14 09:03:42, tommi-webrtc wrote: > should this be in the 'internal' namespace? I'm assuming it's not supposed to be > used outside this header. You are right, changed. https://codereview.webrtc.org/1697743003/diff/60001/webrtc/base/buffer.h#newc... webrtc/base/buffer.h:66: assert(IsConsistent()); On 2016/02/14 09:03:42, tommi-webrtc wrote: > use RTC_DCHECK instead of assert() Changed here and in other places. https://codereview.webrtc.org/1697743003/diff/60001/webrtc/base/buffer.h#newc... webrtc/base/buffer.h:126: // Reimplement parts of RefCountedObject which is not in rtc_base_approved. On 2016/02/14 09:03:42, tommi-webrtc wrote: > On 2016/02/13 16:08:46, joachim wrote: > > Are there plans to move "refcount.h" into "rtc_base_approved"? This would > allow > > removing this code duplication. > > Yes, that should be trivial. Thanks for your CL to move it, please see my last comment about also moving "scoped_ref_ptr.h". This makes the code here clearer and you wouldn't have to call AddRef/Release manually. > Btw, does the refcount always have to be thread safe? Depending on how we'll > use this code, we might want to have thread safety abstracted away in a traits > class. That way we could have one that uses atomic ops, and another that > DCHECKs correct thread usage and is single threaded. Hmm, for the buffers it probably doesn't need to be thread safe, though I didn't check yet. I will look into making the refcounting more flexible in a separate CL so we could change this later.
Hmmm... I'm not convinced. You add a lot of complexity, including an extra heap allocation, so that Buffer users will get automatic copy-on-write. What's the rationale? Are existing users making a bunch of unnecessary copies, or do you intend to add new users that will? Since Buffer is (currently) a simple, low-level thing, which also happens to be approximately identical to the BufferData class you introduce, might it be a better idea to add a new class---CowBuffer, for example---that has the same interface as Buffer, but has copy-on-write behavior? I'm guessing CowBuffer could use Buffer for storage, just like Buffer uses BufferData in the current CL, but I haven't thought it through carefully. The rationale here would be that although some users want to pay for copy-on-write, most don't. https://codereview.webrtc.org/1697743003/diff/100001/webrtc/base/buffer.cc File webrtc/base/buffer.cc (right): https://codereview.webrtc.org/1697743003/diff/100001/webrtc/base/buffer.cc#ne... webrtc/base/buffer.cc:60: } Hmm. With this change, you no longer have the property that an empty buffer makes no allocations. https://codereview.webrtc.org/1697743003/diff/100001/webrtc/base/buffer.cc#ne... webrtc/base/buffer.cc:78: } Why not =default? It's one line shorter... https://codereview.webrtc.org/1697743003/diff/100001/webrtc/base/buffer.h File webrtc/base/buffer.h (right): https://codereview.webrtc.org/1697743003/diff/100001/webrtc/base/buffer.h#new... webrtc/base/buffer.h:64: BufferData(const void* data, size_t size, size_t capacity); Buffer itself has a large number of constructors because it's convenient for callers. BufferData has only one caller, though, so it makes more sense to minimize the number of redundant methods and just have the call sites do the extra work instead. I'm thinking you could probably reduce the number of constructors to one without harm. https://codereview.webrtc.org/1697743003/diff/100001/webrtc/base/buffer.h#new... webrtc/base/buffer.h:77: } Why have all these accessors when you allow Buffer to read private members? https://codereview.webrtc.org/1697743003/diff/100001/webrtc/base/buffer.h#new... webrtc/base/buffer.h:128: friend class rtc::Buffer; Hmm. Since BufferData is intended to be used only by Buffer, why not make it a struct without private members (since you apparently don't want to hide its internals) and put it in the private section of Buffer?
Thanks for your feedback, I uploaded a new patch set that addresses your comments. The first code path where unnecessary copies are made is in the datachannel code where incoming Buffer objects are copied to/from DataBuffer objects which are also cloned, invoking another copy (see referenced bug). I didn't check for other areas yet. With a new class like "CowBuffer" the creator of the buffer object needs to know if it will later down the code path be copied or not - I don't know if that's feasible. I agree there is a small overhead when writing to buffers for checking if the data was cloned. However for cases where the data was not cloned, it's a refcount check only and for cases where data was cloned, the data will also be copied (as is right now). https://codereview.webrtc.org/1697743003/diff/100001/webrtc/base/buffer.cc File webrtc/base/buffer.cc (right): https://codereview.webrtc.org/1697743003/diff/100001/webrtc/base/buffer.cc#ne... webrtc/base/buffer.cc:60: } On 2016/02/14 14:43:45, kwiberg-webrtc wrote: > Hmm. With this change, you no longer have the property that an empty buffer > makes no allocations. Changed to only allocate if necessary. https://codereview.webrtc.org/1697743003/diff/100001/webrtc/base/buffer.cc#ne... webrtc/base/buffer.cc:78: } On 2016/02/14 14:43:45, kwiberg-webrtc wrote: > Why not =default? It's one line shorter... Right, that was a leftover of some earlier patch set. Reverted to original dtor. https://codereview.webrtc.org/1697743003/diff/100001/webrtc/base/buffer.h File webrtc/base/buffer.h (right): https://codereview.webrtc.org/1697743003/diff/100001/webrtc/base/buffer.h#new... webrtc/base/buffer.h:64: BufferData(const void* data, size_t size, size_t capacity); On 2016/02/14 14:43:46, kwiberg-webrtc wrote: > Buffer itself has a large number of constructors because it's convenient for > callers. BufferData has only one caller, though, so it makes more sense to > minimize the number of redundant methods and just have the call sites do the > extra work instead. I'm thinking you could probably reduce the number of > constructors to one without harm. Done. https://codereview.webrtc.org/1697743003/diff/100001/webrtc/base/buffer.h#new... webrtc/base/buffer.h:77: } On 2016/02/14 14:43:46, kwiberg-webrtc wrote: > Why have all these accessors when you allow Buffer to read private members? Right, the "IsConsistent" checks are already done by their caller. Removed the accessors. https://codereview.webrtc.org/1697743003/diff/100001/webrtc/base/buffer.h#new... webrtc/base/buffer.h:128: friend class rtc::Buffer; On 2016/02/14 14:43:45, kwiberg-webrtc wrote: > Hmm. Since BufferData is intended to be used only by Buffer, why not make it a > struct without private members (since you apparently don't want to hide its > internals) and put it in the private section of Buffer? Ok, makes sense.
Still not entirely convinced that CoW is the right thing for Buffer. Its behavior gets much more complicated, since you often have to consider both the shared and non-shared cases. If unintended copies are a problem, it might be better to make the type move-only. https://codereview.webrtc.org/1697743003/diff/120001/webrtc/base/buffer.cc File webrtc/base/buffer.cc (right): https://codereview.webrtc.org/1697743003/diff/120001/webrtc/base/buffer.cc#ne... webrtc/base/buffer.cc:34: Buffer::Buffer(Buffer&& buf) : data_(std::move(buf.data_)) { If I'm reading things right, scoped_refptr isn't movable, so this will be a normal copy. But performance will be in the ballpark of what the caller expects of a move, so it's probably OK to leave it like this. https://codereview.webrtc.org/1697743003/diff/120001/webrtc/base/buffer.h File webrtc/base/buffer.h (right): https://codereview.webrtc.org/1697743003/diff/120001/webrtc/base/buffer.h#new... webrtc/base/buffer.h:27: class Buffer; This forward declaration should no longer be necessary. https://codereview.webrtc.org/1697743003/diff/120001/webrtc/base/buffer.h#new... webrtc/base/buffer.h:169: data_->AppendData(data, size); Here you'll potentially clone the data, then immediately reallocate it because of insufficient capacity. Solve by passing required capacity to CloneDataIfReferenced? https://codereview.webrtc.org/1697743003/diff/120001/webrtc/base/buffer.h#new... webrtc/base/buffer.h:188: data_->SetSize(size); Here you'll potentially clone the data, then immediately reallocate it because of insufficient capacity. https://codereview.webrtc.org/1697743003/diff/120001/webrtc/base/buffer.h#new... webrtc/base/buffer.h:204: data_->EnsureCapacity(capacity); Here you'll potentially clone the data, then immediately reallocate it because of insufficient capacity. https://codereview.webrtc.org/1697743003/diff/120001/webrtc/base/buffer.h#new... webrtc/base/buffer.h:291: return (data_ || capacity_ == 0) && capacity_ >= size_; Perhaps better to make this data_ && capacity_ > 0 && capacity_ >= size_; and completely eliminate any remaining cases where you have a BufferData with zero capacity? That way, you eliminate one of two ways to construct a zero-capacity Buffer.
https://codereview.webrtc.org/1697743003/diff/120001/webrtc/base/buffer.cc File webrtc/base/buffer.cc (right): https://codereview.webrtc.org/1697743003/diff/120001/webrtc/base/buffer.cc#ne... webrtc/base/buffer.cc:34: Buffer::Buffer(Buffer&& buf) : data_(std::move(buf.data_)) { On 2016/02/15 12:33:16, kwiberg-webrtc wrote: > If I'm reading things right, scoped_refptr isn't movable, so this will be a > normal copy. But performance will be in the ballpark of what the caller expects > of a move, so it's probably OK to leave it like this. scoped_refptr isn't movable, so this would leave two Buffer instances pointing to the same data. Instead, you could do: Buffer::Buffer(Buffer&& buf) { data_.swap(buf.data_); } This also avoids extra AddRef/Release calls.
https://codereview.webrtc.org/1697743003/diff/120001/webrtc/base/buffer.cc File webrtc/base/buffer.cc (right): https://codereview.webrtc.org/1697743003/diff/120001/webrtc/base/buffer.cc#ne... webrtc/base/buffer.cc:34: Buffer::Buffer(Buffer&& buf) : data_(std::move(buf.data_)) { On 2016/02/15 14:56:51, tommi-webrtc wrote: > On 2016/02/15 12:33:16, kwiberg-webrtc wrote: > > If I'm reading things right, scoped_refptr isn't movable, so this will be a > > normal copy. But performance will be in the ballpark of what the caller > expects > > of a move, so it's probably OK to leave it like this. > > scoped_refptr isn't movable, so this would leave two Buffer instances pointing > to the same data. Instead, you could do: > > Buffer::Buffer(Buffer&& buf) { > data_.swap(buf.data_); > } > > This also avoids extra AddRef/Release calls. Yeah, that's better. Introducing sharing is going to be more expensive than I thought, since it can lead to an extra clone of the data if the new copy writes to the data before the old copy is destroyed. Maybe scoped_ref_ptr should support move? Or maybe we should use shared_ptr...
On 2016/02/15 12:33:16, kwiberg-webrtc wrote: > Still not entirely convinced that CoW is the right thing for Buffer. Its > behavior gets much more complicated, since you often have to consider both the > shared and non-shared cases. If unintended copies are a problem, it might be > better to make the type move-only. Tommi and I discussed this face-to-face, and came to the conclusion that keeping Buffer as-is but making it move-only, plus adding a CopyOnWriteBuffer (with a good name!) seems best to us. That lets us keep the lean and mean Buffer, but will cause compile failures when someone attempts to copy it. In those places, the CoW buffer can be used instead. Does that make sense?
On 2016/02/15 17:40:18, kwiberg-webrtc wrote: > On 2016/02/15 12:33:16, kwiberg-webrtc wrote: > > Still not entirely convinced that CoW is the right thing for Buffer. Its > > behavior gets much more complicated, since you often have to consider both the > > shared and non-shared cases. If unintended copies are a problem, it might be > > better to make the type move-only. > > Tommi and I discussed this face-to-face, and came to the conclusion that keeping > Buffer as-is but making it move-only, plus adding a CopyOnWriteBuffer (with a > good name!) seems best to us. That lets us keep the lean and mean Buffer, but > will cause compile failures when someone attempts to copy it. In those places, > the CoW buffer can be used instead. > > Does that make sense? Sure, thanks for the feedback. I'll look into updating this CL over the next days and let you know when there is more to review...
On 2016/02/15 22:19:17, joachim wrote: > On 2016/02/15 17:40:18, kwiberg-webrtc wrote: > > On 2016/02/15 12:33:16, kwiberg-webrtc wrote: > > > Still not entirely convinced that CoW is the right thing for Buffer. Its > > > behavior gets much more complicated, since you often have to consider both > the > > > shared and non-shared cases. If unintended copies are a problem, it might be > > > better to make the type move-only. > > > > Tommi and I discussed this face-to-face, and came to the conclusion that > keeping > > Buffer as-is but making it move-only, plus adding a CopyOnWriteBuffer (with a > > good name!) seems best to us. That lets us keep the lean and mean Buffer, but > > will cause compile failures when someone attempts to copy it. In those places, > > the CoW buffer can be used instead. > > > > Does that make sense? > > Sure, thanks for the feedback. I'll look into updating this CL over the next > days and let you know when there is more to review... Sorry we're asking you to re-do everything... :-) But hopefully you can re-use most of what you wrote.
Description was changed from ========== Share data between copied Buffer objects. This CL introduces a refcounted BufferData object that holds the contents of a Buffer and is shared between copied buffers to avoid unnecessary allocations / memory copies. BUG=webrtc:5155 ========== to ========== Add CopyOnWriteBuffer class This CL introduces a new class CopyOnWriteBuffer that holds data in a refcounted Buffer which is shared between copied CopyOnWriteBuffer to avoid unnecessary allocations / memory copies. BUG=webrtc:5155 ==========
Ok, I decided to split this up in two CLs. This one will add the new class and once it has landed, I will create another CL to change the Buffer to be move-only. This will cause quite some changes to change parameters from "Buffer" to "CopyOnWriteBuffer". @kwiberg: ptal
Looks good. Just some small comments: https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... File webrtc/base/copyonwritebuffer.cc (right): https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.cc:22: : buffer_(std::move(buf.buffer_)) { Same comment here as for the move assignment operator. https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... File webrtc/base/copyonwritebuffer.h (right): https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.h:15: #include <utility> // std::swap (C++11 and later) We're guaranteed to be C++11 or later nowadays. https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.h:61: return reinterpret_cast<T*>(buffer_->data()); I think you can write return buffer_->data<T>(); instead. https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.h:72: return reinterpret_cast<T*>(buffer_->data()); data<T>() https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.h:91: buffer_ = std::move(buf.buffer_); IIRC, we concluded earlier that scoped_refptr doesn't have move. So swap + set to null is probably a better choice. For bonus points, post a bug about scoped_refptr needing move support and link to it from here. https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.h:155: if (!buffer_) { !buffer_ && size > 0 Otherwise, you violate the "buffer capacity is > 0" invariant. https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.h:168: if (!buffer_) { !buffer_ && capacity > 0 for the same reason as above. https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.h:205: scoped_refptr< RefCountedObject<Buffer> > buffer_; All those extra spaces aren't necessary, because C++11. Oh, and is't probably a good idea to document either that buffer_'s capacity is always >0, or that there are two ways to represent a capacity-0 buffer. https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... File webrtc/base/copyonwritebuffer_unittest.cc (right): https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer_unittest.cc:25: void EnsureBuffersEqual(const CopyOnWriteBuffer& buf1, Perhaps call this "EnsureBuffersShareData" or something, since that's what it does. https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer_unittest.cc:37: // from calling non-const "data()". Hmm. I almost suggested a cdata method when reviewing the class itself, so I'll go ahead and do that now. https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer_unittest.cc:38: void EnsureBufferDataNotEqual(const CopyOnWriteBuffer& buf1, EnsureBuffersDontShareData?
All changed + added more tests. https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... File webrtc/base/copyonwritebuffer.cc (right): https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.cc:22: : buffer_(std::move(buf.buffer_)) { On 2016/02/19 12:36:20, kwiberg-webrtc wrote: > Same comment here as for the move assignment operator. Done. https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... File webrtc/base/copyonwritebuffer.h (right): https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.h:15: #include <utility> // std::swap (C++11 and later) On 2016/02/19 12:36:20, kwiberg-webrtc wrote: > We're guaranteed to be C++11 or later nowadays. Actually still need both. "algorithm" provides "std::max", "utility" provides "std::swap". Therefore just removed comments. https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.h:61: return reinterpret_cast<T*>(buffer_->data()); On 2016/02/19 12:36:20, kwiberg-webrtc wrote: > I think you can write > > return buffer_->data<T>(); > > instead. Done. https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.h:72: return reinterpret_cast<T*>(buffer_->data()); On 2016/02/19 12:36:20, kwiberg-webrtc wrote: > data<T>() Done. https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.h:91: buffer_ = std::move(buf.buffer_); On 2016/02/19 12:36:20, kwiberg-webrtc wrote: > IIRC, we concluded earlier that scoped_refptr doesn't have move. So swap + set > to null is probably a better choice. > > For bonus points, post a bug about scoped_refptr needing move support and link > to it from here. Done + bonus points ;-) https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.h:155: if (!buffer_) { On 2016/02/19 12:36:20, kwiberg-webrtc wrote: > !buffer_ && size > 0 > > Otherwise, you violate the "buffer capacity is > 0" invariant. Done. https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.h:168: if (!buffer_) { On 2016/02/19 12:36:20, kwiberg-webrtc wrote: > !buffer_ && capacity > 0 > > for the same reason as above. Done. https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.h:205: scoped_refptr< RefCountedObject<Buffer> > buffer_; On 2016/02/19 12:36:20, kwiberg-webrtc wrote: > All those extra spaces aren't necessary, because C++11. > > Oh, and is't probably a good idea to document either that buffer_'s capacity is > always >0, or that there are two ways to represent a capacity-0 buffer. Done. https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... File webrtc/base/copyonwritebuffer_unittest.cc (right): https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer_unittest.cc:25: void EnsureBuffersEqual(const CopyOnWriteBuffer& buf1, On 2016/02/19 12:36:21, kwiberg-webrtc wrote: > Perhaps call this "EnsureBuffersShareData" or something, since that's what it > does. Done. https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer_unittest.cc:37: // from calling non-const "data()". On 2016/02/19 12:36:21, kwiberg-webrtc wrote: > Hmm. I almost suggested a cdata method when reviewing the class itself, so I'll > go ahead and do that now. Done. https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer_unittest.cc:38: void EnsureBufferDataNotEqual(const CopyOnWriteBuffer& buf1, On 2016/02/19 12:36:21, kwiberg-webrtc wrote: > EnsureBuffersDontShareData? Done.
Just a few more comments, most of them optional. https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... File webrtc/base/copyonwritebuffer.h (right): https://codereview.webrtc.org/1697743003/diff/140001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.h:15: #include <utility> // std::swap (C++11 and later) On 2016/02/19 20:37:42, joachim wrote: > On 2016/02/19 12:36:20, kwiberg-webrtc wrote: > > We're guaranteed to be C++11 or later nowadays. > > Actually still need both. "algorithm" provides "std::max", "utility" provides > "std::swap". Therefore just removed comments. Acknowledged. https://codereview.webrtc.org/1697743003/diff/160001/webrtc/base/copyonwriteb... File webrtc/base/copyonwritebuffer.cc (right): https://codereview.webrtc.org/1697743003/diff/160001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.cc:24: buf.buffer_ = nullptr; Use swap here too, just as in the move assignment. Otherwise you get an extra incref+decref pair, which is not insignificant since IIRC the refcounts are atomic. You should end up with something like CopyOnWriteBuffer::CopyOnWriteBuffer(CopyOnWriteBuffer&& buf) { // commentz here!!! std::swap(buffer_, buf.buffer_); } Note that since buffer_ starts out as null, you don't have to null buf.buffer_ afterwards. https://codereview.webrtc.org/1697743003/diff/160001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.cc:37: } If you wanted to, you could use ?: to set these in the initializers instead of in the constructor body. https://codereview.webrtc.org/1697743003/diff/160001/webrtc/base/copyonwriteb... File webrtc/base/copyonwritebuffer.h (right): https://codereview.webrtc.org/1697743003/diff/160001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.h:61: return buffer_->data<T>(); For clarity and brevity, perhaps replace this function body as "return cdata<T>();"? https://codereview.webrtc.org/1697743003/diff/160001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.h:221: // Will only be allocated if capacity has been set at least once. Perhaps be more explicit: "buffer_ is either null, or points to an rtc::Buffer with capacity >0"? For paranoia bonus points (and even more explicitness), make a private method that tests the invariant (like rtc::Buffer has) and DCHECK it everywhere. But I'll let you decide if you want that.
All changed, ptal. https://codereview.webrtc.org/1697743003/diff/160001/webrtc/base/copyonwriteb... File webrtc/base/copyonwritebuffer.cc (right): https://codereview.webrtc.org/1697743003/diff/160001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.cc:24: buf.buffer_ = nullptr; On 2016/02/20 06:43:32, kwiberg-webrtc wrote: > Use swap here too, just as in the move assignment. Otherwise you get an extra > incref+decref pair, which is not insignificant since IIRC the refcounts are > atomic. > > You should end up with something like > > CopyOnWriteBuffer::CopyOnWriteBuffer(CopyOnWriteBuffer&& buf) { > // commentz here!!! > std::swap(buffer_, buf.buffer_); > } > > Note that since buffer_ starts out as null, you don't have to null buf.buffer_ > afterwards. Done. https://codereview.webrtc.org/1697743003/diff/160001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.cc:37: } On 2016/02/20 06:43:32, kwiberg-webrtc wrote: > If you wanted to, you could use ?: to set these in the initializers instead of > in the constructor body. Done. https://codereview.webrtc.org/1697743003/diff/160001/webrtc/base/copyonwriteb... File webrtc/base/copyonwritebuffer.h (right): https://codereview.webrtc.org/1697743003/diff/160001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.h:61: return buffer_->data<T>(); On 2016/02/20 06:43:32, kwiberg-webrtc wrote: > For clarity and brevity, perhaps replace this function body as "return > cdata<T>();"? Done. https://codereview.webrtc.org/1697743003/diff/160001/webrtc/base/copyonwriteb... webrtc/base/copyonwritebuffer.h:221: // Will only be allocated if capacity has been set at least once. On 2016/02/20 06:43:32, kwiberg-webrtc wrote: > Perhaps be more explicit: "buffer_ is either null, or points to an rtc::Buffer > with capacity >0"? Done > For paranoia bonus points (and even more explicitness), make a private method > that tests the invariant (like rtc::Buffer has) and DCHECK it everywhere. But > I'll let you decide if you want that. Done
lgtm
On 2016/02/24 11:49:41, kwiberg-webrtc wrote: > lgtm Thanks! @tommi: Ptal for owner review.
The CQ bit was checked by tommi@webrtc.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1697743003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1697743003/180001
Message was sent while issue was closed.
Description was changed from ========== Add CopyOnWriteBuffer class This CL introduces a new class CopyOnWriteBuffer that holds data in a refcounted Buffer which is shared between copied CopyOnWriteBuffer to avoid unnecessary allocations / memory copies. BUG=webrtc:5155 ========== to ========== Add CopyOnWriteBuffer class This CL introduces a new class CopyOnWriteBuffer that holds data in a refcounted Buffer which is shared between copied CopyOnWriteBuffer to avoid unnecessary allocations / memory copies. BUG=webrtc:5155 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add CopyOnWriteBuffer class This CL introduces a new class CopyOnWriteBuffer that holds data in a refcounted Buffer which is shared between copied CopyOnWriteBuffer to avoid unnecessary allocations / memory copies. BUG=webrtc:5155 ========== to ========== Add CopyOnWriteBuffer class This CL introduces a new class CopyOnWriteBuffer that holds data in a refcounted Buffer which is shared between copied CopyOnWriteBuffer to avoid unnecessary allocations / memory copies. BUG=webrtc:5155 Committed: https://crrev.com/13041cf11f1c2691bb3ddc7d28e74cae9333b685 Cr-Commit-Position: refs/heads/master@{#11767} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/13041cf11f1c2691bb3ddc7d28e74cae9333b685 Cr-Commit-Position: refs/heads/master@{#11767} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
