Bug 101448 - Move childrenAffectedBy bits from RenderStyle to Element
Summary: Move childrenAffectedBy bits from RenderStyle to Element
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 420+
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
: 98021 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-11-07 04:50 PST by Allan Sandfeld Jensen
Modified: 2012-11-28 05:10 PST (History)
13 users (show)

See Also:


Attachments
Patch (60.87 KB, patch)
2012-11-07 04:56 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (61.18 KB, patch)
2012-11-07 05:47 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (60.21 KB, patch)
2012-11-26 05:51 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (64.03 KB, patch)
2012-11-27 06:18 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (64.47 KB, patch)
2012-11-27 07:34 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (66.18 KB, patch)
2012-11-28 03:42 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Allan Sandfeld Jensen 2012-11-07 04:50:05 PST
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.
Comment 1 Allan Sandfeld Jensen 2012-11-07 04:56:37 PST
Created attachment 172759 [details]
Patch
Comment 2 WebKit Review Bot 2012-11-07 04:59:49 PST
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.
Comment 3 Early Warning System Bot 2012-11-07 05:05:21 PST
Comment on attachment 172759 [details]
Patch

Attachment 172759 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14764122
Comment 4 Early Warning System Bot 2012-11-07 05:06:11 PST
Comment on attachment 172759 [details]
Patch

Attachment 172759 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14758310
Comment 5 EFL EWS Bot 2012-11-07 05:28:23 PST
Comment on attachment 172759 [details]
Patch

Attachment 172759 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14745658
Comment 6 Peter Beverloo (cr-android ews) 2012-11-07 05:47:28 PST
Comment on attachment 172759 [details]
Patch

Attachment 172759 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14763174
Comment 7 Allan Sandfeld Jensen 2012-11-07 05:47:49 PST
Created attachment 172776 [details]
Patch

Sorry for making RenderStyle smaller
Comment 8 WebKit Review Bot 2012-11-07 05:51:42 PST
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.
Comment 9 Build Bot 2012-11-07 07:40:13 PST
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
Comment 10 Allan Sandfeld Jensen 2012-11-07 11:34:16 PST
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?
Comment 11 WebKit Review Bot 2012-11-07 16:00:56 PST
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
Comment 12 Allan Sandfeld Jensen 2012-11-26 05:51:38 PST
Created attachment 175980 [details]
Patch

Rebased and fixed regressions.
Comment 13 WebKit Review Bot 2012-11-26 05:57:06 PST
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.
Comment 14 WebKit Review Bot 2012-11-26 07:19:28 PST
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
Comment 15 WebKit Review Bot 2012-11-26 07:55:05 PST
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
Comment 16 Antti Koivisto 2012-11-26 09:06:08 PST
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?
Comment 17 Allan Sandfeld Jensen 2012-11-26 09:46:57 PST
(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.
Comment 18 Ojan Vafai 2012-11-26 10:00:17 PST
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.
Comment 19 Antti Koivisto 2012-11-26 10:46:35 PST
The bug could also use a better title.
Comment 20 Allan Sandfeld Jensen 2012-11-26 12:12:36 PST
(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?
Comment 21 Antti Koivisto 2012-11-26 12:21:28 PST
"Style dependencies" is very generic. "Move affectedBy bits from RenderStyle to Element" or something.
Comment 22 Allan Sandfeld Jensen 2012-11-27 06:18:38 PST
Created attachment 176254 [details]
Patch

Copy local affectedBy bits when style sharing. Perhaps later local and parent affectedBy bits should be split
Comment 23 WebKit Review Bot 2012-11-27 06:20:58 PST
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.
Comment 24 Allan Sandfeld Jensen 2012-11-27 07:34:09 PST
Created attachment 176265 [details]
Patch

Split affectedBy Hover/Active/Drag into childrenAffectedBy and Style::affectedBy. Further cleanup.
Comment 25 Allan Sandfeld Jensen 2012-11-27 08:12:43 PST
(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.
Comment 26 Elliott Sprehn 2012-11-27 08:20:12 PST
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; }
Comment 27 Antti Koivisto 2012-11-27 08:44:58 PST
(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.
Comment 28 Antti Koivisto 2012-11-27 11:59:56 PST
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.
Comment 29 Allan Sandfeld Jensen 2012-11-27 12:02:26 PST
*** Bug 98021 has been marked as a duplicate of this bug. ***
Comment 30 Elliott Sprehn 2012-11-27 12:26:07 PST
(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.
Comment 31 Allan Sandfeld Jensen 2012-11-28 01:46:50 PST
(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.
Comment 32 Allan Sandfeld Jensen 2012-11-28 03:42:41 PST
Created attachment 176447 [details]
Patch

Move all the bits to Element, and partially inline accessors
Comment 33 Antti Koivisto 2012-11-28 04:04:20 PST
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.
Comment 34 Allan Sandfeld Jensen 2012-11-28 04:42:29 PST
(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.
Comment 35 WebKit Review Bot 2012-11-28 05:10:30 PST
Comment on attachment 176447 [details]
Patch

Clearing flags on attachment: 176447

Committed r136001: <http://trac.webkit.org/changeset/136001>
Comment 36 WebKit Review Bot 2012-11-28 05:10:37 PST
All reviewed patches have been landed.  Closing bug.