Bug 223226 – Add assertions to guard against heap allocations on the audio thread

• Component: Web Audio
• Status: RESOLVED FIXED

[Chris Dumez]

Avoid heap allocations on the audio rendering thread, for performance reasons.

[Chris Dumez]

Created attachment 423269 [details]
WIP Patch

[Yusuke Suzuki]

Comment on attachment 423269 [details]
WIP Patch

How about,
1. Adding a `setForbidMallocUseInCurrentThread(bool)`
2. Use ForbidMallocUse scoped variable which set and reset the configuration.
such an utility can be useful for the other cases.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #2)
> Comment on attachment 423269 [details]
> WIP Patch
> 
> How about,
> 1. Adding a `setForbidMallocUseInCurrentThread(bool)`
> 2. Use ForbidMallocUse scoped variable which set and reset the configuration.
> such an utility can be useful for the other cases.

BTW, in JSC, we have DisallowScope<T> and some implementations like, DisallowGC.

[Chris Dumez]

(In reply to Yusuke Suzuki from comment #2)
> Comment on attachment 423269 [details]
> WIP Patch
> 
> How about,
> 1. Adding a `setForbidMallocUseInCurrentThread(bool)`
> 2. Use ForbidMallocUse scoped variable which set and reset the configuration.
> such an utility can be useful for the other cases.

As it turns out, it is useful for WebAudio to turn it off too. I'll make the changes you suggest.

[Chris Dumez]

Created attachment 423279 [details]
WIP Patch

[Chris Dumez]

Created attachment 423285 [details]
WIP Patch

[Chris Dumez]

Created attachment 423540 [details]
WIP Patch

[Chris Dumez]

Created attachment 423630 [details]
WIP Patch

[Chris Dumez]

Created attachment 423648 [details]
WIP Patch

[Chris Dumez]

Created attachment 423672 [details]
Patch

[Chris Dumez]

Created attachment 423733 [details]
Patch

[Chris Dumez]

Created attachment 423765 [details]
Patch

[Chris Dumez]

Created attachment 423923 [details]
Patch

[Chris Dumez]

Created attachment 423932 [details]
Patch

[Chris Dumez]

Created attachment 423937 [details]
Patch

[Chris Dumez]

Created attachment 423948 [details]
Patch

[Darin Adler]

Comment on attachment 423948 [details]
Patch

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

What’s the overall strategy here? Why do we forbid memory allocation and exactly where? When we are done, can there be any remaining exceptions, or do we have to do this 100% to get the result we are looking for?

> Source/WTF/wtf/FastMalloc.cpp:58
> +thread_local static unsigned forbidMallocUseScopeCount;
> +thread_local static unsigned disableMallocRestrictionScopeCount;

Wow, I have never heard of the C++ thread_local keyword until today! For use outside assertions: How does it compare in efficiency to the other kinds of per-thread stuff?

> Source/WTF/wtf/FastMalloc.h:132
> +    ~ForbidMallocUseForCurrentThreadScope() { }

Why not use = default here too? Or omit explicit constructor and destructors entirely?

> Source/WTF/wtf/FastMalloc.h:148
> +    ~DisableMallocRestrictionsForCurrentThreadScope() { }

Why not use = default here too? Or omit explicit constructor and destructors entirely?

> Source/WebCore/Modules/webaudio/AudioDestinationNode.cpp:90
> +        ForbidMallocUseForCurrentThreadScope forbidMallocUse;

I don’t understand why this is scoped to only cover this part of the rendering and doesn’t cover pre-render and post-render. Should that be obvious?

[Chris Dumez]

(In reply to Darin Adler from comment #17)
> Comment on attachment 423948 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423948&action=review
> 
> What’s the overall strategy here? Why do we forbid memory allocation and
> exactly where? When we are done, can there be any remaining exceptions, or
> do we have to do this 100% to get the result we are looking for?

Basically, the audio thread should avoid waiting on a lock and doing a heap allocation may require grabbing a lock, which would negatively impact audio performance.
As a result, we want to get heap allocations as close to 0 as possible on the audio thread. I don't think we can realistically get to 0 though so I think some exceptions will remain.
However, the assertions will help make sure we avoid introducing new heap allocations without realizing the impact on performance. If someone does introduce such a heap allocation in the
future, they will hit the assertion and will need to either get rid of the heap allocation (ideally) OR add a DisableMallocRestrictionScope to allow it (because no other choice). Because
of the DisableMallocRestrictionScope, it will be obvious when reviewing that we are consciously introducing a new heap allocation on the audio thread.
 
> > Source/WTF/wtf/FastMalloc.cpp:58
> > +thread_local static unsigned forbidMallocUseScopeCount;
> > +thread_local static unsigned disableMallocRestrictionScopeCount;
> 
> Wow, I have never heard of the C++ thread_local keyword until today! For use
> outside assertions: How does it compare in efficiency to the other kinds of
> per-thread stuff?

I did not compare the performance with other kinds of per-thread stuff. I honestly don't have much experience with per-thread stuff. I initially tried to add these counts to WTF::Thread but calling Thread::current() can do a heap allocation and I would end up in an infinite loop :)

> 
> > Source/WTF/wtf/FastMalloc.h:132
> > +    ~ForbidMallocUseForCurrentThreadScope() { }
> 
> Why not use = default here too? Or omit explicit constructor and destructors
> entirely?
> 
> > Source/WTF/wtf/FastMalloc.h:148
> > +    ~DisableMallocRestrictionsForCurrentThreadScope() { }
> 
> Why not use = default here too? Or omit explicit constructor and destructors
> entirely?

Ok, so this is intentional or the compiler complains about unused scope variables. I did not want to have to UNUSED_PARAM() the scope variables.

> > Source/WebCore/Modules/webaudio/AudioDestinationNode.cpp:90
> > +        ForbidMallocUseForCurrentThreadScope forbidMallocUse;
> 
> I don’t understand why this is scoped to only cover this part of the
> rendering and doesn’t cover pre-render and post-render. Should that be
> obvious?

I tried to explain in the changelog but basically, the idea is to expand to cover pre-render / post-render in the future. However, those have quite a few heap allocations and I have not had time to go through those yet.
I figure I should land the assertions ASAP for what's already done and we can work to expand the scope later on.

[Chris Dumez]

(In reply to Chris Dumez from comment #18)
> (In reply to Darin Adler from comment #17)
> > Comment on attachment 423948 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=423948&action=review
> > 
> > What’s the overall strategy here? Why do we forbid memory allocation and
> > exactly where? When we are done, can there be any remaining exceptions, or
> > do we have to do this 100% to get the result we are looking for?
> 
> Basically, the audio thread should avoid waiting on a lock and doing a heap
> allocation may require grabbing a lock, which would negatively impact audio
> performance.
> As a result, we want to get heap allocations as close to 0 as possible on
> the audio thread. I don't think we can realistically get to 0 though so I
> think some exceptions will remain.
> However, the assertions will help make sure we avoid introducing new heap
> allocations without realizing the impact on performance. If someone does
> introduce such a heap allocation in the
> future, they will hit the assertion and will need to either get rid of the
> heap allocation (ideally) OR add a DisableMallocRestrictionScope to allow it
> (because no other choice). Because
> of the DisableMallocRestrictionScope, it will be obvious when reviewing that
> we are consciously introducing a new heap allocation on the audio thread.
>  
> > > Source/WTF/wtf/FastMalloc.cpp:58
> > > +thread_local static unsigned forbidMallocUseScopeCount;
> > > +thread_local static unsigned disableMallocRestrictionScopeCount;
> > 
> > Wow, I have never heard of the C++ thread_local keyword until today! For use
> > outside assertions: How does it compare in efficiency to the other kinds of
> > per-thread stuff?
> 
> I did not compare the performance with other kinds of per-thread stuff. I
> honestly don't have much experience with per-thread stuff. I initially tried
> to add these counts to WTF::Thread but calling Thread::current() can do a
> heap allocation and I would end up in an infinite loop :)
> 
> > 
> > > Source/WTF/wtf/FastMalloc.h:132
> > > +    ~ForbidMallocUseForCurrentThreadScope() { }
> > 
> > Why not use = default here too? Or omit explicit constructor and destructors
> > entirely?
> > 
> > > Source/WTF/wtf/FastMalloc.h:148
> > > +    ~DisableMallocRestrictionsForCurrentThreadScope() { }
> > 
> > Why not use = default here too? Or omit explicit constructor and destructors
> > entirely?
> 
> Ok, so this is intentional or the compiler complains about unused scope
> variables. I did not want to have to UNUSED_PARAM() the scope variables.

I am curious if there is a better way to deal with those compiler warnings in release?

[Radar WebKit Bug Importer]

<rdar://problem/75716380>

[Chris Dumez]

Comment on attachment 423948 [details]
Patch

Clearing flags on attachment: 423948

Committed r274871 (235653@main): <https://commits.webkit.org/235653@main>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.