Loading the mdn-bcd-collector sometimes (but not always) asserts here in debug. This implies that feature detection can sometimes assert, which doesn’t seem great. <rdar://87980379>
Are you sure the link is correct? Not only I am unable to reproduce but also, logging shows AudioWorkletProcessor::process() is not even called.
Ok, now I can reproduce. You provided an http:// link but this can only reproduce with an https:// link it seems.
Created attachment 450045 [details] Patch
Created attachment 450053 [details] Patch
Comment on attachment 450053 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450053&action=review > Source/WebCore/ChangeLog:14 > + To address the issue, I added a visitor to for the AudioWorkletGlobalScope wrapper s/to for the/to the/
Created attachment 450060 [details] Patch
Comment on attachment 450060 [details] Patch r=me
Maybe AudioWorkletProcessor should inherit from ScriptWrappable. That's a slightly more idiomatic way to point to your own wrapper. (The semantics are ultimately the same.)
(In reply to Geoffrey Garen from comment #8) > Maybe AudioWorkletProcessor should inherit from ScriptWrappable. That's a > slightly more idiomatic way to point to your own wrapper. (The semantics are > ultimately the same.) If I use ScriptWrappable, how does the AudioWorkletGlobalScope "visit" the AudioWorkletProcessor then? I don't see a visit function on ScriptWrappable. I *think* I should be able to call SlotVisitor::append() with the Weak<> data member as parameter. However, ScriptWrappable doesn't have a getter for that Weak<> (only one that returns the raw pointer).
Created attachment 450139 [details] Patch
Comment on attachment 450139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450139&action=review Ok, I think I got it but Geoff, could you please double check? > Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.cpp:216 > + visitor.addOpaqueRoot(&processor); I now call addOpaqueRoot() here. > Source/WebCore/Modules/webaudio/AudioWorkletProcessor.cpp:255 > + auto result = JSCallbackData::invokeCallback(vm, wrapper(), jsUndefined(), args, JSCallbackData::CallbackType::Object, Identifier::fromString(vm, "process"), returnedException); I now use ScriptWrappable::wrapper() here instead of m_processCallback. > Source/WebCore/Modules/webaudio/AudioWorkletProcessor.h:56 > +class AudioWorkletProcessor : public ScriptWrappable, public ThreadSafeRefCounted<AudioWorkletProcessor>, public CanMakeWeakPtr<AudioWorkletProcessor> { AudioWorkletProcessor how subclasses ScriptWrappable. > Source/WebCore/Modules/webaudio/AudioWorkletProcessor.h:-76 > - JSValueInWrappedObject m_processCallback; I dropped the m_processCallback data member that is no longer needed now that I can get the JS wrapper from ScriptWrappable::wrapper(). > Source/WebCore/Modules/webaudio/AudioWorkletProcessor.idl:32 > + GenerateIsReachable=Impl, I added GenerateIsReachable=Impl on AudioWorkletProcessor since AudioWorkletGlobalScope adds the processors as opaque roots when visiting.
Comment on attachment 450139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450139&action=review I’ll let Geoff do the actual r+ >> Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.cpp:216 >> + visitor.addOpaqueRoot(&processor); > > I now call addOpaqueRoot() here. I guess this is specifically a SlotVisitor, not a generic Visitor, which would be more of a arbitrary function, not an object to call addOpaqueRoot on. In the past, there have been some lock-free wants to do marking/visiting, and that was sort of an overall goal for our GC system. Not saying I know how to achieve that here, or that it’s really important. > Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.h:61 > + template<typename Visitor> void visitProcessors(Visitor&); I think it’s worth using the name SlotVisitor here to be a little more explicit. > Source/WebCore/bindings/js/JSAudioWorkletGlobalScopeCustom.cpp:35 > +using namespace JSC; I think it’s nicer to just do JSC:: as needed rather than this, especially on a new source file.
(In reply to Darin Adler from comment #12) > Comment on attachment 450139 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=450139&action=review > > I’ll let Geoff do the actual r+ > > >> Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.cpp:216 > >> + visitor.addOpaqueRoot(&processor); > > > > I now call addOpaqueRoot() here. > > I guess this is specifically a SlotVisitor, not a generic Visitor, which > would be more of a arbitrary function, not an object to call addOpaqueRoot > on. I am not sure I understand this comment. #define DEFINE_VISIT_ADDITIONAL_CHILDREN(className) \ template void className::visitAdditionalChildren(JSC::AbstractSlotVisitor&); \ template void className::visitAdditionalChildren(JSC::SlotVisitor&) So I need to be able to deal with AbstractSlotVisitor too, no? > > In the past, there have been some lock-free wants to do marking/visiting, > and that was sort of an overall goal for our GC system. Not saying I know > how to achieve that here, or that it’s really important. > > > Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.h:61 > > + template<typename Visitor> void visitProcessors(Visitor&); > > I think it’s worth using the name SlotVisitor here to be a little more > explicit. > > > Source/WebCore/bindings/js/JSAudioWorkletGlobalScopeCustom.cpp:35 > > +using namespace JSC; > > I think it’s nicer to just do JSC:: as needed rather than this, especially > on a new source file. Ok.
Comment on attachment 450139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450139&action=review >>>> Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.cpp:216 >>>> + visitor.addOpaqueRoot(&processor); >>> >>> I now call addOpaqueRoot() here. >> >> I guess this is specifically a SlotVisitor, not a generic Visitor, which would be more of a arbitrary function, not an object to call addOpaqueRoot on. >> >> In the past, there have been some lock-free wants to do marking/visiting, and that was sort of an overall goal for our GC system. Not saying I know how to achieve that here, or that it’s really important. > > I am not sure I understand this comment. > #define DEFINE_VISIT_ADDITIONAL_CHILDREN(className) \ > template void className::visitAdditionalChildren(JSC::AbstractSlotVisitor&); \ > template void className::visitAdditionalChildren(JSC::SlotVisitor&) > > So I need to be able to deal with AbstractSlotVisitor too, no? Yes, AbstractSlotVisitor, but I was simply pointing out that the Visitor pattern generally means a function object, not an object of class AbstractSlotVisitor.
Comment on attachment 450139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450139&action=review >>>>> Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.cpp:216 >>>>> + visitor.addOpaqueRoot(&processor); >>>> >>>> I now call addOpaqueRoot() here. >>> >>> I guess this is specifically a SlotVisitor, not a generic Visitor, which would be more of a arbitrary function, not an object to call addOpaqueRoot on. >>> >>> In the past, there have been some lock-free wants to do marking/visiting, and that was sort of an overall goal for our GC system. Not saying I know how to achieve that here, or that it’s really important. >> >> I am not sure I understand this comment. >> #define DEFINE_VISIT_ADDITIONAL_CHILDREN(className) \ >> template void className::visitAdditionalChildren(JSC::AbstractSlotVisitor&); \ >> template void className::visitAdditionalChildren(JSC::SlotVisitor&) >> >> So I need to be able to deal with AbstractSlotVisitor too, no? > > Yes, AbstractSlotVisitor, but I was simply pointing out that the Visitor pattern generally means a function object, not an object of class AbstractSlotVisitor. Sorry, I am still not following. Should I update AudioWorkletGlobalScope::visitProcessors() to take in a JSC::AbstractSlotVisitor& and drop the templating? It seems SlotVisitor inherits from AbstractSlotVisitor and addOpaqueRoot() is defined on AbstractSlotVisitor anyway.
(In reply to Chris Dumez from comment #15) > Sorry, I am still not following. My comment was intended to only be about naming. > Should I update AudioWorkletGlobalScope::visitProcessors() to take in a > JSC::AbstractSlotVisitor& and drop the templating? It seems SlotVisitor > inherits from AbstractSlotVisitor and addOpaqueRoot() is defined on > AbstractSlotVisitor anyway. Yes, you probably should! I was not suggesting it, but I think you have hit on something. Unless there is a significant performance benefit to using the template function.
Created attachment 450154 [details] Patch
Ok. I fixed the naming though some existing code likely needs updating too. E.g.: ``` void Range::visitNodesConcurrently(JSC::AbstractSlotVisitor& visitor) const { visitor.addOpaqueRoot(root(&m_start.container())); visitor.addOpaqueRoot(root(&m_end.container())); } ```
I was referring to the name of the template argument.
Not the name of the function.
(In reply to Darin Adler from comment #20) > Not the name of the function. Ahhh you wanted me to rename the template parameter from "Visitor" to "SlotVisitor", I get it now. I got confused because I didn't realize you were talking about the template parameter and JSC::SlotVisitor is an actual type. Well, in my latest iteration the template parameter is gone anyway. I did rename the function as well from visitProcessors() to addProcessorsToOpaqueRoots(). I am not sure which one is better. I found both patterns in our code base.
Created attachment 450161 [details] Patch
Comment on attachment 450161 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450161&action=review > Source/WTF/wtf/WeakHashSet.h:101 > + return m_set.add(*static_cast<const T&>(value).weakPtrFactory().template createWeakPtr<T>(const_cast<U&>(value), enableWeakPtrThreadingAssertions).m_impl); This seems like such a long argument name. Is there any shorter name that would be sufficiently unambiguous? I might just call the local "assertions" and not worry that was too confusing. After all we still have the strong typing of EnableWeakPtrThreadingAssertions that will follow us around. > Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.cpp:216 > + Locker locker { m_processorsLock }; > + m_processors.forEach([&](auto& processor) { > + visitor.addOpaqueRoot(&processor); > + }); If I understand correctly, it’s generally bad for performance to have to take locks during GC marking. Would be nice to have some lock-free safe way to do this. > Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.h:61 > + void processorIsNoLongerNeeded(AudioWorkletProcessor&); > + > + void visitProcessors(JSC::AbstractSlotVisitor&); Not sure these need to be in separate paragraphs. > Source/WebCore/Modules/webaudio/AudioWorkletProcessor.h:37 > +#include <wtf/Lock.h> Any way to avoid needing this new include in the header? I don’t see any new use of Lock below.
Comment on attachment 450161 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450161&action=review >> Source/WTF/wtf/WeakHashSet.h:101 >> + return m_set.add(*static_cast<const T&>(value).weakPtrFactory().template createWeakPtr<T>(const_cast<U&>(value), enableWeakPtrThreadingAssertions).m_impl); > > This seems like such a long argument name. Is there any shorter name that would be sufficiently unambiguous? I might just call the local "assertions" and not worry that was too confusing. After all we still have the strong typing of EnableWeakPtrThreadingAssertions that will follow us around. Or "assertionsPolicy" or "policy".
Comment on attachment 450161 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450161&action=review >>> Source/WTF/wtf/WeakHashSet.h:101 >>> + return m_set.add(*static_cast<const T&>(value).weakPtrFactory().template createWeakPtr<T>(const_cast<U&>(value), enableWeakPtrThreadingAssertions).m_impl); >> >> This seems like such a long argument name. Is there any shorter name that would be sufficiently unambiguous? I might just call the local "assertions" and not worry that was too confusing. After all we still have the strong typing of EnableWeakPtrThreadingAssertions that will follow us around. > > Or "assertionsPolicy" or "policy". Ok, will go with assertionsPolicy. >> Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.cpp:216 >> + }); > > If I understand correctly, it’s generally bad for performance to have to take locks during GC marking. Would be nice to have some lock-free safe way to do this. Not sure how to avoid the locking at this point since this iterates a WeakHashSet on a GC thread and the WeakHashSet gets modified on the audio worklet thread. I asked on the JSC channel to see if there are suggestions. >> Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.h:61 >> + void visitProcessors(JSC::AbstractSlotVisitor&); > > Not sure these need to be in separate paragraphs. OK >> Source/WebCore/Modules/webaudio/AudioWorkletProcessor.h:37 >> +#include <wtf/Lock.h> > > Any way to avoid needing this new include in the header? I don’t see any new use of Lock below. Indeed, this is a reminiscence of an earlier patch iteration. Will drop.
Comment on attachment 450161 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450161&action=review >>> Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.cpp:216 >>> + }); >> >> If I understand correctly, it’s generally bad for performance to have to take locks during GC marking. Would be nice to have some lock-free safe way to do this. > > Not sure how to avoid the locking at this point since this iterates a WeakHashSet on a GC thread and the WeakHashSet gets modified on the audio worklet thread. I asked on the JSC channel to see if there are suggestions. I hope to learn about this, too; what are the best techniques for a case like this. Seems likely we will land it like this, but want to do the best long term.
(In reply to Darin Adler from comment #23) > Comment on attachment 450161 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=450161&action=review > > > Source/WTF/wtf/WeakHashSet.h:101 > > + return m_set.add(*static_cast<const T&>(value).weakPtrFactory().template createWeakPtr<T>(const_cast<U&>(value), enableWeakPtrThreadingAssertions).m_impl); > > This seems like such a long argument name. Is there any shorter name that > would be sufficiently unambiguous? I might just call the local "assertions" > and not worry that was too confusing. After all we still have the strong > typing of EnableWeakPtrThreadingAssertions that will follow us around. > > > Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.cpp:216 > > + Locker locker { m_processorsLock }; > > + m_processors.forEach([&](auto& processor) { > > + visitor.addOpaqueRoot(&processor); > > + }); > > If I understand correctly, it’s generally bad for performance to have to > take locks during GC marking. Would be nice to have some lock-free safe way > to do this. It's only bad for perf if we do it a lot. How many of these objects will be live at any time? During a full GC that walks the whole heap, how many AudioWorkletGlobalScopes are we going to see? If it's between 0 and 100 then grabbing a lock is fine. If it's more than 100 then maybe grabbing a lock is bad. > > > Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.h:61 > > + void processorIsNoLongerNeeded(AudioWorkletProcessor&); > > + > > + void visitProcessors(JSC::AbstractSlotVisitor&); > > Not sure these need to be in separate paragraphs. > > > Source/WebCore/Modules/webaudio/AudioWorkletProcessor.h:37 > > +#include <wtf/Lock.h> > > Any way to avoid needing this new include in the header? I don’t see any new > use of Lock below.
(In reply to Darin Adler from comment #26) > Comment on attachment 450161 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=450161&action=review > > >>> Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.cpp:216 > >>> + }); > >> > >> If I understand correctly, it’s generally bad for performance to have to take locks during GC marking. Would be nice to have some lock-free safe way to do this. > > > > Not sure how to avoid the locking at this point since this iterates a WeakHashSet on a GC thread and the WeakHashSet gets modified on the audio worklet thread. I asked on the JSC channel to see if there are suggestions. > > I hope to learn about this, too; what are the best techniques for a case > like this. Seems likely we will land it like this, but want to do the best > long term. JSC's own object model falls back on locks for interactions with GC when the alternatives are too crazy. JSC uses lock-free tricks for scanning the most frequently allocated objects - like plain JS objects and plain JS arrays. But even for those, when they get into some highly dynamic state (like they are dictionary objects), we use locks. In fact, each JSObject has a two-bit lock in its header for those cases. So, I would go ahead and use locks in cases like this, unless you think that there will really be a lot of instances of the object, and you have a good idea for how to avoid locking.
(In reply to Filip Pizlo from comment #27) > (In reply to Darin Adler from comment #23) > > Comment on attachment 450161 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=450161&action=review > > > > > Source/WTF/wtf/WeakHashSet.h:101 > > > + return m_set.add(*static_cast<const T&>(value).weakPtrFactory().template createWeakPtr<T>(const_cast<U&>(value), enableWeakPtrThreadingAssertions).m_impl); > > > > This seems like such a long argument name. Is there any shorter name that > > would be sufficiently unambiguous? I might just call the local "assertions" > > and not worry that was too confusing. After all we still have the strong > > typing of EnableWeakPtrThreadingAssertions that will follow us around. > > > > > Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.cpp:216 > > > + Locker locker { m_processorsLock }; > > > + m_processors.forEach([&](auto& processor) { > > > + visitor.addOpaqueRoot(&processor); > > > + }); > > > > If I understand correctly, it’s generally bad for performance to have to > > take locks during GC marking. Would be nice to have some lock-free safe way > > to do this. > > It's only bad for perf if we do it a lot. > > How many of these objects will be live at any time? During a full GC that > walks the whole heap, how many AudioWorkletGlobalScopes are we going to see? > > If it's between 0 and 100 then grabbing a lock is fine. > > If it's more than 100 then maybe grabbing a lock is bad. AudioWorkletGlobalScope's wrapper is the global object so there is a single AudioWorkletGlobalScope for this VM which lives on the AudioWorklet thread. Also, I don't expect contention here as registering / unregistering AudioWorkletProcessors is a rare operation. > > > > > > Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.h:61 > > > + void processorIsNoLongerNeeded(AudioWorkletProcessor&); > > > + > > > + void visitProcessors(JSC::AbstractSlotVisitor&); > > > > Not sure these need to be in separate paragraphs. > > > > > Source/WebCore/Modules/webaudio/AudioWorkletProcessor.h:37 > > > +#include <wtf/Lock.h> > > > > Any way to avoid needing this new include in the header? I don’t see any new > > use of Lock below.
Created attachment 450257 [details] Patch
Committed r288759 (246547@main): <https://commits.webkit.org/246547@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 450257 [details].