Add a ref-counted data holder to ArrayBufferContents

Add a ref-counted data holder to ArrayBufferContents

This is in preparation for adding the DOMSharedArrayBuffer object, which needs
to be able to share allocated array memory between multiple WTF::ArrayBuffers.

BUG=497295
R=jochen@chromium.org,haraken@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198023

[binji, 2015-06-18 19:38:16]

Separated out from https://codereview.chromium.org/1097773004, PTAL

[haraken, 2015-06-19 12:05:12]

Thanks for spliting the patch!

https://codereview.chromium.org/1186863005/diff/1/Source/wtf/ArrayBufferConte...
File Source/wtf/ArrayBufferContents.cpp (right):

https://codereview.chromium.org/1186863005/diff/1/Source/wtf/ArrayBufferConte...
Source/wtf/ArrayBufferContents.cpp:60: if (!data) {

Avoid using if(!...) {} else {} pattern.

https://codereview.chromium.org/1186863005/diff/1/Source/wtf/ArrayBufferConte...
Source/wtf/ArrayBufferContents.cpp:64: // (partitionAllocGeneric guarantees
valid pointer for size 0)

Nit: partitionAllocGeneric => PartitionAlloc (ArrayBuffer uses a dedicated
partition for ArrayBuffers. It's different from partitionAllocGeneric.)

https://codereview.chromium.org/1186863005/diff/1/Source/wtf/ArrayBufferConte...
Source/wtf/ArrayBufferContents.cpp:67: m_holder->adopt(data, sizeInBytes,
isShared);

Help me understand: Why isn't this allocateNew? If isShared is NotShared, I
guess we need to call allocateNew.

https://codereview.chromium.org/1186863005/diff/1/Source/wtf/ArrayBufferConte...
Source/wtf/ArrayBufferContents.cpp:78: m_holder.clear();

I think it's better to introduce DataHolder::clear. We can call
m_holder->clear() here. (See the below comment.)

https://codereview.chromium.org/1186863005/diff/1/Source/wtf/ArrayBufferConte...
Source/wtf/ArrayBufferContents.cpp:85: if (!m_holder->isShared())

It would be better to just call clear() here and let DataHolder::clear() decide
what to do there (i.e., not clear the data is the data is a shared one).

Question: Why don't we clear the data if the data is a shared one?

https://codereview.chromium.org/1186863005/diff/1/Source/wtf/ArrayBufferConte...
Source/wtf/ArrayBufferContents.cpp:91: ASSERT(!m_holder->isShared() &&
!other.m_holder->isShared());

Is this a TODO? Or are you not planning to support copyTo for shared buffers?

https://codereview.chromium.org/1186863005/diff/1/Source/wtf/ArrayBufferConte...
Source/wtf/ArrayBufferContents.cpp:100: if (policy == ZeroInitialize && data)

Not related to this CL, but I think we should just crash here if PartitionAlloc
failed at allocating the data. Continuing the execution won't make much sense
and will just lead to crashes in other places (which are hard to investigate).

https://codereview.chromium.org/1186863005/diff/1/Source/wtf/ArrayBufferConte...
Source/wtf/ArrayBufferContents.cpp:116:
ArrayBufferContents::DataHolder::~DataHolder()

You can do this in DataHolder::clear().

https://codereview.chromium.org/1186863005/diff/1/Source/wtf/ArrayBufferConte...
Source/wtf/ArrayBufferContents.cpp:135: void
ArrayBufferContents::DataHolder::adopt(void* data, unsigned sizeInBytes,
SharingType isShared)

Question: What is the purpose of adopt?

https://codereview.chromium.org/1186863005/diff/1/Source/wtf/ArrayBufferConte...
File Source/wtf/ArrayBufferContents.h (right):

https://codereview.chromium.org/1186863005/diff/1/Source/wtf/ArrayBufferConte...
Source/wtf/ArrayBufferContents.h:68: bool isShared() const { return m_holder ?
m_holder->isShared() : false; }

If we introduce DataHolder::clear(), we won't need the m_holder here. (See the
comment in ArrayBufferContents.cpp.)

