229680 2021-08-30 12:24:17 -0700 REGRESSION (r272900): wpt.fyi loading performance is very slow (regressed, and slower than other browsers) 2021-09-06 14:12:48 -0700 1 1 1 Unclassified WebKit DOM Safari Technology Preview Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 229960 148695 221386 1 simon.fraser webkit-unassigned ap cdumez cgarcia changseok darin esprehn+autocc ews-watchlist glenn gsnedders koivisto kondapallykalyan pdr rniwa simon.fraser webkit-bug-importer oldest_to_newest 1788613 0 simon.fraser 2021-08-30 12:24:17 -0700 Load https://wpt.fyi/results/?label=master&label=experimental&aligned. Scroll down and click on the link for the css/ tests. Note how long it takes to load the new page. Before r272900: a few seconds. After 272900: many tens of seconds. 1788616 1 simon.fraser 2021-08-30 12:25:58 -0700 Sample shows: + ! 1620 WebCore::Element::dispatchMouseEvent(WebCore::PlatformMouseEvent const&, WTF::AtomString const&, int, WebCore::Element*, WebCore::IsSyntheticClick) (in WebCore) + 802 [0x10814d432] + ! 1620 WebCore::EventDispatcher::dispatchEvent(WebCore::Node&, WebCore::Event&) (in WebCore) + 4771 [0x1081683a3] + ! 1620 WebCore::dispatchEventInDOM(WebCore::Event&, WebCore::EventPath const&) (in WebCore) + 240 [0x108168e40] + ! 1620 WebCore::EventTarget::fireEventListeners(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) (in WebCore) + 611 [0x10816c3e3] + ! 1620 WebCore::EventTarget::innerInvokeEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::RawPtrTraits<WebCore::RegisteredEventListener>, WTF::DefaultRefDerefTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>, WebCore::EventTarget::EventInvokePhase) (in WebCore) + 428 [0x10816cb6c] + ! 1507 WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&) (in WebCore) + 1965 [0x107e286ad] + ! : 1507 WebCore::JSExecState::~JSExecState() (in WebCore) + 214 [0x107e2c756] + ! : 1507 WebCore::MicrotaskQueue::performMicrotaskCheckpoint() (in WebCore) + 149 [0x108181d35] + ! : 1507 WTF::Detail::CallableWrapper<WebCore::WindowEventLoop::queueMutationObserverCompoundMicrotask()::$_0, void>::call() (in WebCore) + 71 [0x1081e6797] + ! : 1507 WebCore::MutationObserver::notifyMutationObservers(WebCore::WindowEventLoop&) (in WebCore) + 2255 [0x10818844f] + ! : 1507 WebCore::JSMutationCallback::handleEvent(WebCore::MutationObserver&, WTF::Vector<WTF::Ref<WebCore::MutationRecord, WTF::RawPtrTraits<WebCore::MutationRecord> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, WebCore::MutationObserver&) (in WebCore) + 515 [0x10758deb3] + ! : 1507 WebCore::JSCallbackData::invokeCallback(WebCore::JSDOMGlobalObject&, JSC::JSObject*, JSC::JSValue, JSC::MarkedArgumentBuffer&, WebCore::JSCallbackData::CallbackType, JSC::PropertyName, WTF::NakedPtr<JSC::Exception>&) (in WebCore) + 611 [0x107e10b33] + ! : 1507 JSC::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (in JavaScriptCore) + 174 [0x10bb8bdce] + ! : 1507 JSC::Interpreter::executeCall(JSC::JSGlobalObject*, JSC::JSObject*, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (in JavaScriptCore) + 555 [0x10b92c81b] + ! : 1507 vmEntryToJavaScript (in JavaScriptCore) + 216 [0x10b251326] + ! : 1501 ??? (in <unknown binary>) [0x3a716bc303aa] + ! : | 1501 ??? (in <unknown binary>) [0x3a716ba2c317] + ! : | 1499 ??? (in <unknown binary>) [0x3a716be87fca] + ! : | + 1452 ??? (in <unknown binary>) [0x3a716bc177d8] + ! : | + ! 1186 ??? (in <unknown binary>) [0x3a716bdfa779] + ! : | + ! : 1186 ??? (in <unknown binary>) [0x3a716ba011d8] + ! : | + ! : 1058 WebCore::jsNodePrototypeFunction_insertBefore(JSC::JSGlobalObject*, JSC::CallFrame*) (in WebCore) + 392 [0x1075bb4a8] + ! : | + ! : | 718 WebCore::ContainerNode::insertBefore(WebCore::Node&, WebCore::Node*) (in WebCore) + 1697 [0x1080ea671] + ! : | + ! : | + 718 WebCore::HTMLElement::childrenChanged(WebCore::ContainerNode::ChildChange const&) (in WebCore) + 18 [0x10834fcb2] + ! : | + ! : | + 697 WebCore::SlotAssignment::didChangeSlot(WTF::AtomString const&, WebCore::ShadowRoot&) (in WebCore) + 260 [0x1081c9f24] + ! : | + ! : | + ! 364 WebCore::RenderTreeUpdater::tearDownRenderers(WebCore::Element&, WebCore::RenderTreeUpdater::TeardownType, WebCore::RenderTreeBuilder&) (in WebCore) + 332 [0x108cd7f3c] + ! : | + ! : | + ! : 106 WebCore::ComposedTreeIterator::traverseNextInShadowTree() (in WebCore) + 96 [0x1080e33a0] + ! : | + ! : | + ! : | 106 WebCore::ElementAndTextDescendantIterator::traverseNext() (in WebCore) + 196,45,... [0x1080e37b4,0x1080e371d,...] + ! : | + ! : | + ! : 67 WebCore::ComposedTreeIterator::traverseNextInShadowTree() (in WebCore) + 192 [0x1080e3400] + ! : | + ! : | + ! : | 67 WebCore::HTMLSlotElement::assignedNodes() const (in WebCore) + 64 [0x1083dd940] + ! : | + ! : | + ! : | 67 WebCore::SlotAssignment::assignedNodesForSlot(WebCore::HTMLSlotElement const&, WebCore::ShadowRoot&) (in WebCore) + 353 [0x1081c6181] + ! : | + ! : | + ! : | 60 WebCore::SlotAssignment::assignSlots(WebCore::ShadowRoot&) (in WebCore) + 258 [0x1081c9032] + ! : | + ! : | + ! : | + 34 WebCore::SlotAssignment::assignToSlot(WebCore::Node&, WTF::AtomString const&) (in WebCore) + 730,340,... [0x1081ca63a,0x1081ca4b4,...] 1788617 2 webkit-bug-importer 2021-08-30 12:26:15 -0700 <rdar://problem/82541045> 1788649 3 simon.fraser 2021-08-30 14:01:56 -0700 <rdar://81789435> 1788827 4 cgarcia 2021-08-31 02:12:59 -0700 hmm, this happens because a lot of elements are added to the same shadow root, so didChangeDefaultSlot() and hostChildElementDidChange() are called too many times. Iterating the composed tree every time makes things super slow. I don't think we need to tear down renderers on every didChangeSlot call, only the ones coming from hostChildElementDidChange() and not in all those cases either. IIRC the problem was only when replacing a previous tree and there was a node with display contents. So, I think we need to find a more specific solution. 1788872 5 436877 koivisto 2021-08-31 05:07:13 -0700 Created attachment 436877 patch 1788877 6 436877 cgarcia 2021-08-31 05:44:28 -0700 Comment on attachment 436877 patch View in context: https://bugs.webkit.org/attachment.cgi?id=436877&action=review > Source/WebCore/rendering/updating/RenderTreeUpdater.cpp:544 > + if (!host.renderer() && !host.hasDisplayContents()) > + return; Oh, good catch! this is consistent with destroyRenderTreeIfNeeded(). 1788879 7 koivisto 2021-08-31 05:50:37 -0700 > Oh, good catch! this is consistent with destroyRenderTreeIfNeeded(). This test should/could probably be in tearDownRenderers but lets do a minimal fix here. 1788975 8 436877 darin 2021-08-31 11:06:48 -0700 Comment on attachment 436877 patch Any good way to regression-test this performance optimization? Maybe we could make a test that is *super-slow* if this is broken. 1789008 9 koivisto 2021-08-31 12:03:46 -0700 > Any good way to regression-test this performance optimization? Maybe we > could make a test that is *super-slow* if this is broken. Those sort of benchmark is layout tests have often been problematic (hard to make reliable and speedy enough). We definitely should have coverage in our regular performance tests for Shadow DOM as it is getting increasingly popular. Speedometer3 is apparently planned to have some. It might also make sense to add microbenchmarks. 1789009 10 ews-feeder 2021-08-31 12:04:26 -0700 Committed r281813 (241149@main): <https://commits.webkit.org/241149@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 436877. 1789052 11 darin 2021-08-31 13:32:50 -0700 (In reply to Antti Koivisto from comment #9) > > Any good way to regression-test this performance optimization? Maybe we > > could make a test that is *super-slow* if this is broken. > > Those sort of benchmark is layout tests have often been problematic (hard to > make reliable and speedy enough). We definitely should have coverage in our > regular performance tests for Shadow DOM as it is getting increasingly > popular. Speedometer3 is apparently planned to have some. It might also make > sense to add microbenchmarks. Seems to me that this optimization for this issue is so huge, it lends itself to a fast test that would simply hang without the optimization. Maybe that’s not right? 1789271 12 koivisto 2021-09-01 02:14:22 -0700 Yeah, maybe. I'll try. 1789365 13 gsnedders 2021-09-01 10:25:00 -0700 Yeah, for anything like this where we're doing > O(n) work it's not too hard to write a regression test; I'm against writing tests for checking whether O(n) differs from O(n), but O(n) (or O(1) in this case) versus O(n^2) is pretty easy to detect. 1790697 14 koivisto 2021-09-06 07:06:06 -0700 Perf test in 229960 436877 2021-08-31 05:07:13 -0700 2021-08-31 12:04:28 -0700 patch repeated-teardown-on-slot-change.patch text/plain 1685 koivisto ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAyMzkwNTVkYTVmNTUuLjI4M2RkODNmY2NkOSAxMDA2NDQKLS0tIGEvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAt MSwzICsxLDE5IEBACisyMDIxLTA4LTMxICBBbnR0aSBLb2l2aXN0byAgPGFudHRpQGFwcGxlLmNv bT4KKworICAgICAgICBSRUdSRVNTSU9OIChyMjcyOTAwKTogd3B0LmZ5aSBsb2FkaW5nIHBlcmZv cm1hbmNlIGlzIHZlcnkgc2xvdyAocmVncmVzc2VkLCBhbmQgc2xvd2VyIHRoYW4gb3RoZXIgYnJv d3NlcnMpCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0y Mjk2ODAKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzgyNTQxMDQ1PgorCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoZSBwYWdlIGlzIGluc2VydGluZyBu ZXcgY2hpbGRyZW4gdG8gc2hhZG93IGhvc3QgYW5kIG9uIGVhY2ggaW5zZXJ0aW9uIHdlIGFyZSB0 cmF2ZXJzaW5nIHRoZSBjb21wb3NlZAorICAgICAgICB0cmVlIHRvIHRlYXIgZG93biByZW5kZXJl cnMsIGV2ZW4gdGhvdWdoIHRoZXJlIGFyZSBub25lLgorCisgICAgICAgICogcmVuZGVyaW5nL3Vw ZGF0aW5nL1JlbmRlclRyZWVVcGRhdGVyLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlJlbmRlclRy ZWVVcGRhdGVyOjp0ZWFyRG93blJlbmRlcmVyc0FmdGVyU2xvdENoYW5nZSk6CisKKyAgICAgICAg SWYgdGhlIGhvc3QgZG9lc24ndCBoYXZlIGEgcmVuZGVyZXIgb3IgJ2Rpc3BsYXk6Y29udGVudHMn IHRoZXJlIGNhbid0IGJlIGFueSByZW5kZXJlcnMgbGVmdCBpbiB0aGUgc3VidHJlZS4KKwogMjAy MS0wOC0yOSAgQWxhbiBCdWp0YXMgIDx6YWxhbkBhcHBsZS5jb20+CiAKICAgICAgICAgW0xGQ11b SUZDXSBNb3ZlICJsaW5lIG5lZWRzIGludGVncmFsIHNuYXBwaW5nIiBjb21wdXRpbmcgdG8gSUZD IGZyb20gdGhlIGludGVncmF0aW9uIGxheWVyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9y ZW5kZXJpbmcvdXBkYXRpbmcvUmVuZGVyVHJlZVVwZGF0ZXIuY3BwIGIvU291cmNlL1dlYkNvcmUv cmVuZGVyaW5nL3VwZGF0aW5nL1JlbmRlclRyZWVVcGRhdGVyLmNwcAppbmRleCA2OWE5ZWEwMTJh ZTQuLjhjMmJjNGVmNGM0OSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL3Vw ZGF0aW5nL1JlbmRlclRyZWVVcGRhdGVyLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJp bmcvdXBkYXRpbmcvUmVuZGVyVHJlZVVwZGF0ZXIuY3BwCkBAIC01NDAsNiArNTQwLDggQEAgdm9p ZCBSZW5kZXJUcmVlVXBkYXRlcjo6dGVhckRvd25SZW5kZXJlcnMoRWxlbWVudCYgcm9vdCkKIHZv aWQgUmVuZGVyVHJlZVVwZGF0ZXI6OnRlYXJEb3duUmVuZGVyZXJzQWZ0ZXJTbG90Q2hhbmdlKEVs ZW1lbnQmIGhvc3QpCiB7CiAgICAgQVNTRVJUKGhvc3Quc2hhZG93Um9vdCgpKTsKKyAgICBpZiAo IWhvc3QucmVuZGVyZXIoKSAmJiAhaG9zdC5oYXNEaXNwbGF5Q29udGVudHMoKSkKKyAgICAgICAg cmV0dXJuOwogICAgIGF1dG8qIHZpZXcgPSBob3N0LmRvY3VtZW50KCkucmVuZGVyVmlldygpOwog ICAgIGlmICghdmlldykKICAgICAgICAgcmV0dXJuOwo=