Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2283053002:
Fix BaseAudioContext::hasPendingActivity() to make it GCed correctly (Closed)
Description[This CL is Work-In-Progress.]
Fix BaseAudioContext::hasPendingActivity() to make it GCed correctly
The realtime AudioContext is not GCed properly even after the tear-down.
It is because the context mark itself as 'has pending activity' no
matter what the actual status is.
This CL changes heuristic of hasPendingActivity method to signal V8
that this context is ready for GC; rtoy@ and I discussed that the best
logic is when the destination handler does not have incoming connection
any more.
BUG=503845
TEST=LayoutTests/webaudio/audiocontext-leak.html
Patch Set 1 #
Total comments: 2
Patch Set 2 : [DO NOT SUBMIT] Adding printf for verification #
Total comments: 1
Messages
Total messages: 27 (6 generated)
hongchan@chromium.org changed reviewers: + rtoy@chromium.org
PTAL.
https://codereview.chromium.org/2283053002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.cpp (right): https://codereview.chromium.org/2283053002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.cpp:137: } Since AudioDestinationNode is an AudioNode with a single input, can't this all be done using audioDestinationHandler()->input(0)->isConnected()? (See PannerHandler::process for an example). This gets rid of a lot of redundant stuff, if this actually works.
> This CL changes heuristic of hasPendingActivity method to signal Oilpan that this context is ready for GC Nit: hasPendingActivity is a mechanism to signal V8, not Oilpan. BTW, will this CL fix the web-audio leaks suppressed in LeakExpectations? https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/LeakExpec...
Description was changed from ========== Fix BaseAudioContext::hasPendingActivity() to make it GCed correctly The realtime AudioContext is not GCed properly even after the tear-down. It is because the context mark itself as 'has pending activity' no matter what the actual status is. This CL changes heuristic of hasPendingActivity method to signal Oilpan that this context is ready for GC; rtoy@ and I discussed that the best logic is when the destination handler does not have incoming connection any more. BUG=503845 TEST=LayoutTests/webaudio/audiocontext-leak.html ========== to ========== Fix BaseAudioContext::hasPendingActivity() to make it GCed correctly The realtime AudioContext is not GCed properly even after the tear-down. It is because the context mark itself as 'has pending activity' no matter what the actual status is. This CL changes heuristic of hasPendingActivity method to signal V8 that this context is ready for GC; rtoy@ and I discussed that the best logic is when the destination handler does not have incoming connection any more. BUG=503845 TEST=LayoutTests/webaudio/audiocontext-leak.html ==========
On 2016/08/27 02:10:03, haraken wrote: > > This CL changes heuristic of hasPendingActivity method to signal Oilpan that > this context is ready for GC > > Nit: hasPendingActivity is a mechanism to signal V8, not Oilpan. > > BTW, will this CL fix the web-audio leaks suppressed in LeakExpectations? > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/LeakExpec... Hmm. I am not sure how the WebAudio is related to the issue 451577 or the issue 578297. I think the comment in LeakExpectation needs to be fixed. Note that this only fixes the leak of BaseAudioContext. However, the approach in the PS1 is not quite correct. I am going to move the proxy variable for the destination connection to DeferredTaskHandler so we don't have to deal with the atomic access. Although PS1 passes the attached layout test, but it makes the renderer crash on other layout tests. I am investigating it at the moment.
Description was changed from ========== Fix BaseAudioContext::hasPendingActivity() to make it GCed correctly The realtime AudioContext is not GCed properly even after the tear-down. It is because the context mark itself as 'has pending activity' no matter what the actual status is. This CL changes heuristic of hasPendingActivity method to signal V8 that this context is ready for GC; rtoy@ and I discussed that the best logic is when the destination handler does not have incoming connection any more. BUG=503845 TEST=LayoutTests/webaudio/audiocontext-leak.html ========== to ========== [This CL is Work-In-Progress.] Fix BaseAudioContext::hasPendingActivity() to make it GCed correctly The realtime AudioContext is not GCed properly even after the tear-down. It is because the context mark itself as 'has pending activity' no matter what the actual status is. This CL changes heuristic of hasPendingActivity method to signal V8 that this context is ready for GC; rtoy@ and I discussed that the best logic is when the destination handler does not have incoming connection any more. BUG=503845 TEST=LayoutTests/webaudio/audiocontext-leak.html ==========
hongchan@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2283053002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.cpp (right): https://codereview.chromium.org/2283053002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.cpp:137: } On 2016/08/26 20:12:15, Raymond Toy wrote: > Since AudioDestinationNode is an AudioNode with a single input, can't this all > be done using audioDestinationHandler()->input(0)->isConnected()? (See > PannerHandler::process for an example). > > This gets rid of a lot of redundant stuff, if this actually works. As we discussed offline, this structure is necessary because the active connection of the input summing junction can only be touched by the audio rendering thread. So I am using the atomic access with a proxy variable, but this causes the renderer crash in WebAudio layout tests.
hongchan@chromium.org changed reviewers: + dominicc@chromium.org
+dominicc@ Could you take a look at why stop() is not getting called at all in the test? This is the test log from PS2: --- BaseAudioContext::hasPendingActivity BaseAudioContext::hasPendingActivity BaseAudioContext::hasPendingActivity BaseAudioContext::hasPendingActivity BaseAudioContext::hasPendingActivity BaseAudioContext::hasPendingActivity BaseAudioContext::hasPendingActivity This is a testharness.js-based test. PASS window.internals exist. PASS window.GCController exist. FAIL audioContextObservation.wasCollected assert_true: audioContextObservation.wasCollected was false instead of true. expected true got false Harness: the test ran to completion. --- stop() doesn't get called, thus neither uninitialize() nor clear() is not called. So clearing the context did not happen when the test is checking if the context is GCed. However, the test runner does not report the leaking (--enable-leak-detection), so perhaps the leak is happening between the end of the layout test and the actual test completion by the test runner? I am not completely sure.
On 2016/08/31 at 16:49:33, hongchan wrote:
> +dominicc@
>
> Could you take a look at why stop() is not getting called at all in the test?
>
> This is the test log from PS2:
> ---
> BaseAudioContext::hasPendingActivity
> BaseAudioContext::hasPendingActivity
> BaseAudioContext::hasPendingActivity
> BaseAudioContext::hasPendingActivity
> BaseAudioContext::hasPendingActivity
> BaseAudioContext::hasPendingActivity
> BaseAudioContext::hasPendingActivity
> This is a testharness.js-based test.
> PASS window.internals exist.
> PASS window.GCController exist.
> FAIL audioContextObservation.wasCollected assert_true:
audioContextObservation.wasCollected was false instead of true. expected true
got false
> Harness: the test ran to completion.
> ---
>
> stop() doesn't get called, thus neither uninitialize() nor clear() is not
called. So clearing the context did not happen when the test is checking if the
context is GCed.
>
> However, the test runner does not report the leaking
(--enable-leak-detection), so perhaps the leak is happening between the end of
the layout test and the actual test completion by the test runner? I am not
completely sure.
Yes, good test and analysis. Great work! I'm glad you're looking at this.
In my experience the likely causes are:
1. The object really is kept alive. The test is a true failure.
2. I suspect V8 doesn't track the liveness of temporaries, like the result of
new AudioContext, in a very precise way so things end up living a tiny bit
longer for that reason. Typically the way around this is to call a function or
even spinning the event loop. So something like:
var audioContext;
(() => { audioContextObservation = internals.observeGC(new AudioContext());
})();
GCController.collectAll();
Should('audioContextObservation.wasCollected',
audioContextObservation.wasCollected).beEqualTo(true);
Or an extreme sanity check:
var audioContextObservation; // global now
function step1() {
audioContextObservation = internals.observeGC(new AudioContext());
requestAnimationFrame(step2);
}
function step2() {
GCController.collectAll();
Should('audioContextObservation.wasCollected',
audioContextObservation.wasCollected).beEqualTo(true);
// signal the test to continue
}
step1();
3. GCController.collectAll doesn't really collect all (sadly). If you look at
the comment here, there's just this magic number that 7 garbage collections are
enough:
https://cs.chromium.org/chromium/src/components/test_runner/gc_controller.cc?...
When I started working on WebKit, I think that number was three or five. It
unfortunately only goes up as our memory management gets more complicated. So
you can try bumping up that number (although it slows down a number of tests) or
call collectAll many times.
HTH!
On 2016/09/02 01:22:11, dominicc (traveling) wrote:
> On 2016/08/31 at 16:49:33, hongchan wrote:
> > +dominicc@
> >
> > Could you take a look at why stop() is not getting called at all in the
test?
> >
> > This is the test log from PS2:
> > ---
> > BaseAudioContext::hasPendingActivity
> > BaseAudioContext::hasPendingActivity
> > BaseAudioContext::hasPendingActivity
> > BaseAudioContext::hasPendingActivity
> > BaseAudioContext::hasPendingActivity
> > BaseAudioContext::hasPendingActivity
> > BaseAudioContext::hasPendingActivity
> > This is a testharness.js-based test.
> > PASS window.internals exist.
> > PASS window.GCController exist.
> > FAIL audioContextObservation.wasCollected assert_true:
> audioContextObservation.wasCollected was false instead of true. expected true
> got false
> > Harness: the test ran to completion.
> > ---
> >
> > stop() doesn't get called, thus neither uninitialize() nor clear() is not
> called. So clearing the context did not happen when the test is checking if
the
> context is GCed.
> >
> > However, the test runner does not report the leaking
> (--enable-leak-detection), so perhaps the leak is happening between the end of
> the layout test and the actual test completion by the test runner? I am not
> completely sure.
>
> Yes, good test and analysis. Great work! I'm glad you're looking at this.
>
> In my experience the likely causes are:
>
> 1. The object really is kept alive. The test is a true failure.
>
> 2. I suspect V8 doesn't track the liveness of temporaries, like the result of
> new AudioContext, in a very precise way so things end up living a tiny bit
> longer for that reason. Typically the way around this is to call a function or
> even spinning the event loop. So something like:
>
> var audioContext;
> (() => { audioContextObservation = internals.observeGC(new AudioContext());
> })();
> GCController.collectAll();
> Should('audioContextObservation.wasCollected',
> audioContextObservation.wasCollected).beEqualTo(true);
>
> Or an extreme sanity check:
>
> var audioContextObservation; // global now
> function step1() {
> audioContextObservation = internals.observeGC(new AudioContext());
> requestAnimationFrame(step2);
> }
> function step2() {
> GCController.collectAll();
> Should('audioContextObservation.wasCollected',
> audioContextObservation.wasCollected).beEqualTo(true);
> // signal the test to continue
> }
> step1();
>
> 3. GCController.collectAll doesn't really collect all (sadly). If you look at
> the comment here, there's just this magic number that 7 garbage collections
are
> enough:
>
>
https://cs.chromium.org/chromium/src/components/test_runner/gc_controller.cc?...
>
> When I started working on WebKit, I think that number was three or five. It
> unfortunately only goes up as our memory management gets more complicated. So
> you can try bumping up that number (although it slows down a number of tests)
or
> call collectAll many times.
>
> HTH!
stop() is called only when the associated Document gets detached. In your test,
the Document is not detached and thus stop() is not called.
I'm confused. If you have the following code:
new AudioContext()
gc();
Are you expecting that the GC collects the AudioContext?
With your CL, the GC won't collect the AudioContext because the AudioContext is
not cleared until the associated Document gets detached.
On 2016/09/02 01:22:11, dominicc (traveling) wrote:
> On 2016/08/31 at 16:49:33, hongchan wrote:
> > +dominicc@
> >
> > Could you take a look at why stop() is not getting called at all in the
test?
> >
> > This is the test log from PS2:
> > ---
> > BaseAudioContext::hasPendingActivity
> > BaseAudioContext::hasPendingActivity
> > BaseAudioContext::hasPendingActivity
> > BaseAudioContext::hasPendingActivity
> > BaseAudioContext::hasPendingActivity
> > BaseAudioContext::hasPendingActivity
> > BaseAudioContext::hasPendingActivity
> > This is a testharness.js-based test.
> > PASS window.internals exist.
> > PASS window.GCController exist.
> > FAIL audioContextObservation.wasCollected assert_true:
> audioContextObservation.wasCollected was false instead of true. expected true
> got false
> > Harness: the test ran to completion.
> > ---
> >
> > stop() doesn't get called, thus neither uninitialize() nor clear() is not
> called. So clearing the context did not happen when the test is checking if
the
> context is GCed.
> >
> > However, the test runner does not report the leaking
> (--enable-leak-detection), so perhaps the leak is happening between the end of
> the layout test and the actual test completion by the test runner? I am not
> completely sure.
>
> Yes, good test and analysis. Great work! I'm glad you're looking at this.
>
> In my experience the likely causes are:
>
> 1. The object really is kept alive. The test is a true failure.
>
> 2. I suspect V8 doesn't track the liveness of temporaries, like the result of
> new AudioContext, in a very precise way so things end up living a tiny bit
> longer for that reason. Typically the way around this is to call a function or
> even spinning the event loop. So something like:
>
> var audioContext;
> (() => { audioContextObservation = internals.observeGC(new AudioContext());
> })();
> GCController.collectAll();
> Should('audioContextObservation.wasCollected',
> audioContextObservation.wasCollected).beEqualTo(true);
>
> Or an extreme sanity check:
>
> var audioContextObservation; // global now
> function step1() {
> audioContextObservation = internals.observeGC(new AudioContext());
> requestAnimationFrame(step2);
> }
> function step2() {
> GCController.collectAll();
> Should('audioContextObservation.wasCollected',
> audioContextObservation.wasCollected).beEqualTo(true);
> // signal the test to continue
> }
> step1();
>
> 3. GCController.collectAll doesn't really collect all (sadly). If you look at
> the comment here, there's just this magic number that 7 garbage collections
are
> enough:
>
>
https://cs.chromium.org/chromium/src/components/test_runner/gc_controller.cc?...
>
> When I started working on WebKit, I think that number was three or five. It
> unfortunately only goes up as our memory management gets more complicated. So
> you can try bumping up that number (although it slows down a number of tests)
or
> call collectAll many times.
>
> HTH!
Thanks Dominic - this is super helpful. I'll do the experiment and report back
here.
On 2016/09/02 03:15:47, haraken wrote:
> On 2016/09/02 01:22:11, dominicc (traveling) wrote:
> > On 2016/08/31 at 16:49:33, hongchan wrote:
> > > +dominicc@
> > >
> > > Could you take a look at why stop() is not getting called at all in the
> test?
> > >
> > > This is the test log from PS2:
> > > ---
> > > BaseAudioContext::hasPendingActivity
> > > BaseAudioContext::hasPendingActivity
> > > BaseAudioContext::hasPendingActivity
> > > BaseAudioContext::hasPendingActivity
> > > BaseAudioContext::hasPendingActivity
> > > BaseAudioContext::hasPendingActivity
> > > BaseAudioContext::hasPendingActivity
> > > This is a testharness.js-based test.
> > > PASS window.internals exist.
> > > PASS window.GCController exist.
> > > FAIL audioContextObservation.wasCollected assert_true:
> > audioContextObservation.wasCollected was false instead of true. expected
true
> > got false
> > > Harness: the test ran to completion.
> > > ---
> > >
> > > stop() doesn't get called, thus neither uninitialize() nor clear() is not
> > called. So clearing the context did not happen when the test is checking if
> the
> > context is GCed.
> > >
> > > However, the test runner does not report the leaking
> > (--enable-leak-detection), so perhaps the leak is happening between the end
of
> > the layout test and the actual test completion by the test runner? I am not
> > completely sure.
> >
> > Yes, good test and analysis. Great work! I'm glad you're looking at this.
> >
> > In my experience the likely causes are:
> >
> > 1. The object really is kept alive. The test is a true failure.
> >
> > 2. I suspect V8 doesn't track the liveness of temporaries, like the result
of
> > new AudioContext, in a very precise way so things end up living a tiny bit
> > longer for that reason. Typically the way around this is to call a function
or
> > even spinning the event loop. So something like:
> >
> > var audioContext;
> > (() => { audioContextObservation = internals.observeGC(new AudioContext());
> > })();
> > GCController.collectAll();
> > Should('audioContextObservation.wasCollected',
> > audioContextObservation.wasCollected).beEqualTo(true);
> >
> > Or an extreme sanity check:
> >
> > var audioContextObservation; // global now
> > function step1() {
> > audioContextObservation = internals.observeGC(new AudioContext());
> > requestAnimationFrame(step2);
> > }
> > function step2() {
> > GCController.collectAll();
> > Should('audioContextObservation.wasCollected',
> > audioContextObservation.wasCollected).beEqualTo(true);
> > // signal the test to continue
> > }
> > step1();
> >
> > 3. GCController.collectAll doesn't really collect all (sadly). If you look
at
> > the comment here, there's just this magic number that 7 garbage collections
> are
> > enough:
> >
> >
>
https://cs.chromium.org/chromium/src/components/test_runner/gc_controller.cc?...
> >
> > When I started working on WebKit, I think that number was three or five. It
> > unfortunately only goes up as our memory management gets more complicated.
So
> > you can try bumping up that number (although it slows down a number of
tests)
> or
> > call collectAll many times.
> >
> > HTH!
>
> stop() is called only when the associated Document gets detached. In your
test,
> the Document is not detached and thus stop() is not called.
>
> I'm confused. If you have the following code:
>
> new AudioContext()
> gc();
>
> Are you expecting that the GC collects the AudioContext?
>
> With your CL, the GC won't collect the AudioContext because the AudioContext
is
> not cleared until the associated Document gets detached.
Thanks for the clarification - that explains the observed behavior. I wasn't
sure how stop() and hasPendingActivity() interact when the document shuts down.
PS1 actually changed the heuristic of hasPendingActivity() by returning the
active connection of the destination node. However, stop() is still not being
called because this happens inside of a document. (no document being detached.)
The context is not cleared, yet hasPendingActivity() returns false because it
has zero connection. So the context leaks badly and the tab crashes after few
reloads.
To resolve this, we can merge all tearing-down tasks into uninitialize() and
call it when:
1) stop() is called. (Document is detached.)
2) An AudioContext object has no JS reference and has zero connection to it.
But I am not sure how we can detect when an JS object is derefed.
On 2016/09/02 04:35:41, hoch wrote:
> On 2016/09/02 03:15:47, haraken wrote:
> > On 2016/09/02 01:22:11, dominicc (traveling) wrote:
> > > On 2016/08/31 at 16:49:33, hongchan wrote:
> > > > +dominicc@
> > > >
> > > > Could you take a look at why stop() is not getting called at all in the
> > test?
> > > >
> > > > This is the test log from PS2:
> > > > ---
> > > > BaseAudioContext::hasPendingActivity
> > > > BaseAudioContext::hasPendingActivity
> > > > BaseAudioContext::hasPendingActivity
> > > > BaseAudioContext::hasPendingActivity
> > > > BaseAudioContext::hasPendingActivity
> > > > BaseAudioContext::hasPendingActivity
> > > > BaseAudioContext::hasPendingActivity
> > > > This is a testharness.js-based test.
> > > > PASS window.internals exist.
> > > > PASS window.GCController exist.
> > > > FAIL audioContextObservation.wasCollected assert_true:
> > > audioContextObservation.wasCollected was false instead of true. expected
> true
> > > got false
> > > > Harness: the test ran to completion.
> > > > ---
> > > >
> > > > stop() doesn't get called, thus neither uninitialize() nor clear() is
not
> > > called. So clearing the context did not happen when the test is checking
if
> > the
> > > context is GCed.
> > > >
> > > > However, the test runner does not report the leaking
> > > (--enable-leak-detection), so perhaps the leak is happening between the
end
> of
> > > the layout test and the actual test completion by the test runner? I am
not
> > > completely sure.
> > >
> > > Yes, good test and analysis. Great work! I'm glad you're looking at this.
> > >
> > > In my experience the likely causes are:
> > >
> > > 1. The object really is kept alive. The test is a true failure.
> > >
> > > 2. I suspect V8 doesn't track the liveness of temporaries, like the result
> of
> > > new AudioContext, in a very precise way so things end up living a tiny bit
> > > longer for that reason. Typically the way around this is to call a
function
> or
> > > even spinning the event loop. So something like:
> > >
> > > var audioContext;
> > > (() => { audioContextObservation = internals.observeGC(new
AudioContext());
> > > })();
> > > GCController.collectAll();
> > > Should('audioContextObservation.wasCollected',
> > > audioContextObservation.wasCollected).beEqualTo(true);
> > >
> > > Or an extreme sanity check:
> > >
> > > var audioContextObservation; // global now
> > > function step1() {
> > > audioContextObservation = internals.observeGC(new AudioContext());
> > > requestAnimationFrame(step2);
> > > }
> > > function step2() {
> > > GCController.collectAll();
> > > Should('audioContextObservation.wasCollected',
> > > audioContextObservation.wasCollected).beEqualTo(true);
> > > // signal the test to continue
> > > }
> > > step1();
> > >
> > > 3. GCController.collectAll doesn't really collect all (sadly). If you look
> at
> > > the comment here, there's just this magic number that 7 garbage
collections
> > are
> > > enough:
> > >
> > >
> >
>
https://cs.chromium.org/chromium/src/components/test_runner/gc_controller.cc?...
> > >
> > > When I started working on WebKit, I think that number was three or five.
It
> > > unfortunately only goes up as our memory management gets more complicated.
> So
> > > you can try bumping up that number (although it slows down a number of
> tests)
> > or
> > > call collectAll many times.
> > >
> > > HTH!
> >
> > stop() is called only when the associated Document gets detached. In your
> test,
> > the Document is not detached and thus stop() is not called.
> >
> > I'm confused. If you have the following code:
> >
> > new AudioContext()
> > gc();
> >
> > Are you expecting that the GC collects the AudioContext?
> >
> > With your CL, the GC won't collect the AudioContext because the AudioContext
> is
> > not cleared until the associated Document gets detached.
>
> Thanks for the clarification - that explains the observed behavior. I wasn't
> sure how stop() and hasPendingActivity() interact when the document shuts
down.
>
> PS1 actually changed the heuristic of hasPendingActivity() by returning the
> active connection of the destination node. However, stop() is still not being
> called because this happens inside of a document. (no document being
detached.)
> The context is not cleared, yet hasPendingActivity() returns false because it
> has zero connection. So the context leaks badly and the tab crashes after few
> reloads.
>
> To resolve this, we can merge all tearing-down tasks into uninitialize() and
> call it when:
>
> 1) stop() is called. (Document is detached.)
> 2) An AudioContext object has no JS reference and has zero connection to it.
>
> But I am not sure how we can detect when an JS object is derefed.
There is no way to detect when a JS object is derefed (by design). However, that
shouldn't be a problem because V8 calls hasPendingActivity() for all existing
wrappers before running a GC. If you think that the DOM object is not needed
from the DOM-side perspective, you can just return false from
hasPendingActivity(). Then the V8 GC will collect the DOM object.
i.e., I think you just need to make hasPendingActivity return false when it has
zero connection or the document is detached.
> There is no way to detect when a JS object is derefed (by design). However, that > shouldn't be a problem because V8 calls hasPendingActivity() for all existing > wrappers before running a GC. If you think that the DOM object is not needed > from the DOM-side perspective, you can just return false from > hasPendingActivity(). Then the V8 GC will collect the DOM object. Does hasPendingActivity() being called means that the object is derefed? We don't need to know the exact timing of dereference. If we can interpret this call as a sign of 'this object is targeted for GC', we can start the uninitialization when the conditions are met. (i.e. no active connection to the destination & the context is cleared)
On 2016/09/02 17:04:25, hoch wrote: > > There is no way to detect when a JS object is derefed (by design). However, > that > > shouldn't be a problem because V8 calls hasPendingActivity() for all existing > > wrappers before running a GC. If you think that the DOM object is not needed > > from the DOM-side perspective, you can just return false from > > hasPendingActivity(). Then the V8 GC will collect the DOM object. > > Does hasPendingActivity() being called means that the object is derefed? No. hasPendingActivity() is called for all wrappers. If you return true, the wrapper will be kept alive. If you return false and the wrapper is unreachable on the V8 side, the wrapper will be collected. > We don't need to know the exact timing of dereference. If we can interpret this > call as a sign of 'this object is targeted for GC', we can start the > uninitialization when the conditions are met. (i.e. no active connection to the > destination & the context is cleared) Again, there's no way to know the timing when a wrapper is GCed. Why doesn't the approach I mentioned in #16 work?
https://codereview.chromium.org/2283053002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right): https://codereview.chromium.org/2283053002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:221: return !m_isCleared; This is different from PS1, where you were returning whether the destination had connections or not. Could this explain the results of this test?
> No. hasPendingActivity() is called for all wrappers. If you return true, the > wrapper will be kept alive. If you return false and the wrapper is unreachable > on the V8 side, the wrapper will be collected. Ack. > Again, there's no way to know the timing when a wrapper is GCed. > Why doesn't the approach I mentioned in #16 work? And the suggestion from #16: "make hasPendingActivity return false it has zero connection or the document is detached." First, we don't care about the case of document being detached, because it already works correctly. stop() is explicitly called then we can do tearing-down. So the concern here is the first condition - "zero connect". Let me simplify the question: a) var ac = new AudioContext(); You don't do anything after that, the destination has zero connection. However, the object should not be GCed because the reference is still active. b) var ac = new AudioContext(); ac = null; In this case, the object must be GCed as soon as possible releasing all the resources allocated. However, the problem here is that we cannot differentiate these two cases from the AudioContext's perspective. So we basically leak the AudioContext in b) - it does not have a JS reference, but we don't release the resources. If any object can signal the AudioContext that 'hey you need to go away', this problem can be fixed easily.
On 2016/09/02 18:19:21, Raymond Toy wrote: > https://codereview.chromium.org/2283053002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right): > > https://codereview.chromium.org/2283053002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:221: return > !m_isCleared; > This is different from PS1, where you were returning whether the destination had > connections or not. Could this explain the results of this test? PS2 is up there simply to show the behavior of hasPendingActivity() and stop() not getting called. I will remove PS2 since now we got the answers for that.
haraken@chromium.org changed reviewers: + tkent@chromium.org
On 2016/09/02 18:40:19, hoch wrote: > > No. hasPendingActivity() is called for all wrappers. If you return true, the > > wrapper will be kept alive. If you return false and the wrapper is unreachable > > on the V8 side, the wrapper will be collected. > > Ack. > > > > Again, there's no way to know the timing when a wrapper is GCed. > > Why doesn't the approach I mentioned in #16 work? > > And the suggestion from #16: "make hasPendingActivity return false it has zero > connection or the document is detached." > > First, we don't care about the case of document being detached, because it > already works correctly. stop() is explicitly called then we can do > tearing-down. > So the concern here is the first condition - "zero connect". Let me simplify the > question: > > a) var ac = new AudioContext(); > > You don't do anything after that, the destination has zero connection. However, > the object should not be GCed because the reference is still active. > > b) var ac = new AudioContext(); ac = null; > > In this case, the object must be GCed as soon as possible releasing all the > resources allocated. > > However, the problem here is that we cannot differentiate these two cases from > the AudioContext's perspective. So we basically leak the AudioContext in b) - it > does not have a JS reference, but we don't release the resources. If any object > can signal the AudioContext that 'hey you need to go away', this problem can be > fixed easily. tkent@ was previously hitting the same problem for AudioNodes and wrote a design doc here: https://docs.google.com/document/d/1MrHQ5RNMTEqlWMR-_yuqmxIG1lxvhA3xF32r2SJ73... Is it helpful?
On 2016/09/02 at 19:11:46, haraken wrote: > On 2016/09/02 18:40:19, hoch wrote: > > > No. hasPendingActivity() is called for all wrappers. If you return true, the > > > wrapper will be kept alive. If you return false and the wrapper is unreachable > > > on the V8 side, the wrapper will be collected. > > > > Ack. > > > > > > > Again, there's no way to know the timing when a wrapper is GCed. > > > Why doesn't the approach I mentioned in #16 work? > > > > And the suggestion from #16: "make hasPendingActivity return false it has zero > > connection or the document is detached." > > > > First, we don't care about the case of document being detached, because it > > already works correctly. stop() is explicitly called then we can do > > tearing-down. > > So the concern here is the first condition - "zero connect". Let me simplify the > > question: > > > > a) var ac = new AudioContext(); > > > > You don't do anything after that, the destination has zero connection. However, > > the object should not be GCed because the reference is still active. > > > > b) var ac = new AudioContext(); ac = null; > > > > In this case, the object must be GCed as soon as possible releasing all the > > resources allocated. > > > > However, the problem here is that we cannot differentiate these two cases from > > the AudioContext's perspective. So we basically leak the AudioContext in b) - it > > does not have a JS reference, but we don't release the resources. If any object > > can signal the AudioContext that 'hey you need to go away', this problem can be > > fixed easily. > > tkent@ was previously hitting the same problem for AudioNodes and wrote a design doc here: https://docs.google.com/document/d/1MrHQ5RNMTEqlWMR-_yuqmxIG1lxvhA3xF32r2SJ73... > > Is it helpful? hongchan@ and I were discussing a little earlier whether we could make the contexts look more like the AudioNode/AudioHandler classes. Then JS could collect the context, while the context handler could stay alive for a bit longer, if needed. On the other hand, I'm fine if we kill the handler right away if the context were collected. There's no expectation that audio should continue to play if you drop all references to the context.
On 2016/09/02 at 20:37:58, rtoy wrote: > On 2016/09/02 at 19:11:46, haraken wrote: > > On 2016/09/02 18:40:19, hoch wrote: > > > > No. hasPendingActivity() is called for all wrappers. If you return true, the > > > > wrapper will be kept alive. If you return false and the wrapper is unreachable > > > > on the V8 side, the wrapper will be collected. > > > > > > Ack. > > > > > > > > > > Again, there's no way to know the timing when a wrapper is GCed. > > > > Why doesn't the approach I mentioned in #16 work? > > > > > > And the suggestion from #16: "make hasPendingActivity return false it has zero > > > connection or the document is detached." > > > > > > First, we don't care about the case of document being detached, because it > > > already works correctly. stop() is explicitly called then we can do > > > tearing-down. > > > So the concern here is the first condition - "zero connect". Let me simplify the > > > question: > > > > > > a) var ac = new AudioContext(); > > > > > > You don't do anything after that, the destination has zero connection. However, > > > the object should not be GCed because the reference is still active. > > > > > > b) var ac = new AudioContext(); ac = null; > > > > > > In this case, the object must be GCed as soon as possible releasing all the > > > resources allocated. > > > > > > However, the problem here is that we cannot differentiate these two cases from > > > the AudioContext's perspective. So we basically leak the AudioContext in b) - it > > > does not have a JS reference, but we don't release the resources. If any object > > > can signal the AudioContext that 'hey you need to go away', this problem can be > > > fixed easily. > > > > tkent@ was previously hitting the same problem for AudioNodes and wrote a design doc here: https://docs.google.com/document/d/1MrHQ5RNMTEqlWMR-_yuqmxIG1lxvhA3xF32r2SJ73... > > > > Is it helpful? > > hongchan@ and I were discussing a little earlier whether we could make the contexts > look more like the AudioNode/AudioHandler classes. Then JS could collect the context, > while the context handler could stay alive for a bit longer, if needed. On the other > hand, I'm fine if we kill the handler right away if the context were collected. There's > no expectation that audio should continue to play if you drop all references to the > context. What does Chris Wilson or Eiji Kitamura think about the idea of stopping the audio if you drop a reference to the context? There was an old crbug which complained that the audio stopped when something got garbage collected too soon; I think some users may expect the audio to continue to play? I think there's a danger if you stop the audio when the context is dropped that authors will cargo-cult leaking the context by stuffing it on window as soon as they create one. I think we should collect the context as soon as practical after it's not observable whether it stays alive. That is, when you don't hear any difference and there's no reference to it which would let you restart it. Everything else that is garbage collected works this way (or tries to.)
On 2016/09/06 at 17:11:24, dominicc wrote: > On 2016/09/02 at 20:37:58, rtoy wrote: > > On 2016/09/02 at 19:11:46, haraken wrote: > > > On 2016/09/02 18:40:19, hoch wrote: > > > > > No. hasPendingActivity() is called for all wrappers. If you return true, the > > > > > wrapper will be kept alive. If you return false and the wrapper is unreachable > > > > > on the V8 side, the wrapper will be collected. > > > > > > > > Ack. > > > > > > > > > > > > > Again, there's no way to know the timing when a wrapper is GCed. > > > > > Why doesn't the approach I mentioned in #16 work? > > > > > > > > And the suggestion from #16: "make hasPendingActivity return false it has zero > > > > connection or the document is detached." > > > > > > > > First, we don't care about the case of document being detached, because it > > > > already works correctly. stop() is explicitly called then we can do > > > > tearing-down. > > > > So the concern here is the first condition - "zero connect". Let me simplify the > > > > question: > > > > > > > > a) var ac = new AudioContext(); > > > > > > > > You don't do anything after that, the destination has zero connection. However, > > > > the object should not be GCed because the reference is still active. > > > > > > > > b) var ac = new AudioContext(); ac = null; > > > > > > > > In this case, the object must be GCed as soon as possible releasing all the > > > > resources allocated. > > > > > > > > However, the problem here is that we cannot differentiate these two cases from > > > > the AudioContext's perspective. So we basically leak the AudioContext in b) - it > > > > does not have a JS reference, but we don't release the resources. If any object > > > > can signal the AudioContext that 'hey you need to go away', this problem can be > > > > fixed easily. > > > > > > tkent@ was previously hitting the same problem for AudioNodes and wrote a design doc here: https://docs.google.com/document/d/1MrHQ5RNMTEqlWMR-_yuqmxIG1lxvhA3xF32r2SJ73... > > > > > > Is it helpful? > > > > hongchan@ and I were discussing a little earlier whether we could make the contexts > > look more like the AudioNode/AudioHandler classes. Then JS could collect the context, > > while the context handler could stay alive for a bit longer, if needed. On the other > > hand, I'm fine if we kill the handler right away if the context were collected. There's > > no expectation that audio should continue to play if you drop all references to the > > context. > > What does Chris Wilson or Eiji Kitamura think about the idea of stopping the audio if you drop a reference to the context? There was an old crbug which complained that the audio stopped when something got garbage collected too soon; I think some users may expect the audio to continue to play? > > I think there's a danger if you stop the audio when the context is dropped that authors will cargo-cult leaking the context by stuffing it on window as soon as they create one. > > I think we should collect the context as soon as practical after it's not observable whether it stays alive. That is, when you don't hear any difference and there's no reference to it which would let you restart it. Everything else that is garbage collected works this way (or tries to.) Should we close this issue for now and reopen it later? (Was there any resolution about the semantics?)
On 2016/10/14 08:23:46, dominicc wrote: > On 2016/09/06 at 17:11:24, dominicc wrote: > > On 2016/09/02 at 20:37:58, rtoy wrote: > > > On 2016/09/02 at 19:11:46, haraken wrote: > > > > On 2016/09/02 18:40:19, hoch wrote: > > > > > > No. hasPendingActivity() is called for all wrappers. If you return > true, the > > > > > > wrapper will be kept alive. If you return false and the wrapper is > unreachable > > > > > > on the V8 side, the wrapper will be collected. > > > > > > > > > > Ack. > > > > > > > > > > > > > > > > Again, there's no way to know the timing when a wrapper is GCed. > > > > > > Why doesn't the approach I mentioned in #16 work? > > > > > > > > > > And the suggestion from #16: "make hasPendingActivity return false it > has zero > > > > > connection or the document is detached." > > > > > > > > > > First, we don't care about the case of document being detached, because > it > > > > > already works correctly. stop() is explicitly called then we can do > > > > > tearing-down. > > > > > So the concern here is the first condition - "zero connect". Let me > simplify the > > > > > question: > > > > > > > > > > a) var ac = new AudioContext(); > > > > > > > > > > You don't do anything after that, the destination has zero connection. > However, > > > > > the object should not be GCed because the reference is still active. > > > > > > > > > > b) var ac = new AudioContext(); ac = null; > > > > > > > > > > In this case, the object must be GCed as soon as possible releasing all > the > > > > > resources allocated. > > > > > > > > > > However, the problem here is that we cannot differentiate these two > cases from > > > > > the AudioContext's perspective. So we basically leak the AudioContext in > b) - it > > > > > does not have a JS reference, but we don't release the resources. If any > object > > > > > can signal the AudioContext that 'hey you need to go away', this problem > can be > > > > > fixed easily. > > > > > > > > tkent@ was previously hitting the same problem for AudioNodes and wrote a > design doc here: > https://docs.google.com/document/d/1MrHQ5RNMTEqlWMR-_yuqmxIG1lxvhA3xF32r2SJ73... > > > > > > > > Is it helpful? > > > > > > hongchan@ and I were discussing a little earlier whether we could make the > contexts > > > look more like the AudioNode/AudioHandler classes. Then JS could collect > the context, > > > while the context handler could stay alive for a bit longer, if needed. On > the other > > > hand, I'm fine if we kill the handler right away if the context were > collected. There's > > > no expectation that audio should continue to play if you drop all references > to the > > > context. > > > > What does Chris Wilson or Eiji Kitamura think about the idea of stopping the > audio if you drop a reference to the context? There was an old crbug which > complained that the audio stopped when something got garbage collected too soon; > I think some users may expect the audio to continue to play? > > > > I think there's a danger if you stop the audio when the context is dropped > that authors will cargo-cult leaking the context by stuffing it on window as > soon as they create one. > > > > I think we should collect the context as soon as practical after it's not > observable whether it stays alive. That is, when you don't hear any difference > and there's no reference to it which would let you restart it. Everything else > that is garbage collected works this way (or tries to.) > > Should we close this issue for now and reopen it later? (Was there any > resolution about the semantics?) I should get back to this later, but currently held up by AudioWorklet spec work and implementation. There seem to be an implication of user-visible change when dropping the context, so I think this needs a wider discussion. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