[binji, 2015-06-19 18:14:57]

My apologies if I'm misunderstanding your suggestions.

https://codereview.chromium.org/1186863005/diff/1/Source/wtf/ArrayBufferConte...
File Source/wtf/ArrayBufferContents.cpp (right):

https://codereview.chromium.org/1186863005/diff/1/Source/wtf/ArrayBufferConte...
Source/wtf/ArrayBufferContents.cpp:60: if (!data) {
On 2015/06/19 at 12:05:11, haraken wrote:
> Avoid using if(!...) {} else {} pattern.

Done.

https://codereview.chromium.org/1186863005/diff/1/Source/wtf/ArrayBufferConte...
Source/wtf/ArrayBufferContents.cpp:64: // (partitionAllocGeneric guarantees
valid pointer for size 0)
On 2015/06/19 at 12:05:11, haraken wrote:
> Nit: partitionAllocGeneric => PartitionAlloc (ArrayBuffer uses a dedicated
partition for ArrayBuffers. It's different from partitionAllocGeneric.)

Done.

https://codereview.chromium.org/1186863005/diff/1/Source/wtf/ArrayBufferConte...
Source/wtf/ArrayBufferContents.cpp:67: m_holder->adopt(data, sizeInBytes,
isShared);
On 2015/06/19 at 12:05:11, haraken wrote:
> Help me understand: Why isn't this allocateNew? If isShared is NotShared, I
guess we need to call allocateNew.

The previous behavior was to assign data to m_data without allocation if data is
not NULL. This keeps that behavior.

https://codereview.chromium.org/1186863005/diff/1/Source/wtf/ArrayBufferConte...
Source/wtf/ArrayBufferContents.cpp:78: m_holder.clear();
On 2015/06/19 at 12:05:12, haraken wrote:
> I think it's better to introduce DataHolder::clear. We can call
m_holder->clear() here. (See the below comment.)

I think that has the wrong behavior if this ArrayBufferContents is shared. I
would not expect clear to clear the data from another buffer.

I did a compile check, and this function is only used in two places:
ArrayBuffer::transfer and ArrayBufferContents::transfer. In both cases it is
meant to be equivalent to "neuter". I've changed the name for clarity.

https://codereview.chromium.org/1186863005/diff/1/Source/wtf/ArrayBufferConte...
Source/wtf/ArrayBufferContents.cpp:85: if (!m_holder->isShared())
On 2015/06/19 at 12:05:12, haraken wrote:
> It would be better to just call clear() here and let DataHolder::clear()
decide what to do there (i.e., not clear the data is the data is a shared one).

AFAICT, we never want to clear() once we have shared contents, because we never
know what the other owner(s) want to do with the data. So I don't think adding
DataHolder::clear makes sense.

> 
> Question: Why don't we clear the data if the data is a shared one?

A shared ArrayBuffer needs to share its contents with another ArrayBuffer via
the ref-counted DataHolder. So if it cleared the data (or neutered itself,
removing its reference to the data), it wouldn't be shared anymore.

https://codereview.chromium.org/1186863005/diff/1/Source/wtf/ArrayBufferConte...
Source/wtf/ArrayBufferContents.cpp:91: ASSERT(!m_holder->isShared() &&
!other.m_holder->isShared());
On 2015/06/19 at 12:05:12, haraken wrote:
> Is this a TODO? Or are you not planning to support copyTo for shared buffers?

AFAICT, this is only used by ArrayBuffer::transfer when the ArrayBuffer being
copied from cannot be neutered. That can never happen with SharedArrayBuffers,
so this function should never be called by SharedArrayBuffers. I suppose I can
write the functionality for it, but it would be untested.

https://codereview.chromium.org/1186863005/diff/1/Source/wtf/ArrayBufferConte...
Source/wtf/ArrayBufferContents.cpp:135: void
ArrayBufferContents::DataHolder::adopt(void* data, unsigned sizeInBytes,
SharingType isShared)
On 2015/06/19 at 12:05:12, haraken wrote:
> Question: What is the purpose of adopt?

To take over the passed in |data| pointer, without allocating new memory.

[haraken, 2015-06-20 00:59:31]

> My apologies if I'm misunderstanding your suggestions.

No, no, I was misunderstanding. Thanks to your explanations, now I understand
the implementation better :)

