Bug 46834 - Limit number of AudioNode deletions per render quantum in AudioContext
Summary: Limit number of AudioNode deletions per render quantum in AudioContext
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-29 13:01 PDT by Chris Rogers
Modified: 2010-09-29 14:57 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.50 KB, patch)
2010-09-29 13:03 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (2.26 KB, patch)
2010-09-29 13:31 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (2.26 KB, patch)
2010-09-29 14:48 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Rogers 2010-09-29 13:01:04 PDT
Limit number of AudioNode deletions per render quantum in AudioContext
Comment 1 Chris Rogers 2010-09-29 13:03:49 PDT
Created attachment 69242 [details]
Patch
Comment 2 Chris Rogers 2010-09-29 13:06:33 PDT
This fixes a problem where garbage collection suddenly releases tons of AudioNodes at the same time and many are deleted in the real-time audio thread.  This patch amortizes the deletions over several render quantums by deleting only a limited amount each time.

NOTE: this patch also contains an incidental minor change to the HRTFDatabaseLoader API.
Comment 3 Kenneth Russell 2010-09-29 13:07:06 PDT
Comment on attachment 69242 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69242&action=review

There are several changes in this patch unrelated to this bug that need to be removed.

> WebCore/webaudio/AudioContext.cpp:110
> +    m_hrtfDatabaseLoader = HRTFDatabaseLoader::createAndLoadAsynchronouslyIfNecessary(sampleRate());

This is an unrelated change.

> WebCore/webaudio/AudioContext.cpp:351
> +        m_graphOwnerThread = thisThread;

This also looks like an unrelated change.

> WebCore/webaudio/AudioContext.cpp:438
> +    ASSERT(isAudioThread() && isGraphOwner());

Is this new assertion related to this change?

> WebCore/webaudio/AudioContext.h:248
> +    RefPtr<HRTFDatabaseLoader> m_hrtfDatabaseLoader;

Unrelated changes shouldn't be in this patch.
Comment 4 Simon Fraser (smfr) 2010-09-29 13:12:32 PDT
>  many are deleted in the real-time audio thread

Isn't the right solution to ensure that these are not deleted on that thread, rather than implementing throttling?
Comment 5 Kenneth Russell 2010-09-29 13:27:47 PDT
(In reply to comment #4)
> >  many are deleted in the real-time audio thread
> 
> Isn't the right solution to ensure that these are not deleted on that thread, rather than implementing throttling?

Moving the deletion of these objects to the main thread requires rethinking some of the synchronization and queueing strategy. This is an incremental fix improving the behavior without completely rewriting things.
Comment 6 Chris Rogers 2010-09-29 13:29:23 PDT
(In reply to comment #4)
> >  many are deleted in the real-time audio thread
> 
> Isn't the right solution to ensure that these are not deleted on that thread, rather than implementing throttling?

Sometimes, they have to be deleted in the real-time audio thread because the garbage collection happens before the "note" has finished playing.  Later on, in the real-time thread the note finishes and there's no more chance to delete it in the main thread.  We could implement a callOnMainThread() type of thing, but I think this would introduce performance problems into the main thread.  I think we need the throttling (for the audio thread) but also try to delete the nodes at every opportunity we have in the main thread (such as in the connect(), disconnect(), and deref() methods.
Comment 7 Chris Rogers 2010-09-29 13:31:26 PDT
Created attachment 69244 [details]
Patch
Comment 8 Kenneth Russell 2010-09-29 13:36:49 PDT
Comment on attachment 69244 [details]
Patch

Looks fine to me.
Comment 9 Chris Rogers 2010-09-29 14:48:09 PDT
Created attachment 69252 [details]
Patch
Comment 10 Chris Rogers 2010-09-29 14:48:39 PDT
last minute tweak: changed int to unsigned.
Comment 11 Kenneth Russell 2010-09-29 14:49:21 PDT
Comment on attachment 69252 [details]
Patch

OK.
Comment 12 Chris Rogers 2010-09-29 14:57:03 PDT
Comment on attachment 69252 [details]
Patch

Clearing flags on attachment: 69252

Committed r68689: <http://trac.webkit.org/changeset/68689>
Comment 13 Chris Rogers 2010-09-29 14:57:08 PDT
All reviewed patches have been landed.  Closing bug.