Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
131183002:
IDL compiler: AudioBuffer interfaces (Closed)
|
Created:
6 years, 11 months ago by Nils Barth (inactive) Modified:
6 years, 11 months ago CC:
blink-reviews, kojih, arv+blink, jsbell+bindings_chromium.org, sof, abarth-chromium, marja+watch_chromium.org, adamk+blink_chromium.org, Nate Chapin, Inactive, kouhei (in TOK), Ken Russell (switch to Gerrit), Raymond Toy Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionIDL compiler: AudioBuffer interfaces
BUG=239771
R=haraken
NOTRY=true
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165107
Patch Set 1 #
Total comments: 9
Patch Set 2 : Revised #
Messages
Total messages: 18 (0 generated)
https://codereview.chromium.org/131183002/diff/1/Source/bindings/templates/in... File Source/bindings/templates/interface.cpp (right): https://codereview.chromium.org/131183002/diff/1/Source/bindings/templates/in... Source/bindings/templates/interface.cpp:716: channelData->buffer()->setDeallocationObserver(V8ArrayBufferDeallocationObserver::instanceTemplate()); I don't fully understand why this code is written here. It seems to me that deallocationObserver should be set when we allocate a C++ AudioBuffer object not when we allocate a wrapper of the C++ object. Would you cc AudioBuffer people? https://codereview.chromium.org/131183002/diff/1/Source/bindings/tests/idls/T... File Source/bindings/tests/idls/TestInterfaceAudioBuffer.idl (right): https://codereview.chromium.org/131183002/diff/1/Source/bindings/tests/idls/T... Source/bindings/tests/idls/TestInterfaceAudioBuffer.idl:31: interface TestInterfaceAudioBuffer : AudioBuffer { IMHO, we don't need to add a new testing interface for this kind of small change. We don't want to increase # of testing IDL files.
https://codereview.chromium.org/131183002/diff/1/Source/bindings/templates/in... File Source/bindings/templates/interface.cpp (right): https://codereview.chromium.org/131183002/diff/1/Source/bindings/templates/in... Source/bindings/templates/interface.cpp:716: channelData->buffer()->setDeallocationObserver(V8ArrayBufferDeallocationObserver::instanceTemplate()); On 2014/01/09 08:52:58, haraken wrote: > > I don't fully understand why this code is written here. > Would you cc AudioBuffer people? Will do! https://codereview.chromium.org/131183002/diff/1/Source/bindings/tests/idls/T... File Source/bindings/tests/idls/TestInterfaceAudioBuffer.idl (right): https://codereview.chromium.org/131183002/diff/1/Source/bindings/tests/idls/T... Source/bindings/tests/idls/TestInterfaceAudioBuffer.idl:31: interface TestInterfaceAudioBuffer : AudioBuffer { On 2014/01/09 08:52:58, haraken wrote: > IMHO, we don't need to add a new testing interface for this kind of small > change. We don't want to increase # of testing IDL files. I'll remove this from the final CL. (I need this for testing though.)
https://codereview.chromium.org/131183002/diff/1/Source/bindings/templates/in... File Source/bindings/templates/interface.cpp (right): https://codereview.chromium.org/131183002/diff/1/Source/bindings/templates/in... Source/bindings/templates/interface.cpp:716: channelData->buffer()->setDeallocationObserver(V8ArrayBufferDeallocationObserver::instanceTemplate()); Hi Raymond, We're currently rewriting the IDL compiler (bindings generator) in Python, and we're checking anything puzzling as we go. haraken had a question about why there is special-case code for AudioBuffer here, namely why we're setting the deallocation observer in the bindings (when we create a wrapper), rather than when we constructor the underlying C++ object in Blink. Any insights? Thanks! On 2014/01/09 08:52:58, haraken wrote: > > I don't fully understand why this code is written here. It seems to me that > deallocationObserver should be set when we allocate a C++ AudioBuffer object not > when we allocate a wrapper of the C++ object. > > Would you cc AudioBuffer people?
+kbr
https://codereview.chromium.org/131183002/diff/1/Source/bindings/templates/in... File Source/bindings/templates/interface.cpp (right): https://codereview.chromium.org/131183002/diff/1/Source/bindings/templates/in... Source/bindings/templates/interface.cpp:716: channelData->buffer()->setDeallocationObserver(V8ArrayBufferDeallocationObserver::instanceTemplate()); +dslomov On 2014/01/10 01:37:10, Nils Barth wrote: > Hi Raymond, > We're currently rewriting the IDL compiler (bindings generator) in Python, and > we're checking anything puzzling as we go. > > haraken had a question about why there is special-case code for AudioBuffer > here, namely why we're setting the deallocation observer in the bindings (when > we create a wrapper), rather than when we constructor the underlying C++ object > in Blink. > > Any insights? I'm not sure -- dslomov@ probably knows better -- but I suspect the reason is that we don't want to set deallocation observers for all ArrayBuffer instances, because doing so will hugely slow down the new V8 implementation of typed arrays. > Thanks! > > On 2014/01/09 08:52:58, haraken wrote: > > > > I don't fully understand why this code is written here. It seems to me that > > deallocationObserver should be set when we allocate a C++ AudioBuffer object > not > > when we allocate a wrapper of the C++ object. > > > > Would you cc AudioBuffer people? >
https://codereview.chromium.org/131183002/diff/1/Source/bindings/templates/in... File Source/bindings/templates/interface.cpp (right): https://codereview.chromium.org/131183002/diff/1/Source/bindings/templates/in... Source/bindings/templates/interface.cpp:716: channelData->buffer()->setDeallocationObserver(V8ArrayBufferDeallocationObserver::instanceTemplate()); *ping* Dmitry: could you PTAL? Thanks! > On 2014/01/10 01:37:10, Nils Barth wrote: > > Hi Raymond, > > We're currently rewriting the IDL compiler (bindings generator) in Python, and > > we're checking anything puzzling as we go. > > > > haraken had a question about why there is special-case code for AudioBuffer > > here, namely why we're setting the deallocation observer in the bindings (when > > we create a wrapper), rather than when we constructor the underlying C++ > object > > in Blink. > > > > Any insights? > On 2014/01/10 02:31:31, Ken Russell wrote: > +dslomov > > I'm not sure -- dslomov@ probably knows better -- but I suspect the reason is > that we don't want to set deallocation observers for all ArrayBuffer instances, > because doing so will hugely slow down the new V8 implementation of typed > arrays.
https://codereview.chromium.org/131183002/diff/1/Source/bindings/templates/in... File Source/bindings/templates/interface.cpp (right): https://codereview.chromium.org/131183002/diff/1/Source/bindings/templates/in... Source/bindings/templates/interface.cpp:716: channelData->buffer()->setDeallocationObserver(V8ArrayBufferDeallocationObserver::instanceTemplate()); On 2014/01/14 07:39:45, Nils Barth wrote: > *ping* Dmitry: could you PTAL? Thanks! Yes, sorry, slipped through the cracks. We only setDeallocationObservers on array buffers that are held by some object in JavaScript heap. The contract is, roughly, "if ArrayBuffer has a deallocation observer then there is an object in V8 heap that holds it (maybe indirectly)". The reason why it is important to only setDeallocationObservers only when array buffer becomes exposed to JS is that on setting the observer V8 gets notified that its heap now references the certain amount of external memory. If you set the observer in the constructor, V8 will get notified when the AudioBuffer is created, not when it is exposed to JS. Does this make sense? > > > On 2014/01/10 01:37:10, Nils Barth wrote: > > > Hi Raymond, > > > We're currently rewriting the IDL compiler (bindings generator) in Python, > and > > > we're checking anything puzzling as we go. > > > > > > haraken had a question about why there is special-case code for AudioBuffer > > > here, namely why we're setting the deallocation observer in the bindings > (when > > > we create a wrapper), rather than when we constructor the underlying C++ > > object > > > in Blink. > > > > > > Any insights? > > > > On 2014/01/10 02:31:31, Ken Russell wrote: > > +dslomov > > > > I'm not sure -- dslomov@ probably knows better -- but I suspect the reason is > > that we don't want to set deallocation observers for all ArrayBuffer > instances, > > because doing so will hugely slow down the new V8 implementation of typed > > arrays.
Change LGTM btw (modulo haraken@'s comment re: test interfaces etc.)
https://codereview.chromium.org/131183002/diff/1/Source/bindings/templates/in... File Source/bindings/templates/interface.cpp (right): https://codereview.chromium.org/131183002/diff/1/Source/bindings/templates/in... Source/bindings/templates/interface.cpp:716: channelData->buffer()->setDeallocationObserver(V8ArrayBufferDeallocationObserver::instanceTemplate()); On 2014/01/14 08:37:58, Dmitry Lomov (chromium) wrote: > On 2014/01/14 07:39:45, Nils Barth wrote: > > *ping* Dmitry: could you PTAL? Thanks! > > Yes, sorry, slipped through the cracks. > We only setDeallocationObservers on array buffers that are held by some object > in JavaScript heap. The contract is, roughly, "if ArrayBuffer has a deallocation > observer then there is an object in V8 heap that holds it (maybe indirectly)". > The reason why it is important to only setDeallocationObservers only when array > buffer becomes exposed to JS is that on setting the observer V8 gets notified > that its heap now references the certain amount of external memory. If you set > the observer in the constructor, V8 will get notified when the AudioBuffer is > created, not when it is exposed to JS. > > Does this make sense? What we want is to notify V8 the fact that Blink allocated a large amount of memory, not the fact that something is exposed to JS, isn't it? In that sense, I thought that we should notify the fact to V8 when we allocate the ArrayBuffer, not when we expose the ArrayBuffer to JS.
On 2014/01/14 09:27:05, haraken wrote: > https://codereview.chromium.org/131183002/diff/1/Source/bindings/templates/in... > File Source/bindings/templates/interface.cpp (right): > > https://codereview.chromium.org/131183002/diff/1/Source/bindings/templates/in... > Source/bindings/templates/interface.cpp:716: > channelData->buffer()->setDeallocationObserver(V8ArrayBufferDeallocationObserver::instanceTemplate()); > On 2014/01/14 08:37:58, Dmitry Lomov (chromium) wrote: > > On 2014/01/14 07:39:45, Nils Barth wrote: > > > *ping* Dmitry: could you PTAL? Thanks! > > > > Yes, sorry, slipped through the cracks. > > We only setDeallocationObservers on array buffers that are held by some object > > in JavaScript heap. The contract is, roughly, "if ArrayBuffer has a > deallocation > > observer then there is an object in V8 heap that holds it (maybe indirectly)". > > The reason why it is important to only setDeallocationObservers only when > array > > buffer becomes exposed to JS is that on setting the observer V8 gets notified > > that its heap now references the certain amount of external memory. If you set > > the observer in the constructor, V8 will get notified when the AudioBuffer is > > created, not when it is exposed to JS. > > > > Does this make sense? > > What we want is to notify V8 the fact that Blink allocated a large amount of > memory, not the fact that something is exposed to JS, isn't it? In that sense, I > thought that we should notify the fact to V8 when we allocate the ArrayBuffer, > not when we expose the ArrayBuffer to JS. No. V8 does not care how much memory Blink in general allocates. V8 GC only cares about the memory that it can free on GC. If Blink allocated a large memory blob but collecting JS garbage will not make that blob go away there is no point in collecting garbage.
On 2014/01/14 09:30:48, Dmitry Lomov (chromium) wrote: > On 2014/01/14 09:27:05, haraken wrote: > > > https://codereview.chromium.org/131183002/diff/1/Source/bindings/templates/in... > > File Source/bindings/templates/interface.cpp (right): > > > > > https://codereview.chromium.org/131183002/diff/1/Source/bindings/templates/in... > > Source/bindings/templates/interface.cpp:716: > > > channelData->buffer()->setDeallocationObserver(V8ArrayBufferDeallocationObserver::instanceTemplate()); > > On 2014/01/14 08:37:58, Dmitry Lomov (chromium) wrote: > > > On 2014/01/14 07:39:45, Nils Barth wrote: > > > > *ping* Dmitry: could you PTAL? Thanks! > > > > > > Yes, sorry, slipped through the cracks. > > > We only setDeallocationObservers on array buffers that are held by some > object > > > in JavaScript heap. The contract is, roughly, "if ArrayBuffer has a > > deallocation > > > observer then there is an object in V8 heap that holds it (maybe > indirectly)". > > > The reason why it is important to only setDeallocationObservers only when > > array > > > buffer becomes exposed to JS is that on setting the observer V8 gets > notified > > > that its heap now references the certain amount of external memory. If you > set > > > the observer in the constructor, V8 will get notified when the AudioBuffer > is > > > created, not when it is exposed to JS. > > > > > > Does this make sense? > > > > What we want is to notify V8 the fact that Blink allocated a large amount of > > memory, not the fact that something is exposed to JS, isn't it? In that sense, > I > > thought that we should notify the fact to V8 when we allocate the ArrayBuffer, > > not when we expose the ArrayBuffer to JS. > > No. V8 does not care how much memory Blink in general allocates. V8 GC only > cares about the memory that it can free on GC. If Blink allocated a large memory > blob but collecting JS garbage will not make that blob go away there is no point > in collecting garbage. Makes sense. Thanks for the clarification! LGTM.
Revised: * Rebased * Removed test file * Added comment to explain why we're generating code here haraken, does the comment LGTY?
BTW Dmitry, thank you for the detailed explanation!
LGTM Actually I'm not really happy to have these special-casings, but it seems this is the most straightforward way to realize what we want.
On 2014/01/15 04:02:10, haraken wrote: > Actually I'm not really happy to have these special-casings, but it seems this > is the most straightforward way to realize what we want. Agreed -- we want to avoid special-casing as much as possible, but sometimes it's the best solution. (Sure beats custom bindings!)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nbarth@chromium.org/131183002/190001
Message was sent while issue was closed.
Change committed as 165107 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
