Avoid heap allocations on the audio rendering thread, for performance reasons.
Created attachment 423269 [details] WIP Patch
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.
(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.
(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.
Created attachment 423279 [details] WIP Patch
Created attachment 423285 [details] WIP Patch
Created attachment 423540 [details] WIP Patch
Created attachment 423630 [details] WIP Patch
Created attachment 423648 [details] WIP Patch
Created attachment 423672 [details] Patch
Created attachment 423733 [details] Patch
Created attachment 423765 [details] Patch
Created attachment 423923 [details] Patch
Created attachment 423932 [details] Patch
Created attachment 423937 [details] Patch
Created attachment 423948 [details] Patch
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?
(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.
(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?
<rdar://problem/75716380>
Comment on attachment 423948 [details] Patch Clearing flags on attachment: 423948 Committed r274871 (235653@main): <https://commits.webkit.org/235653@main>
All reviewed patches have been landed. Closing bug.