Move childrenAffectedBy bits from RenderStyle to Element

• Component: CSS
• Status: RESOLVED FIXED

[Allan Sandfeld Jensen]

RenderStyle currently contains a number of observed dependencies used for dynamic restyle. Many of these are recorded when children of the element are being styled and not when the element who owns the style is being styled. This complicates the code in a number of places since this changes the style after styling is considered done. It also leads to subtle bugs as seen in bug #98021, and similar bugs must be assumed to exists for accidently sharing other dynamic set flags (though those bugs only leeds to unnecessary restyles).

Since these dependencies do not belong with the style, they should instead be moved to Node or Element. To begin with I suggest moving them to RareData, if they turn out to be too common, we can try to fit some of them into the Node bitflags.

[Allan Sandfeld Jensen]

Created attachment 172759 [details]
Patch

[WebKit Review Bot]

Attachment 172759 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/rendering/style/RenderStyle.h:247:  _explicitInheritance is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/rendering/style/RenderStyle.h:251:  _page_break_before is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/rendering/style/RenderStyle.h:257:  _unique is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/rendering/style/RenderStyle.h:258:  _emptyState is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/rendering/style/RenderStyle.h:259:  _firstChildState is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/rendering/style/RenderStyle.h:260:  _lastChildState is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 6 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 172759 [details]
Patch

Attachment 172759 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14764122

[Early Warning System Bot]

Comment on attachment 172759 [details]
Patch

Attachment 172759 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14758310

[EFL EWS Bot]

Comment on attachment 172759 [details]
Patch

Attachment 172759 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14745658

[Peter Beverloo (cr-android ews)]

Comment on attachment 172759 [details]
Patch

Attachment 172759 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14763174

[Allan Sandfeld Jensen]

Created attachment 172776 [details]
Patch

Sorry for making RenderStyle smaller

[WebKit Review Bot]

Attachment 172776 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/rendering/style/RenderStyle.h:247:  _explicitInheritance is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/rendering/style/RenderStyle.h:251:  _page_break_before is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/rendering/style/RenderStyle.h:257:  _unique is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/rendering/style/RenderStyle.h:258:  _emptyState is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/rendering/style/RenderStyle.h:259:  _firstChildState is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/rendering/style/RenderStyle.h:260:  _lastChildState is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 6 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 172776 [details]
Patch

Attachment 172776 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14757390

New failing tests:
fast/css-generated-content/drag-state.html
mathml/presentation/roots.xhtml
mathml/presentation/under.xhtml
mathml/presentation/underover.xhtml

[Allan Sandfeld Jensen]

Comment on attachment 172776 [details]
Patch

Four tests fail consistently on both mac on chrome. The three mathml tests doesn't run on Qt (no mathml), and the last test passes on Qt. Where can I see the detailed results from the EWS bots these days?

[WebKit Review Bot]

Comment on attachment 172776 [details]
Patch

Attachment 172776 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14760507

New failing tests:
touchadjustment/touch-links-active.html
fast/css-generated-content/drag-state.html
mathml/presentation/roots.xhtml
mathml/presentation/under.xhtml
touchadjustment/event-triggered-widgets.html
mathml/presentation/underover.xhtml

[Allan Sandfeld Jensen]

Created attachment 175980 [details]
Patch

Rebased and fixed regressions.

[WebKit Review Bot]

Attachment 175980 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/rendering/style/RenderStyle.h:249:  _explicitInheritance is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/rendering/style/RenderStyle.h:253:  _page_break_before is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/rendering/style/RenderStyle.h:259:  _affectedByUncommonAttributeSelectors is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/rendering/style/RenderStyle.h:260:  _unique is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/rendering/style/RenderStyle.h:261:  _emptyState is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/rendering/style/RenderStyle.h:262:  _firstChildState is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/rendering/style/RenderStyle.h:263:  _lastChildState is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 7 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 175980 [details]
Patch

Attachment 175980 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14987656

New failing tests:
touchadjustment/touch-links-active.html
touchadjustment/event-triggered-widgets.html
fast/text/atsui-small-caps-punctuation-size.html

[WebKit Review Bot]

Comment on attachment 175980 [details]
Patch

Attachment 175980 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14983684

New failing tests:
touchadjustment/touch-links-active.html
touchadjustment/event-triggered-widgets.html
fast/text/atsui-small-caps-punctuation-size.html

