Remove ArrayBufferDeallocationObserver

Remove ArrayBufferDeallocationObserver

It was introduced a long time ago because we didn't want to report memory of
ArrayBuffers that do not (yet) have wrappers to V8. The underlying idea was
that we don't want to report memory that is not guaranteed to be freed by V8 GC
to V8. However, the assumption has changed, and now we're reporting Blink's
memory to V8 even if the memory is not guaranteed to be freed by V8 GC.

BUG=none
R=haraken@chromium.org

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

[binji, 2015-06-12 20:11:15]

Is this what you meant by your comment in
https://codereview.chromium.org/1097773004?

I copied your explanation from there for the CL description, I hope that's OK.
:)

[binji, 2015-06-12 20:37:18]

On 2015/06/12 at 20:11:15, binji wrote:
> Is this what you meant by your comment in
https://codereview.chromium.org/1097773004?
> 
> I copied your explanation from there for the CL description, I hope that's OK.
:)

Hm, seems that WTF cannot reference v8.h directly.

[haraken, 2015-06-13 03:32:03]

haraken@chromium.org changed reviewers:
+ yukishiino@chromium.org

[haraken, 2015-06-13 03:32:04]

Thanks for working on this!

> Hm, seems that WTF cannot reference v8.h directly.

Right, wtf/ cannot depend on v8/.

yukishiino@: Is there any reason we need to keep ArrayBuffer and
ArrayBufferContents in wtf/? Can we probably move them to platform/? (Then
ArrayBufferContents can depend on v8/.)

[Yuki, 2015-06-15 04:49:18]

On 2015/06/13 03:32:04, haraken wrote:
> Thanks for working on this!
> 
> > Hm, seems that WTF cannot reference v8.h directly.
> 
> Right, wtf/ cannot depend on v8/.
> 
> yukishiino@: Is there any reason we need to keep ArrayBuffer and
> ArrayBufferContents in wtf/? Can we probably move them to platform/? (Then
> ArrayBufferContents can depend on v8/.)

wtf/ArrayBuffer and wtf/ArrayBufferView should be migrated to
core/dom/DOMArrayBuffer and core/dom/DOMArrayBufferView, but I've not had enough
time to work on it.  If someone can do it, I appreciate very much.

For ArrayBufferContents, some code in platform/ (graphics-related code) needs to
transfer the data from/to DOMArrayBuffer, so it's hard to move it to
core/dom/DOMArrayBufferContents.

I'm not sure, but it's possible that some code in wtf/ also depends on
ArrayBufferContents.  It may not be easy to move ArrayBufferContents from wtf/
to platform/.

For ArrayBufferContents and v8.h, ArrayBufferDeallocationObserver uses Injection
pattern.  wtf/ provides a pure interface, and
core/dom/DOMArrayBufferDeallocationObserver implements it with v8.h.

Why does ArrayBufferContents need to depend on v8.h?  If DeallocationObserver is
gone, why does it depend on v8.h?

[haraken, 2015-06-15 04:53:29]

On 2015/06/15 04:49:18, Yuki wrote:
> On 2015/06/13 03:32:04, haraken wrote:
> > Thanks for working on this!
> > 
> > > Hm, seems that WTF cannot reference v8.h directly.
> > 
> > Right, wtf/ cannot depend on v8/.
> > 
> > yukishiino@: Is there any reason we need to keep ArrayBuffer and
> > ArrayBufferContents in wtf/? Can we probably move them to platform/? (Then
> > ArrayBufferContents can depend on v8/.)
> 
> wtf/ArrayBuffer and wtf/ArrayBufferView should be migrated to
> core/dom/DOMArrayBuffer and core/dom/DOMArrayBufferView, but I've not had
enough
> time to work on it.  If someone can do it, I appreciate very much.
> 
> For ArrayBufferContents, some code in platform/ (graphics-related code) needs
to
> transfer the data from/to DOMArrayBuffer, so it's hard to move it to
> core/dom/DOMArrayBufferContents.
> 
> I'm not sure, but it's possible that some code in wtf/ also depends on
> ArrayBufferContents.  It may not be easy to move ArrayBufferContents from wtf/
> to platform/.
> 
> For ArrayBufferContents and v8.h, ArrayBufferDeallocationObserver uses
Injection
> pattern.  wtf/ provides a pure interface, and
> core/dom/DOMArrayBufferDeallocationObserver implements it with v8.h.
> 
> Why does ArrayBufferContents need to depend on v8.h?  If DeallocationObserver
is
> gone, why does it depend on v8.h?

We need to call v8::AdjustAmountOfMemory() when the ArrayBufferContents
allocates/deallocates the buffer.