https://codereview.chromium.org/1186863005/diff/20001/Source/wtf/ArrayBufferC...
File Source/wtf/ArrayBufferContents.cpp (right):

https://codereview.chromium.org/1186863005/diff/20001/Source/wtf/ArrayBufferC...
Source/wtf/ArrayBufferContents.cpp:86: neuter();

I'm wondering if we should call neuter() for a shared buffer as well.

Imagine that you have shared ArrayBufferContents X1 and X2 which share the
DataHolder C. When X1 is transfered to another ArrayBufferContents, we want to
drop a reference to C (meaning that we want to call neuter()). When X2 is
transfered to another ArrayBufferContents, we want to drop another reference to
C, at which point the DataHolder loses all referece counts and thus the
DataHolder gets collected.

So I think what we want to do is something like:

ArrayBufferContents::~ArrayBufferContents()
{
} // m_holder.clear() is implicitly called.

void ArrayBufferContents::transfer(ArrayBufferContents& other)
{
  ...;
  m_holder.clear();
}

[binji, 2015-06-20 01:36:21]

https://codereview.chromium.org/1186863005/diff/20001/Source/wtf/ArrayBufferC...
File Source/wtf/ArrayBufferContents.cpp (right):

https://codereview.chromium.org/1186863005/diff/20001/Source/wtf/ArrayBufferC...
Source/wtf/ArrayBufferContents.cpp:86: neuter();
On 2015/06/20 at 00:59:31, haraken wrote:
> I'm wondering if we should call neuter() for a shared buffer as well.
> 
> Imagine that you have shared ArrayBufferContents X1 and X2 which share the
DataHolder C. When X1 is transfered to another ArrayBufferContents, we want to
drop a reference to C (meaning that we want to call neuter()). When X2 is
transfered to another ArrayBufferContents, we want to drop another reference to
C, at which point the DataHolder loses all referece counts and thus the
DataHolder gets collected.
> 
> So I think what we want to do is something like:
> 
> ArrayBufferContents::~ArrayBufferContents()
> {
> } // m_holder.clear() is implicitly called.
> 
> void ArrayBufferContents::transfer(ArrayBufferContents& other)
> {
>   ...;
>   m_holder.clear();
> }

Right, but we shouldn't ever be transferring a shared ArrayBufferContents and
neutering it; they should never be neutered.

I think the problem is that the ArrayBufferContents::transfer() function is
actually doing two things:
1) share this data holder with another ArrayBufferContents
2) neuter this data holder

And I'm explicitly blocking the second one in transfer. So now it is confusing
when you call transfer(). Is it neutering or not?

It's clearer if I make transfer() only do step #1 above, and have an explicit
call to neuter afterward, only in the case when the ArrayBuffer is not shared.

I've implemented that in patchset #3.

[haraken, 2015-06-20 08:35:52]

On 2015/06/20 01:36:21, binji wrote:
>
https://codereview.chromium.org/1186863005/diff/20001/Source/wtf/ArrayBufferC...
> File Source/wtf/ArrayBufferContents.cpp (right):
> 
>
https://codereview.chromium.org/1186863005/diff/20001/Source/wtf/ArrayBufferC...
> Source/wtf/ArrayBufferContents.cpp:86: neuter();
> On 2015/06/20 at 00:59:31, haraken wrote:
> > I'm wondering if we should call neuter() for a shared buffer as well.
> > 
> > Imagine that you have shared ArrayBufferContents X1 and X2 which share the
> DataHolder C. When X1 is transfered to another ArrayBufferContents, we want to
> drop a reference to C (meaning that we want to call neuter()). When X2 is
> transfered to another ArrayBufferContents, we want to drop another reference
to
> C, at which point the DataHolder loses all referece counts and thus the
> DataHolder gets collected.
> > 
> > So I think what we want to do is something like:
> > 
> > ArrayBufferContents::~ArrayBufferContents()
> > {
> > } // m_holder.clear() is implicitly called.
> > 
> > void ArrayBufferContents::transfer(ArrayBufferContents& other)
> > {
> >   ...;
> >   m_holder.clear();
> > }
> 
> Right, but we shouldn't ever be transferring a shared ArrayBufferContents and
> neutering it; they should never be neutered.
> 
> I think the problem is that the ArrayBufferContents::transfer() function is
> actually doing two things:
> 1) share this data holder with another ArrayBufferContents
> 2) neuter this data holder
> 
> And I'm explicitly blocking the second one in transfer. So now it is confusing
> when you call transfer(). Is it neutering or not?
> 
> It's clearer if I make transfer() only do step #1 above, and have an explicit
> call to neuter afterward, only in the case when the ArrayBuffer is not shared.
> 
> I've implemented that in patchset #3.