[Antti Koivisto]

This looks great. Only concern I have is the potential explosion in number of ElementRareDatas if these selectors are used heavily. Do you think that might be an issue?

[Allan Sandfeld Jensen]

(In reply to comment #16)
> This looks great. Only concern I have is the potential explosion in number of ElementRareDatas if these selectors are used heavily. Do you think that might be an issue?

It is a possible. The patch needs to be checked for memory-usage regression before landing. If some of the flags turns out to be too common (perhaps affectedByHover), the flag can be moved to Node.

[Ojan Vafai]

Patch looks like a big improvement to me. Just as a sanity check, could you instrument your local build to count the extra number of RareDatas we make on popular sites (e.g. gmail, facebook, twitter)?

Also, could you add COMPILE_ASSERTS for the sizes of RenderStyle and NodeRareData? Ideally you would do this in a separate patch that we land first. That way, it's easier to see the effect this patch has on the sizes of these classes. http://trac.webkit.org/browser/trunk/Source/WebCore/css/RuleSet.h#L99 is an example of what I'm asking for.

[Antti Koivisto]

The bug could also use a better title.

[Allan Sandfeld Jensen]

(In reply to comment #15)
> (From update of attachment 175980 [details])
> Attachment 175980 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/14983684
> 
> New failing tests:
> touchadjustment/touch-links-active.html
> touchadjustment/event-triggered-widgets.html
> fast/text/atsui-small-caps-punctuation-size.html

touchadjustment/event-triggered-widgets.html fails due to an error in the test which is exposed by this patch. The last of the test cases shares style with the earlier test-cases, and therefore also used to share their dependencies, which made touch-adjustment think it was affected by hover/active. 

I think fast/text/atsui-small-caps-punctuation-size.html is currently flaky? 

Still trying to figure out what is going on with touchadjustment/touch-links-active.html.

(In reply to comment #19)
> The bug could also use a better title.

I am open to suggestions. I am also incorporating the test-case from bug #98021 since it also solves that bug, but that bug tittle might be too specific.

Uncouple style-dependencies and style?

[Antti Koivisto]

"Style dependencies" is very generic. "Move affectedBy bits from RenderStyle to Element" or something.

[Allan Sandfeld Jensen]

Created attachment 176254 [details]
Patch

Copy local affectedBy bits when style sharing. Perhaps later local and parent affectedBy bits should be split

[WebKit Review Bot]

Attachment 176254 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/rendering/style/RenderStyle.h:249:  _explicitInheritance is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/rendering/style/RenderStyle.h:253:  _page_break_before is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/rendering/style/RenderStyle.h:259:  _affectedByUncommonAttributeSelectors is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/rendering/style/RenderStyle.h:260:  _unique is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/rendering/style/RenderStyle.h:261:  _emptyState is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/rendering/style/RenderStyle.h:262:  _firstChildState is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/rendering/style/RenderStyle.h:263:  _lastChildState is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 7 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Allan Sandfeld Jensen]

Created attachment 176265 [details]
Patch

Split affectedBy Hover/Active/Drag into childrenAffectedBy and Style::affectedBy. Further cleanup.

[Allan Sandfeld Jensen]

(In reply to comment #16)
> This looks great. Only concern I have is the potential explosion in number of ElementRareDatas if these selectors are used heavily. Do you think that might be an issue?

With the local affectedBy kept in RenderStyle in the latest version, it is certainly not a problem. I get roughly the same amount of rareData objects created on facebook with an without the patch.

[Elliott Sprehn]

Comment on attachment 176265 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176265&action=review

This patch hides lots of duplicate hasRareData() checks all over the code. Could we return a struct of flags instead?

> Source/WebCore/dom/Element.cpp:1316
> +    bool hasIndirectAdjacentRules = childrenAffectedByForwardPositionalRules();

Again this checks for hasRareData() repeatedly.

> Source/WebCore/page/TouchAdjustment.cpp:79
> +    if (node->childrenAffectedByActive() || node->childrenAffectedByHover())

This means you check for the rare data twice now inside those methods. Can we instead use a struct that contains these bitfields and you just return the struct? I don't like that this means hidden hasRareData() checks all over.

const AffectedByFlags& Node::affectedByFlags() const { return hasRareData() ? rareData()->styleFlags() : staticDefaultStyleFlags; }

[Antti Koivisto]

(In reply to comment #26)
> This means you check for the rare data twice now inside those methods. Can we instead use a struct that contains these bitfields and you just return the struct? I don't like that this means hidden hasRareData() checks all over.

hasRareData() is a bit test. There is no point in doing anything complicated to avoid it. Inlining the getters in Element might be a good move though.

[Antti Koivisto]

Comment on attachment 176265 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176265&action=review

r-, all accessors should move up to Element. Looks good otherwise.

> Source/WebCore/ChangeLog:5
> +        https://bugs.webkit.org/show_bug.cgi?id=101448
> +        https://bugs.webkit.org/show_bug.cgi?id=98021

You should probably dupe one of these bugs to another.

> Source/WebCore/dom/Element.cpp:1963
> +bool Element::childrenAffectedByFirstChildRules() const
> +{
> +    return hasRareData() && elementRareData()->childrenAffectedByFirstChildRules();
> +}

Might make sense to inline the getters though it is ok to do it if this actually shows up in profiles.

> Source/WebCore/dom/Element.cpp:1968
> +void Element::setChildrenAffectedByFirstChildRules()
> +{
> +    ensureElementRareData()->setChildrenAffectedByFirstChildRules(true);
> +}

I'm still slightly worried that a bad rule can make large number of Elements have RareData. I guess we'll see if this is a real problem.

We have some room in the Node bitfield and more could be made. Perhaps the most popular bits could go there instead of RareData.

> Source/WebCore/dom/Node.h:556
> +    bool childrenAffectedByHover() const;
> +    void setChildrenAffectedByHover(bool);
> +    bool childrenAffectedByActive() const;
> +    void setChildrenAffectedByActive(bool);
> +    bool childrenAffectedByDrag() const;
> +    void setChildrenAffectedByDrag(bool);

I don't think these bits can be set for non-elements either. The accessors should be in Element and the clients should cast.

[Allan Sandfeld Jensen]

*** Bug 98021 has been marked as a duplicate of this bug. ***

[Elliott Sprehn]

(In reply to comment #27)
> (In reply to comment #26)
> > This means you check for the rare data twice now inside those methods. Can we instead use a struct that contains these bitfields and you just return the struct? I don't like that this means hidden hasRareData() checks all over.
> 
> hasRareData() is a bit test. There is no point in doing anything complicated to avoid it. Inlining the getters in Element might be a good move though.

Do we know adding this really isn't slower? When I changed ::renderer() to have a hasRareData check in it there was a 1-2% regression in html5-full-render.html just from accessing renderer() repeatedly.

[Allan Sandfeld Jensen]

(In reply to comment #30)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > This means you check for the rare data twice now inside those methods. Can we instead use a struct that contains these bitfields and you just return the struct? I don't like that this means hidden hasRareData() checks all over.
> > 
> > hasRareData() is a bit test. There is no point in doing anything complicated to avoid it. Inlining the getters in Element might be a good move though.
> 
> Do we know adding this really isn't slower? When I changed ::renderer() to have a hasRareData check in it there was a 1-2% regression in html5-full-render.html just from accessing renderer() repeatedly.

The overhead of an uninlined function-call is an order of magnitude larger than a bit-check, but moving the definitions to the header files, the calls should be much better optimized, plus a good compiler should be able to optimize the bit-check as well.

The only call that would be called relatively often though is probably the hover-check which is called for every mouse-move.

[Allan Sandfeld Jensen]

Created attachment 176447 [details]
Patch

Move all the bits to Element, and partially inline accessors

[Antti Koivisto]

Comment on attachment 176447 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176447&action=review

r=me

> Source/WebCore/page/TouchAdjustment.cpp:85
> +        if (renderStyle->affectedByActive() || renderStyle->affectedByHover())

This could move to the Node bitfield in the future.

[Allan Sandfeld Jensen]

(In reply to comment #33)
> (From update of attachment 176447 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176447&action=review
> 
> r=me
> 
> > Source/WebCore/page/TouchAdjustment.cpp:85
> > +        if (renderStyle->affectedByActive() || renderStyle->affectedByHover())
> 
> This could move to the Node bitfield in the future.

Just note that if you do that (like the earlier patches did), that you will need to copy the flags when elements share styles.

[WebKit Review Bot]

Comment on attachment 176447 [details]
Patch

Clearing flags on attachment: 176447

Committed r136001: <http://trac.webkit.org/changeset/136001>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.