DeallocationObserver was used to control the timing when
v8::AdjustAmountOfMemory() should be called (i.e., we wanted to call
v8::AdjustAmountOfMemory() when the wrapper of ArrayBuffer is created). After
this CL, v8::AdjustAmountOfMemory() is going to be called every time the
underlying buffer is allocated/deallocated.

[Yuki, 2015-06-15 05:09:45]

On 2015/06/15 04:49:18, Yuki wrote:
> On 2015/06/13 03:32:04, haraken wrote:
> > Thanks for working on this!
> > 
> > > Hm, seems that WTF cannot reference v8.h directly.
> > 
> > Right, wtf/ cannot depend on v8/.
> > 
> > yukishiino@: Is there any reason we need to keep ArrayBuffer and
> > ArrayBufferContents in wtf/? Can we probably move them to platform/? (Then
> > ArrayBufferContents can depend on v8/.)
> 
> wtf/ArrayBuffer and wtf/ArrayBufferView should be migrated to
> core/dom/DOMArrayBuffer and core/dom/DOMArrayBufferView, but I've not had
enough
> time to work on it.  If someone can do it, I appreciate very much.
> 
> For ArrayBufferContents, some code in platform/ (graphics-related code) needs
to
> transfer the data from/to DOMArrayBuffer, so it's hard to move it to
> core/dom/DOMArrayBufferContents.
> 
> I'm not sure, but it's possible that some code in wtf/ also depends on
> ArrayBufferContents.  It may not be easy to move ArrayBufferContents from wtf/
> to platform/.
> 
> For ArrayBufferContents and v8.h, ArrayBufferDeallocationObserver uses
Injection
> pattern.  wtf/ provides a pure interface, and
> core/dom/DOMArrayBufferDeallocationObserver implements it with v8.h.
> 
> Why does ArrayBufferContents need to depend on v8.h?  If DeallocationObserver
is
> gone, why does it depend on v8.h?

Sorry for commenting without reading the code.  I took a quick look at the CL,
and saw that you just moved
v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory to
wtf/ArrayBufferContents.

If this is the only reason, I'd recommend to use Injection pattern as same as
before because it should be easier to move ArrayBufferContents to platform/.

wtf/ArrayBufferContents:
  static void (*arrayBufferAllocationListener)(int size);
  static void SetAllocationListener(listener) {
    CHECK(!arrayBufferAllocationListener);
    arrayBufferAllocationListener = listener;
  }

static initializer in core/ (I don't know what's the most appropriate class.):
  void staticInitializer() {
   
wtf::ArrayBufferContents::SetAllocationListener(arrayBufferAllocationListener);
  }
  void arrayBufferAllocationListener(int size) {
    v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(size);
  }

It may not look great, but it's straight forward.

[haraken, 2015-06-15 05:17:08]

On 2015/06/15 05:09:45, Yuki wrote:
> On 2015/06/15 04:49:18, Yuki wrote:
> > On 2015/06/13 03:32:04, haraken wrote:
> > > Thanks for working on this!
> > > 
> > > > Hm, seems that WTF cannot reference v8.h directly.
> > > 
> > > Right, wtf/ cannot depend on v8/.
> > > 
> > > yukishiino@: Is there any reason we need to keep ArrayBuffer and
> > > ArrayBufferContents in wtf/? Can we probably move them to platform/? (Then
> > > ArrayBufferContents can depend on v8/.)
> > 
> > wtf/ArrayBuffer and wtf/ArrayBufferView should be migrated to
> > core/dom/DOMArrayBuffer and core/dom/DOMArrayBufferView, but I've not had
> enough
> > time to work on it.  If someone can do it, I appreciate very much.
> > 
> > For ArrayBufferContents, some code in platform/ (graphics-related code)
needs
> to
> > transfer the data from/to DOMArrayBuffer, so it's hard to move it to
> > core/dom/DOMArrayBufferContents.
> > 
> > I'm not sure, but it's possible that some code in wtf/ also depends on
> > ArrayBufferContents.  It may not be easy to move ArrayBufferContents from
wtf/
> > to platform/.
> > 
> > For ArrayBufferContents and v8.h, ArrayBufferDeallocationObserver uses
> Injection
> > pattern.  wtf/ provides a pure interface, and
> > core/dom/DOMArrayBufferDeallocationObserver implements it with v8.h.
> > 
> > Why does ArrayBufferContents need to depend on v8.h?  If
DeallocationObserver
> is
> > gone, why does it depend on v8.h?
> 
> Sorry for commenting without reading the code.  I took a quick look at the CL,
> and saw that you just moved
> v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory to
> wtf/ArrayBufferContents.
> 
> If this is the only reason, I'd recommend to use Injection pattern as same as
> before because it should be easier to move ArrayBufferContents to platform/.
> 
> wtf/ArrayBufferContents:
>   static void (*arrayBufferAllocationListener)(int size);
>   static void SetAllocationListener(listener) {
>     CHECK(!arrayBufferAllocationListener);
>     arrayBufferAllocationListener = listener;
>   }
> 
> static initializer in core/ (I don't know what's the most appropriate class.):
>   void staticInitializer() {
>    
>
wtf::ArrayBufferContents::SetAllocationListener(arrayBufferAllocationListener);
>   }
>   void arrayBufferAllocationListener(int size) {
>     v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(size);
>   }
> 
> It may not look great, but it's straight forward.