Thanks, help me understand a bit more:

When you use a shared buffer, there are N ArrayBufferContents that are pointing
to one DataHolder, right? If that is the case, what's an issue of calling
m_holder.clear() in transfer()? Even if you call it, it will just decrease one
reference to the DataHolder. The DataHolder isn't destructed until it loses all
references to it.

[binji, 2015-06-22 17:17:34]

On 2015/06/20 at 08:35:52, haraken wrote:
> Thanks, help me understand a bit more:
> 
> When you use a shared buffer, there are N ArrayBufferContents that are
pointing to one DataHolder, right? If that is the case, what's an issue of
calling m_holder.clear() in transfer()? Even if you call it, it will just
decrease one reference to the DataHolder. The DataHolder isn't destructed until
it loses all references to it.

Right, there's no issue in calling m_holder.clear(), aside from the fact that
that ArrayBufferContents object won't be able to access the memory anymore.
Here's the basic idea:

  var ab = new ArrayBuffer(100);
  var sab = new SharedArrayBuffer(100);

  var worker = new Worker(...);
  
  worker.postMessage(ab);          // 1. Copy the contents of the ArrayBuffer to
the worker. |ab| is not neutered.
  worker.postMessage(ab, [ab]);    // 2. Transfer the contents of the
ArrayBuffer to the worker. |ab| is neutered.
  worker.postMessage(sab);         // 3. Not allowed.
  worker.postMessage(sab, [sab]);  // 4. Share the contents of the
SharedArrayBuffer with the worker. |sab| is not neutered.

Cases 2 and 4 doesn't call ArrayBuffer::transfer. Case 2 must neuter the
ArrayBuffer (and all associated views). Case 4 must not or its buffer wouldn't
be shared.
Case 1 doesn't call ArrayBuffer::transfer, instead the contents of the
ArrayBuffer are serialized directly.

[haraken, 2015-06-23 00:19:18]

On 2015/06/22 17:17:34, binji wrote:
> On 2015/06/20 at 08:35:52, haraken wrote:
> > Thanks, help me understand a bit more:
> > 
> > When you use a shared buffer, there are N ArrayBufferContents that are
> pointing to one DataHolder, right? If that is the case, what's an issue of
> calling m_holder.clear() in transfer()? Even if you call it, it will just
> decrease one reference to the DataHolder. The DataHolder isn't destructed
until
> it loses all references to it.
> 
> Right, there's no issue in calling m_holder.clear(), aside from the fact that
> that ArrayBufferContents object won't be able to access the memory anymore.
> Here's the basic idea:
> 
>   var ab = new ArrayBuffer(100);
>   var sab = new SharedArrayBuffer(100);
> 
>   var worker = new Worker(...);
>   
>   worker.postMessage(ab);          // 1. Copy the contents of the ArrayBuffer
to
> the worker. |ab| is not neutered.
>   worker.postMessage(ab, [ab]);    // 2. Transfer the contents of the
> ArrayBuffer to the worker. |ab| is neutered.
>   worker.postMessage(sab);         // 3. Not allowed.
>   worker.postMessage(sab, [sab]);  // 4. Share the contents of the
> SharedArrayBuffer with the worker. |sab| is not neutered.
> 
> Cases 2 and 4 doesn't call ArrayBuffer::transfer. Case 2 must neuter the
> ArrayBuffer (and all associated views). Case 4 must not or its buffer wouldn't
> be shared.
> Case 1 doesn't call ArrayBuffer::transfer, instead the contents of the
> ArrayBuffer are serialized directly.

