It's cheaper to check if the thing isn't in the prototype map since allocating a Weak is expensive.
Created attachment 322865 [details] patch
Comment on attachment 322865 [details] patch r=me
Comment on attachment 322865 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=322865&action=review > Source/JavaScriptCore/runtime/PrototypeMapInlines.h:48 > + m_prototypes.set(object, object); Doesn't this do the search twice? Can you use the result iterator from the search instead?
(In reply to JF Bastien from comment #3) > Comment on attachment 322865 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322865&action=review > > > Source/JavaScriptCore/runtime/PrototypeMapInlines.h:48 > > + m_prototypes.set(object, object); > > Doesn't this do the search twice? Can you use the result iterator from the > search instead? Yeah, this is the optimal thing to do. I'll add the necessary API to WeakMap to allow this to happen.
Created attachment 322873 [details] patch for landing
Comment on attachment 322873 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=322873&action=review r=me > Source/JavaScriptCore/runtime/PrototypeMapInlines.h:47 > + auto addResult = m_prototypes.add(object, Weak<JSObject>()); Saam, using auto‽
(In reply to JF Bastien from comment #6) > Comment on attachment 322873 [details] > patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322873&action=review > > r=me > > > Source/JavaScriptCore/runtime/PrototypeMapInlines.h:47 > > + auto addResult = m_prototypes.add(object, Weak<JSObject>()); > > Saam, using auto‽ Always for addResult 😎
Comment on attachment 322873 [details] patch for landing Clearing flags on attachment: 322873 Committed r222929: <http://trac.webkit.org/changeset/222929>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34840746>
This change caused LayoutTests to exit early with an assertion failure: https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/r222929%20(3378)/results.html ASSERTION FAILED: addResult.iterator->value.get() == object ./runtime/PrototypeMapInlines.h(51) : void JSC::PrototypeMap::addPrototype(JSC::JSObject *) 1 0x118ffb06d WTFCrash 2 0x117d5009d JSC::PrototypeMap::addPrototype(JSC::JSObject*) 3 0x117f657e0 JSC::ObjectPrototype::finishCreation(JSC::VM&, JSC::JSGlobalObject*) 4 0x117f67ffe JSC::ObjectPrototype::create(JSC::VM&, JSC::JSGlobalObject*, JSC::Structure*) 5 0x117d8bf6d JSC::JSGlobalObject::init(JSC::VM&) 6 0x117daae7c JSC::JSGlobalObject::finishCreation(JSC::VM&, JSC::JSObject*) 7 0x10d1220fa WebCore::JSDOMGlobalObject::finishCreation(JSC::VM&, JSC::JSObject*) 8 0x10d271058 WebCore::JSDOMWindowBase::finishCreation(JSC::VM&, WebCore::JSDOMWindowProxy*) 9 0x10d19f9b6 WebCore::JSDOMWindow::finishCreation(JSC::VM&, WebCore::JSDOMWindowProxy*) 10 0x10d27a664 WebCore::JSDOMWindow::create(JSC::VM&, JSC::Structure*, WTF::Ref<WebCore::DOMWindow>&&, WebCore::JSDOMWindowProxy*) 11 0x10d27a127 WebCore::JSDOMWindowProxy::setWindow(WTF::RefPtr<WebCore::DOMWindow>&&) 12 0x10e48d30e WebCore::ScriptController::setDOMWindowForWindowProxy(WebCore::DOMWindow*) 13 0x10c8a5c05 WebCore::FrameLoader::clear(WebCore::Document*, bool, bool, bool) 14 0x10c57a739 WebCore::DocumentWriter::begin(WebCore::URL const&, bool, WebCore::Document*) 15 0x10c530f8e WebCore::DocumentLoader::commitData(char const*, unsigned long) 16 0x10c530a25 WebCore::DocumentLoader::finishedLoading() 17 0x10c537e25 WebCore::DocumentLoader::maybeLoadEmpty() 18 0x10c537fad WebCore::DocumentLoader::startLoadingMainResource() 19 0x10c8c0165 WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const&, WebCore::FormState*, bool, WebCore::AllowNavigationToInvalidURL)::$_7::operator()() const 20 0x10c8bfeb9 WTF::Function<void ()>::CallableWrapper<WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const&, WebCore::FormState*, bool, WebCore::AllowNavigationToInvalidURL)::$_7>::call() 21 0x10bd9d9db WTF::Function<void ()>::operator()() const 22 0x10c8ac80d WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const&, WebCore::FormState*, bool, WebCore::AllowNavigationToInvalidURL) 23 0x10c8be9e8 WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WebCore::FormState*, WebCore::AllowNavigationToInvalidURL)::$_5::operator()(WebCore::ResourceRequest const&, WebCore::FormState*, bool) const 24 0x10c8be972 WTF::Function<void (WebCore::ResourceRequest const&, WebCore::FormState*, bool)>::CallableWrapper<WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WebCore::FormState*, WebCore::AllowNavigationToInvalidURL)::$_5>::call(WebCore::ResourceRequest const&, WebCore::FormState*, bool) 25 0x10dfd3bad WTF::Function<void (WebCore::ResourceRequest const&, WebCore::FormState*, bool)>::operator()(WebCore::ResourceRequest const&, WebCore::FormState*, bool) const 26 0x10dfd2bc9 WTF::CompletionHandler<void (WebCore::ResourceRequest const&, WebCore::FormState*, bool)>::operator()(WebCore::ResourceRequest const&, WebCore::FormState*, bool) 27 0x10dfd60b9 WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest const&, bool, WebCore::DocumentLoader*, WebCore::FormState*, WTF::CompletionHandler<void (WebCore::ResourceRequest const&, WebCore::FormState*, bool)>&&)::$_1::operator()(WebCore::PolicyAction) 28 0x10dfd5c8a WTF::Function<void (WebCore::PolicyAction)>::CallableWrapper<WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest const&, bool, WebCore::DocumentLoader*, WebCore::FormState*, WTF::CompletionHandler<void (WebCore::ResourceRequest const&, WebCore::FormState*, bool)>&&)::$_1>::call(WebCore::PolicyAction) 29 0x1055d10b1 WTF::Function<void (WebCore::PolicyAction)>::operator()(WebCore::PolicyAction) const 30 0x105cf69c5 WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(WebCore::NavigationAction const&, WebCore::ResourceRequest const&, bool, WebCore::FormState*, WTF::Function<void (WebCore::PolicyAction)>&&) 31 0x10dfd2a3a WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest const&, bool, WebCore::DocumentLoader*, WebCore::FormState*, WTF::CompletionHandler<void (WebCore::ResourceRequest const&, WebCore::FormState*, bool)>&&) LEAK: 1 WebPageProxy
(In reply to Ryan Haddad from comment #11) > This change caused LayoutTests to exit early with an assertion failure: > https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/ > r222929%20(3378)/results.html > > ASSERTION FAILED: addResult.iterator->value.get() == object > ./runtime/PrototypeMapInlines.h(51) : void > JSC::PrototypeMap::addPrototype(JSC::JSObject *) > 1 0x118ffb06d WTFCrash > 2 0x117d5009d JSC::PrototypeMap::addPrototype(JSC::JSObject*) > 3 0x117f657e0 JSC::ObjectPrototype::finishCreation(JSC::VM&, > JSC::JSGlobalObject*) > 4 0x117f67ffe JSC::ObjectPrototype::create(JSC::VM&, JSC::JSGlobalObject*, > JSC::Structure*) > 5 0x117d8bf6d JSC::JSGlobalObject::init(JSC::VM&) > 6 0x117daae7c JSC::JSGlobalObject::finishCreation(JSC::VM&, JSC::JSObject*) > 7 0x10d1220fa WebCore::JSDOMGlobalObject::finishCreation(JSC::VM&, > JSC::JSObject*) > 8 0x10d271058 WebCore::JSDOMWindowBase::finishCreation(JSC::VM&, > WebCore::JSDOMWindowProxy*) > 9 0x10d19f9b6 WebCore::JSDOMWindow::finishCreation(JSC::VM&, > WebCore::JSDOMWindowProxy*) > 10 0x10d27a664 WebCore::JSDOMWindow::create(JSC::VM&, JSC::Structure*, > WTF::Ref<WebCore::DOMWindow>&&, WebCore::JSDOMWindowProxy*) > 11 0x10d27a127 > WebCore::JSDOMWindowProxy::setWindow(WTF::RefPtr<WebCore::DOMWindow>&&) > 12 0x10e48d30e > WebCore::ScriptController::setDOMWindowForWindowProxy(WebCore::DOMWindow*) > 13 0x10c8a5c05 WebCore::FrameLoader::clear(WebCore::Document*, bool, bool, > bool) > 14 0x10c57a739 WebCore::DocumentWriter::begin(WebCore::URL const&, bool, > WebCore::Document*) > 15 0x10c530f8e WebCore::DocumentLoader::commitData(char const*, unsigned > long) > 16 0x10c530a25 WebCore::DocumentLoader::finishedLoading() > 17 0x10c537e25 WebCore::DocumentLoader::maybeLoadEmpty() > 18 0x10c537fad WebCore::DocumentLoader::startLoadingMainResource() > 19 0x10c8c0165 > WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore:: > ResourceRequest const&, WebCore::FormState*, bool, > WebCore::AllowNavigationToInvalidURL)::$_7::operator()() const > 20 0x10c8bfeb9 WTF::Function<void > ()>::CallableWrapper<WebCore::FrameLoader:: > continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const&, > WebCore::FormState*, bool, > WebCore::AllowNavigationToInvalidURL)::$_7>::call() > 21 0x10bd9d9db WTF::Function<void ()>::operator()() const > 22 0x10c8ac80d > WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore:: > ResourceRequest const&, WebCore::FormState*, bool, > WebCore::AllowNavigationToInvalidURL) > 23 0x10c8be9e8 > WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, > WebCore::FrameLoadType, WebCore::FormState*, > WebCore::AllowNavigationToInvalidURL)::$_5::operator()(WebCore:: > ResourceRequest const&, WebCore::FormState*, bool) const > 24 0x10c8be972 WTF::Function<void (WebCore::ResourceRequest const&, > WebCore::FormState*, > bool)>::CallableWrapper<WebCore::FrameLoader::loadWithDocumentLoader(WebCore: > :DocumentLoader*, WebCore::FrameLoadType, WebCore::FormState*, > WebCore::AllowNavigationToInvalidURL)::$_5>::call(WebCore::ResourceRequest > const&, WebCore::FormState*, bool) > 25 0x10dfd3bad WTF::Function<void (WebCore::ResourceRequest const&, > WebCore::FormState*, bool)>::operator()(WebCore::ResourceRequest const&, > WebCore::FormState*, bool) const > 26 0x10dfd2bc9 WTF::CompletionHandler<void (WebCore::ResourceRequest > const&, WebCore::FormState*, bool)>::operator()(WebCore::ResourceRequest > const&, WebCore::FormState*, bool) > 27 0x10dfd60b9 > WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest > const&, bool, WebCore::DocumentLoader*, WebCore::FormState*, > WTF::CompletionHandler<void (WebCore::ResourceRequest const&, > WebCore::FormState*, bool)>&&)::$_1::operator()(WebCore::PolicyAction) > 28 0x10dfd5c8a WTF::Function<void > (WebCore::PolicyAction)>::CallableWrapper<WebCore::PolicyChecker:: > checkNavigationPolicy(WebCore::ResourceRequest const&, bool, > WebCore::DocumentLoader*, WebCore::FormState*, WTF::CompletionHandler<void > (WebCore::ResourceRequest const&, WebCore::FormState*, > bool)>&&)::$_1>::call(WebCore::PolicyAction) > 29 0x1055d10b1 WTF::Function<void > (WebCore::PolicyAction)>::operator()(WebCore::PolicyAction) const > 30 0x105cf69c5 > WebKit::WebFrameLoaderClient:: > dispatchDecidePolicyForNavigationAction(WebCore::NavigationAction const&, > WebCore::ResourceRequest const&, bool, WebCore::FormState*, > WTF::Function<void (WebCore::PolicyAction)>&&) > 31 0x10dfd2a3a > WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest > const&, bool, WebCore::DocumentLoader*, WebCore::FormState*, > WTF::CompletionHandler<void (WebCore::ResourceRequest const&, > WebCore::FormState*, bool)>&&) > LEAK: 1 WebPageProxy Looking into it. This is really surprising.
This is the bug: ``` void Heap::pruneStaleEntriesFromWeakGCMaps() { if (m_collectionScope != CollectionScope::Full) return; for (auto& pruneCallback : m_weakGCMaps.values()) pruneCallback(); } ``` But the WeakGCMap we use is: WeakGCMap<JSObject*, JSObject*> which really is HashMap<JSObject*, Weak<JSObject>> It's really suspicious we don't clear entries in this map on every GC. I'm working on a more fundamental fix in: https://bugs.webkit.org/show_bug.cgi?id=177907
Reverted r222929 for reason: Caused assertion failures during LayoutTests. Committed r222939: <http://trac.webkit.org/changeset/222939>
*** This bug has been marked as a duplicate of bug 177907 ***
(In reply to Saam Barati from comment #13) > This is the bug: > > ``` > void Heap::pruneStaleEntriesFromWeakGCMaps() > { > if (m_collectionScope != CollectionScope::Full) > return; > for (auto& pruneCallback : m_weakGCMaps.values()) > pruneCallback(); > } > ``` > But the WeakGCMap we use is: > WeakGCMap<JSObject*, JSObject*> > > which really is > HashMap<JSObject*, Weak<JSObject>> > > It's really suspicious we don't clear entries in this map on every GC. I'm > working on a more fundamental fix in: > https://bugs.webkit.org/show_bug.cgi?id=177907 No, your patch was wrong. You should have said: auto addResult = m_prototypes.add(object, Weak<JSObject>()); if (addResult.isNewEntry || !addResult.iterator->value) addResult.iterator->value = Weak<JSObject>(object); else ASSERT(addResult.iterator->value.get() == object); Pruning a WeakGCMap is an optimization. All methods in WeakGCMap must assume that an entry with a null value is as if the entry was not there at all.
Comment on attachment 322873 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=322873&action=review > Source/JavaScriptCore/runtime/PrototypeMapInlines.h:48 > + if (addResult.isNewEntry) This should have also checked if the value is null.
I'll fix this in https://bugs.webkit.org/show_bug.cgi?id=178051