Yes, if it's not easy to move the ArrayBufferContents to platform/ and you have
some plan to find a better home for the ArrayBufferContents, we can just use the
inject pattern in this CL.

[binji, 2015-06-15 17:28:24]

On 2015/06/15 at 05:17:08, haraken wrote:
> On 2015/06/15 05:09:45, Yuki wrote:
> > On 2015/06/15 04:49:18, Yuki wrote:
> > > On 2015/06/13 03:32:04, haraken wrote:
> > > > Thanks for working on this!
> > > > 
> > > > > Hm, seems that WTF cannot reference v8.h directly.
> > > > 
> > > > Right, wtf/ cannot depend on v8/.
> > > > 
> > > > yukishiino@: Is there any reason we need to keep ArrayBuffer and
> > > > ArrayBufferContents in wtf/? Can we probably move them to platform/?
(Then
> > > > ArrayBufferContents can depend on v8/.)
> > > 
> > > wtf/ArrayBuffer and wtf/ArrayBufferView should be migrated to
> > > core/dom/DOMArrayBuffer and core/dom/DOMArrayBufferView, but I've not had
> > enough
> > > time to work on it.  If someone can do it, I appreciate very much.
> > > 
> > > For ArrayBufferContents, some code in platform/ (graphics-related code)
needs
> > to
> > > transfer the data from/to DOMArrayBuffer, so it's hard to move it to
> > > core/dom/DOMArrayBufferContents.
> > > 
> > > I'm not sure, but it's possible that some code in wtf/ also depends on
> > > ArrayBufferContents.  It may not be easy to move ArrayBufferContents from
wtf/
> > > to platform/.
> > > 
> > > For ArrayBufferContents and v8.h, ArrayBufferDeallocationObserver uses
> > Injection
> > > pattern.  wtf/ provides a pure interface, and
> > > core/dom/DOMArrayBufferDeallocationObserver implements it with v8.h.
> > > 
> > > Why does ArrayBufferContents need to depend on v8.h?  If
DeallocationObserver
> > is
> > > gone, why does it depend on v8.h?
> > 
> > Sorry for commenting without reading the code.  I took a quick look at the
CL,
> > and saw that you just moved
> > v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory to
> > wtf/ArrayBufferContents.
> > 
> > If this is the only reason, I'd recommend to use Injection pattern as same
as
> > before because it should be easier to move ArrayBufferContents to platform/.
> > 
> > wtf/ArrayBufferContents:
> >   static void (*arrayBufferAllocationListener)(int size);
> >   static void SetAllocationListener(listener) {
> >     CHECK(!arrayBufferAllocationListener);
> >     arrayBufferAllocationListener = listener;
> >   }
> > 
> > static initializer in core/ (I don't know what's the most appropriate
class.):
> >   void staticInitializer() {
> >    
> >
wtf::ArrayBufferContents::SetAllocationListener(arrayBufferAllocationListener);
> >   }
> >   void arrayBufferAllocationListener(int size) {
> >     v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(size);
> >   }
> > 
> > It may not look great, but it's straight forward.
> 
> Yes, if it's not easy to move the ArrayBufferContents to platform/ and you
have some plan to find a better home for the ArrayBufferContents, we can just
use the inject pattern in this CL.

Done, PTAL

[haraken, 2015-06-15 17:33:29]

https://codereview.chromium.org/1183763003/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/1183763003/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Initializer.cpp:408:
WTF::ArrayBufferContents::setAllocationListener(arrayBufferContentsAllocationListener);

I'd move this to initializeWithoutV8 in WebKit.cpp. See how
histogramEnumerationFunction in WebKit.cpp works with wtf/PartitionAlloc. We can
do a similar thing.

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

https://codereview.chromium.org/1183763003/diff/20001/Source/wtf/ArrayBufferC...
Source/wtf/ArrayBufferContents.cpp:105:
allocationListener(static_cast<int>(size));

Just to confirm: Is 'int' enough?

[binji, 2015-06-15 19:19:46]