Thanks, now I understand the point :) LGTM!

> Cases 2 and 4 doesn't call ArrayBuffer::transfer. Case 2 must neuter the
> ArrayBuffer (and all associated views). Case 4 must not or its buffer wouldn't
> be shared.

^^^ Just to clarify: Cases 2 and 4 *call* ArrayBuffer::transfer, right?

[binji, 2015-06-23 04:36:03]

On 2015/06/23 at 00:19:18, haraken wrote:
> On 2015/06/22 17:17:34, binji wrote:
> > On 2015/06/20 at 08:35:52, haraken wrote:
> > > Thanks, help me understand a bit more:
> > > 
> > > When you use a shared buffer, there are N ArrayBufferContents that are
> > pointing to one DataHolder, right? If that is the case, what's an issue of
> > calling m_holder.clear() in transfer()? Even if you call it, it will just
> > decrease one reference to the DataHolder. The DataHolder isn't destructed
until
> > it loses all references to it.
> > 
> > Right, there's no issue in calling m_holder.clear(), aside from the fact
that
> > that ArrayBufferContents object won't be able to access the memory anymore.
> > Here's the basic idea:
> > 
> >   var ab = new ArrayBuffer(100);
> >   var sab = new SharedArrayBuffer(100);
> > 
> >   var worker = new Worker(...);
> >   
> >   worker.postMessage(ab);          // 1. Copy the contents of the
ArrayBuffer to
> > the worker. |ab| is not neutered.
> >   worker.postMessage(ab, [ab]);    // 2. Transfer the contents of the
> > ArrayBuffer to the worker. |ab| is neutered.
> >   worker.postMessage(sab);         // 3. Not allowed.
> >   worker.postMessage(sab, [sab]);  // 4. Share the contents of the
> > SharedArrayBuffer with the worker. |sab| is not neutered.
> > 
> > Cases 2 and 4 doesn't call ArrayBuffer::transfer. Case 2 must neuter the
> > ArrayBuffer (and all associated views). Case 4 must not or its buffer
wouldn't
> > be shared.
> > Case 1 doesn't call ArrayBuffer::transfer, instead the contents of the
> > ArrayBuffer are serialized directly.
> 
> Thanks, now I understand the point :) LGTM!
> 
> > Cases 2 and 4 doesn't call ArrayBuffer::transfer. Case 2 must neuter the
> > ArrayBuffer (and all associated views). Case 4 must not or its buffer
wouldn't
> > be shared.
> 
> ^^^ Just to clarify: Cases 2 and 4 *call* ArrayBuffer::transfer, right?

Oops, yeah. That was a typo. :)

[kinuko, 2015-06-23 10:44:37]

Drive-by nit comment (looking good in general, btw)

https://codereview.chromium.org/1186863005/diff/40001/Source/wtf/ArrayBufferC...
File Source/wtf/ArrayBufferContents.cpp (right):

https://codereview.chromium.org/1186863005/diff/40001/Source/wtf/ArrayBufferC...
Source/wtf/ArrayBufferContents.cpp:83: other.m_holder = m_holder;
Transfer used to mean transfering ownership and not having neuter() here feels a
bit weird.  I see that SharedArrayBuffer extends the definition of 'transfer',
but ArrayBuffer::transfer() still behaves in the same way as it used to for
non-shared one, while this one does not, which seems inconsistent.

