| Summary: | Don't always do a style recalc for display: contents elements. | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Emilio Cobos Álvarez <ecobos> |
| Component: | CSS | Assignee: | Emilio Cobos Álvarez <ecobos> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | buildbot, cdumez, cmarcelo, commit-queue, dbates, dewei_zhu, esprehn+autocc, kangil.han, koivisto, krollin, rniwa, webkit-bug-importer |
| Priority: | P2 | Keywords: | InRadar |
| Version: | WebKit Nightly Build | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Bug Depends on: | 172822 | ||
| Bug Blocks: | |||
| Attachments: | |||
|
Description
Emilio Cobos Álvarez
2017-05-31 08:26:59 PDT
Created attachment 311594 [details]
Patch
Comment on attachment 311594 [details] Patch Attachment 311594 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3848731 New failing tests: fast/shadow-dom/shadow-host-transition.html imported/blink/fast/css-generated-content/summary-before-after-content.html fast/shadow-dom/css-scoping-slot-with-id.html fast/shadow-dom/shadow-host-animation.html fast/shadow-dom/input-element-in-shadow.html fast/shadow-dom/shadow-layout-after-slot-fallback-changes.html fast/shadow-dom/css-scoping-host-and-slotted-context-invalidation.html fast/html/details-add-summary-child-1.html fast/shadow-dom/css-scoping-slotted-invalidation.html fast/html/details-replace-summary-child.html Created attachment 311599 [details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 311594 [details] Patch Attachment 311594 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3848745 New failing tests: fast/shadow-dom/shadow-host-transition.html imported/blink/fast/css-generated-content/summary-before-after-content.html fast/shadow-dom/css-scoping-slot-with-id.html fast/shadow-dom/shadow-host-animation.html fast/shadow-dom/input-element-in-shadow.html fast/shadow-dom/shadow-layout-after-slot-fallback-changes.html fast/shadow-dom/css-scoping-host-and-slotted-context-invalidation.html fast/html/details-add-summary-child-1.html fast/shadow-dom/css-scoping-slotted-invalidation.html fast/html/details-replace-summary-child.html Created attachment 311601 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 311594 [details] Patch Attachment 311594 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3848748 New failing tests: fast/shadow-dom/shadow-host-transition.html imported/blink/fast/css-generated-content/summary-before-after-content.html fast/shadow-dom/css-scoping-slot-with-id.html fast/shadow-dom/shadow-host-animation.html fast/shadow-dom/input-element-in-shadow.html fast/shadow-dom/shadow-layout-after-slot-fallback-changes.html fast/shadow-dom/css-scoping-host-and-slotted-context-invalidation.html fast/html/details-add-summary-child-1.html fast/shadow-dom/css-scoping-slotted-invalidation.html fast/html/details-replace-summary-child.html Created attachment 311604 [details]
Archive of layout-test-results from ews113 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 311594 [details] Patch Attachment 311594 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3848949 New failing tests: fast/shadow-dom/shadow-host-transition.html imported/blink/fast/css-generated-content/summary-before-after-content.html fast/shadow-dom/css-scoping-slot-with-id.html fast/shadow-dom/shadow-host-animation.html fast/shadow-dom/shadow-layout-after-slot-fallback-changes.html fast/shadow-dom/css-scoping-host-and-slotted-context-invalidation.html fast/html/details-add-summary-child-1.html fast/shadow-dom/css-scoping-slotted-invalidation.html fast/html/details-replace-summary-child.html Created attachment 311610 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
So if I apply this patch on top, which is effectively going back to the current state of affairs, all those tests pass... so I think this patch is correct, but is uncovering a few style invalidation bugs in presence of slots... I'll try to investigate them.
diff --git a/Source/WebCore/style/StyleTreeResolver.cpp b/Source/WebCore/style/StyleTreeResolver.cpp
index 6fe220d9b72..4849defe4e6 100644
--- a/Source/WebCore/style/StyleTreeResolver.cpp
+++ b/Source/WebCore/style/StyleTreeResolver.cpp
@@ -250,6 +250,7 @@ ElementUpdate TreeResolver::createAnimatedElementUpdate(std::unique_ptr<RenderSt
auto change = Detach;
if (auto* oldStyle = element.existingComputedStyle())
change = determineChange(*oldStyle, *newStyle);
+ change = std::max(change, Inherit);
return makeUpdate(WTFMove(newStyle), change);
}
@@ -335,6 +336,8 @@ static bool shouldResolveElement(const Element& element, Style::Change parentCha
if (existingStyle && existingStyle->hasExplicitlyInheritedProperties())
return true;
}
+ if (element.hasDisplayContents())
+ return true;
if (element.needsStyleRecalc())
return true;
if (shouldResolvePseudoElement(element.beforePseudoElement()))
Created attachment 312925 [details]
Patch
Sorry for the lag, I got sidetracked. I finally got this figured out, please take a look at the updated patch when you have the time, thanks! :) Comment on attachment 312925 [details]
Patch
Looks green.
Comment on attachment 312925 [details]
Patch
Looks good, r=me
Comment on attachment 312925 [details] Patch Clearing flags on attachment: 312925 Committed r218318: <http://trac.webkit.org/changeset/218318> All reviewed patches have been landed. Closing bug. We have a 11% PLT regression and this is the most likely culprit in the range. Reverted r218318 for reason: Seems to have caused an 11% PLT regression. Rolling out to confirm. Committed r218345: <http://trac.webkit.org/changeset/218345> (In reply to Chris Dumez from comment #18) > Reverted r218318 for reason: > > Seems to have caused an 11% PLT regression. Rolling out to confirm. > > Committed r218345: <http://trac.webkit.org/changeset/218345> I speculatively rolled out the patch to confirm. I'll re-land if it turns out it does not fix the regression (given there are multiple commits in the regression range). (In reply to Chris Dumez from comment #19) > (In reply to Chris Dumez from comment #18) > > Reverted r218318 for reason: > > > > Seems to have caused an 11% PLT regression. Rolling out to confirm. > > > > Committed r218345: <http://trac.webkit.org/changeset/218345> > > I speculatively rolled out the patch to confirm. I'll re-land if it turns > out it does not fix the regression (given there are multiple commits in the > regression range). I'm really not sure how could it be the case... This replaces an inline function in some relatively hot sites by an out-of-line one, but apart from that it's not doing anything more inefficiently in any particular way for it to justify an 11% (!) regression on page load time. I guess I can try to micro-optimize it inlining the function and saving a call here and there to hasDisplayContents or what not... Chris, was this patch definitely the culprit of the regression? (In reply to Emilio Cobos Álvarez from comment #20) > (In reply to Chris Dumez from comment #19) > > (In reply to Chris Dumez from comment #18) > > > Reverted r218318 for reason: > > > > > > Seems to have caused an 11% PLT regression. Rolling out to confirm. > > > > > > Committed r218345: <http://trac.webkit.org/changeset/218345> > > > > I speculatively rolled out the patch to confirm. I'll re-land if it turns > > out it does not fix the regression (given there are multiple commits in the > > regression range). > > I'm really not sure how could it be the case... This replaces an inline > function in some relatively hot sites by an out-of-line one, but apart from > that it's not doing anything more inefficiently in any particular way for it > to justify an 11% (!) regression on page load time. > > I guess I can try to micro-optimize it inlining the function and saving a > call here and there to hasDisplayContents or what not... > > Chris, was this patch definitely the culprit of the regression? Yes, the roll out was successful so I am fairly confident it was this patch. Note that 11% on page load time is unlikely to be due to lack of inlining. Something else must be going on. Antti may have an idea since he knows both this code and PLT well. There are no display:contents in PLT so the regression is from something that is affecting the normal case. Haven't had chance to look further yet. However my guess is that this has something to do with re-resolving display:none subtrees with cached computed style unnecessarily. (In reply to Antti Koivisto from comment #22) > There are no display:contents in PLT so the regression is from something > that is affecting the normal case. Haven't had chance to look further yet. > However my guess is that this has something to do with re-resolving > display:none subtrees with cached computed style unnecessarily. Yes, that sounds likely. I really assumed that those are very uncommon, but perhaps I'm wrong. I guess I can take some numbers and come back with an updated patch if they're not as uncommon as I thought. Is there a way I can run the PLT tests locally? I guess I can test in a couple of common pages manually, but it'd be nice to ensure it doesn't regress anything. (In reply to Emilio Cobos Álvarez from comment #23) > (In reply to Antti Koivisto from comment #22) > > There are no display:contents in PLT so the regression is from something > > that is affecting the normal case. Haven't had chance to look further yet. > > However my guess is that this has something to do with re-resolving > > display:none subtrees with cached computed style unnecessarily. > > Yes, that sounds likely. I really assumed that those are very uncommon, but > perhaps I'm wrong. I guess I can take some numbers and come back with an > updated patch if they're not as uncommon as I thought. Hmm... So looking a bit more closely at the implications of the patch, when someone calls getComputedStyle on a display: none element, we won't arrive to the resetStyleForNonRenderedDescendants path, and depending on how deep that path is, it could be a long unnecessary walk down the tree... I'll submit a patch that only checks for display: contents styles. It should fix the PLT regression, but I don't know how to check locally... Also, is there a way I can write a test for this? Do we have any API to get the amount of styled elements? I don't see anything off-hand. If there's interest I could write it and add a test for this. Created attachment 313198 [details]
Proposed patch
Comment on attachment 313198 [details]
Proposed patch
Looks green, as expected.
As I said before I'd expect this to fix the PLT regression, so marking cq?. Let me know if I can test it locally somehow. I'll file a bug to test number of restyles programmatically and add a test for this if you're fine with that.
> Also, is there a way I can write a test for this? Do we have any API to get
> the amount of styled elements? I don't see anything off-hand. If there's
> interest I could write it and add a test for this.
Yes, there is window.internals.lastStyleUpdateSize.
Comment on attachment 313198 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=313198&action=review r=me but a test against the regressed case would be cool. You can use internals.lastStyleUpdateSize or add a new testing interface if that is not good for this. > Source/WebCore/style/StyleTreeResolver.cpp:175 > + if (auto* cachedStyle = element.existingComputedStyle()) > + return cachedStyle->display() == CONTENTS ? cachedStyle : nullptr; You could make this read bit better as if (element.hasDisplayContents()) return element.existingComputedStyle(); Created attachment 314593 [details]
Patch
Gah, of course forgot about this one again. Sorry for not adding a test, but I found it hard to add one... I'm unsure the cause for the perf regression is restyling too many undisplayed elements, if only because when we do restyle them, we override the `style` pointer with the ElementUpdate's style, which makes it null again and makes us go to the proper path. I tried to add a nodesTraversed counter, but that not only seemed somewhat overkill, but also insufficient, since I had no way to force us traverse into an undisplayed subtree... However, I still think it's worth to just check for display: contents elements, if only to avoid the dangling pointer inside the if (shouldResolve) block, after clearing the display: none style. Comment on attachment 314593 [details]
Patch
Ok, r=me
Comment on attachment 314593 [details] Patch Clearing flags on attachment: 314593 Committed r220202: <http://trac.webkit.org/changeset/220202> All reviewed patches have been landed. Closing bug. |