https://codereview.chromium.org/1183763003/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/1183763003/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Initializer.cpp:408:
WTF::ArrayBufferContents::setAllocationListener(arrayBufferContentsAllocationListener);
On 2015/06/15 at 17:33:29, haraken wrote:
> I'd move this to initializeWithoutV8 in WebKit.cpp. See how
histogramEnumerationFunction in WebKit.cpp works with wtf/PartitionAlloc. We can
do a similar thing.

Done.

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

https://codereview.chromium.org/1183763003/diff/20001/Source/wtf/ArrayBufferC...
Source/wtf/ArrayBufferContents.cpp:105:
allocationListener(static_cast<int>(size));
On 2015/06/15 at 17:33:29, haraken wrote:
> Just to confirm: Is 'int' enough?

I assume so: the DOMArrayBufferDeallocationObserver functions also did a
static_cast to int.

[haraken, 2015-06-15 20:05:25]

haraken@chromium.org changed reviewers:
+ tkent@chromium.org

[haraken, 2015-06-15 20:05:26]

LGTM

tkent-san for wtf/.

https://codereview.chromium.org/1183763003/diff/40001/Source/web/WebKit.cpp
File Source/web/WebKit.cpp (right):

https://codereview.chromium.org/1183763003/diff/40001/Source/web/WebKit.cpp#n...
Source/web/WebKit.cpp:166: static void arrayBufferAllocationListener(int size)

arrayBufferAllocationListener => adjustAmountOfExternalAllocatedMemory ?

(since this funciton is not specific to array buffers.)

https://codereview.chromium.org/1183763003/diff/40001/Source/wtf/WTF.cpp
File Source/wtf/WTF.cpp (right):

https://codereview.chromium.org/1183763003/diff/40001/Source/wtf/WTF.cpp#newc...
Source/wtf/WTF.cpp:57:
ArrayBufferContents::setAllocationListener(arrayBufferAllocationListenerFunction);

setAdjustAmountOfExternalAllocatedMemory ?

[binji, 2015-06-15 22:53:12]

https://codereview.chromium.org/1183763003/diff/40001/Source/web/WebKit.cpp
File Source/web/WebKit.cpp (right):

https://codereview.chromium.org/1183763003/diff/40001/Source/web/WebKit.cpp#n...
Source/web/WebKit.cpp:166: static void arrayBufferAllocationListener(int size)
On 2015/06/15 at 20:05:25, haraken wrote:
> arrayBufferAllocationListener => adjustAmountOfExternalAllocatedMemory ?
> 
> (since this funciton is not specific to array buffers.)

Done.

https://codereview.chromium.org/1183763003/diff/40001/Source/wtf/WTF.cpp
File Source/wtf/WTF.cpp (right):

https://codereview.chromium.org/1183763003/diff/40001/Source/wtf/WTF.cpp#newc...
Source/wtf/WTF.cpp:57:
ArrayBufferContents::setAllocationListener(arrayBufferAllocationListenerFunction);
On 2015/06/15 at 20:05:25, haraken wrote:
> setAdjustAmountOfExternalAllocatedMemory ?

Done.

[tkent, 2015-06-15 23:21:32]

lgtm

https://codereview.chromium.org/1183763003/diff/60001/Source/wtf/ArrayBufferC...
File Source/wtf/ArrayBufferContents.h (right):

https://codereview.chromium.org/1183763003/diff/60001/Source/wtf/ArrayBufferC...
Source/wtf/ArrayBufferContents.h:76: static
AdjustAmountOfExternalAllocatedMemoryFunction
adjustAmountOfExternalAllocatedMemoryFunction;
adjustAmountOfExternalAllocatedMemoryFunction ->
s_adjustAmountOfExternalAllocatedMemoryFunction

[binji, 2015-06-16 06:20:55]

The CQ bit was checked by binji@chromium.org

[binji, 2015-06-16 06:20:56]

https://codereview.chromium.org/1183763003/diff/60001/Source/wtf/ArrayBufferC...
File Source/wtf/ArrayBufferContents.h (right):

https://codereview.chromium.org/1183763003/diff/60001/Source/wtf/ArrayBufferC...
Source/wtf/ArrayBufferContents.h:76: static
AdjustAmountOfExternalAllocatedMemoryFunction
adjustAmountOfExternalAllocatedMemoryFunction;
On 2015/06/15 at 23:21:32, tkent wrote:
> adjustAmountOfExternalAllocatedMemoryFunction ->
s_adjustAmountOfExternalAllocatedMemoryFunction

Done.

[binji, 2015-06-16 06:20:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/1183763003/#ps80001
(title: "s_ prefix")

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

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

[commit-bot: I haz the power, 2015-06-16 08:13:24]

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