Bug 172753 – Don't always do a style recalc for display: contents elements.

• Component: CSS
• Status: RESOLVED FIXED

[Emilio Cobos Álvarez]

Followup to bug 172721.

[Emilio Cobos Álvarez]

Created attachment 311594 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Emilio Cobos Álvarez]

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()))

[Emilio Cobos Álvarez]

Created attachment 312925 [details]
Patch

[Emilio Cobos Álvarez]

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! :)

[Emilio Cobos Álvarez]

Comment on attachment 312925 [details]
Patch

Looks green.

[Antti Koivisto]

Comment on attachment 312925 [details]
Patch

Looks good, r=me

[WebKit Commit Bot]

Comment on attachment 312925 [details]
Patch

Clearing flags on attachment: 312925

Committed r218318: <http://trac.webkit.org/changeset/218318>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

We have a 11% PLT regression and this is the most likely culprit in the range.

[Chris Dumez]

Reverted r218318 for reason:

Seems to have caused an 11% PLT regression. Rolling out to confirm.

Committed r218345: <http://trac.webkit.org/changeset/218345>

[Chris Dumez]

(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).

[Emilio Cobos Álvarez]

(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?

[Chris Dumez]

(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.

[Antti Koivisto]

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.

[Emilio Cobos Álvarez]

(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.

[Emilio Cobos Álvarez]

(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.

[Emilio Cobos Álvarez]

Created attachment 313198 [details]
Proposed patch

[Emilio Cobos Álvarez]

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.

[Antti Koivisto]

> 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.

[Antti Koivisto]

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();

[Emilio Cobos Álvarez]

Created attachment 314593 [details]
Patch

[Emilio Cobos Álvarez]

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.

[Antti Koivisto]

Comment on attachment 314593 [details]
Patch

Ok, r=me

[WebKit Commit Bot]

Comment on attachment 314593 [details]
Patch

Clearing flags on attachment: 314593

Committed r220202: <http://trac.webkit.org/changeset/220202>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33698758>