Maybe just keeping the neutering code as is but add a explanatory comment (for
why we don't neuter() for shared one) would be enough?  Or could we use
different method name from 'transfer' to avoid confusion?

[binji, 2015-06-23 19:12:18]

On 2015/06/23 at 10:44:37, kinuko wrote:
> Drive-by nit comment (looking good in general, btw)
> 
>
https://codereview.chromium.org/1186863005/diff/40001/Source/wtf/ArrayBufferC...
> File Source/wtf/ArrayBufferContents.cpp (right):
> 
>
https://codereview.chromium.org/1186863005/diff/40001/Source/wtf/ArrayBufferC...
> Source/wtf/ArrayBufferContents.cpp:83: other.m_holder = m_holder;
> Transfer used to mean transfering ownership and not having neuter() here feels
a bit weird.  I see that SharedArrayBuffer extends the definition of 'transfer',
but ArrayBuffer::transfer() still behaves in the same way as it used to for
non-shared one, while this one does not, which seems inconsistent.
> 
> Maybe just keeping the neutering code as is but add a explanatory comment (for
why we don't neuter() for shared one) would be enough?  Or could we use
different method name from 'transfer' to avoid confusion?

You're right, creating a new function makes this clearer. I added two new
functions ArrayBuffer::shareContentsWith and ArrayBufferContents::shareWith.

[kinuko, 2015-06-24 03:05:34]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2015-06-24 03:05:37]

Feels easier to follow, thanks.

https://codereview.chromium.org/1186863005/diff/60001/Source/wtf/ArrayBuffer.cpp
File Source/wtf/ArrayBuffer.cpp (right):

https://codereview.chromium.org/1186863005/diff/60001/Source/wtf/ArrayBuffer....
Source/wtf/ArrayBuffer.cpp:80: m_contents.transfer(result);
Did you mean to call shareWith here?

[binji, 2015-06-24 04:41:36]

https://codereview.chromium.org/1186863005/diff/60001/Source/wtf/ArrayBuffer.cpp
File Source/wtf/ArrayBuffer.cpp (right):

https://codereview.chromium.org/1186863005/diff/60001/Source/wtf/ArrayBuffer....
Source/wtf/ArrayBuffer.cpp:80: m_contents.transfer(result);
On 2015/06/24 at 03:05:36, kinuko wrote:
> Did you mean to call shareWith here?

Oops, yes. Thanks!

[kinuko, 2015-06-24 06:59:08]

thanks, non-owner lgtm

https://codereview.chromium.org/1186863005/diff/80001/Source/wtf/ArrayBufferC...
File Source/wtf/ArrayBufferContents.cpp (right):

https://codereview.chromium.org/1186863005/diff/80001/Source/wtf/ArrayBufferC...
Source/wtf/ArrayBufferContents.cpp:128: m_isShared = NotShared;
Any reason we explicitly clear these in dtor?

[binji, 2015-06-24 17:52:43]

https://codereview.chromium.org/1186863005/diff/80001/Source/wtf/ArrayBufferC...
File Source/wtf/ArrayBufferContents.cpp (right):

https://codereview.chromium.org/1186863005/diff/80001/Source/wtf/ArrayBufferC...
Source/wtf/ArrayBufferContents.cpp:128: m_isShared = NotShared;
On 2015/06/24 at 06:59:08, kinuko wrote:
> Any reason we explicitly clear these in dtor?

No reason, though my guess is that I did it because the ArrayBuffer destructor
used to call clear() too.

[binji, 2015-06-25 19:34:14]

jochen: can I get an OWNERS review for Source/bindings and Source/wtf?

[jochen (gone - plz use gerrit), 2015-06-26 14:55:11]

lgtm

[binji, 2015-06-29 17:55:36]

The CQ bit was checked by binji@chromium.org

[binji, 2015-06-29 17:55:37]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1186863005/#ps80001
(title: "type in ArrayBuffer::shareContentsWith")

[commit-bot: I haz the power, 2015-06-29 17:55:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1186863005/80001

[commit-bot: I haz the power, 2015-06-29 20:56:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-29 20:56:06]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/68666)

[binji, 2015-06-29 21:20:41]

The CQ bit was checked by binji@chromium.org

[commit-bot: I haz the power, 2015-06-29 21:21:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1186863005/80001

[commit-bot: I haz the power, 2015-06-29 22:07:13]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198023