Bug 101448

Created attachment 172759 [details] Patch

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 on attachment 172759 [details] Patch Attachment 172759 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14764122

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

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

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

Created attachment 172776 [details] Patch Sorry for making RenderStyle smaller

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

Created attachment 175980 [details] Patch Rebased and fixed regressions.

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

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?

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

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.

The bug could also use a better title.

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

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

Created attachment 176254 [details] Patch Copy local affectedBy bits when style sharing. Perhaps later local and parent affectedBy bits should be split

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.

Created attachment 176265 [details] Patch Split affectedBy Hover/Active/Drag into childrenAffectedBy and Style::affectedBy. Further cleanup.

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

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

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

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

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

Created attachment 176447 [details] Patch Move all the bits to Element, and partially inline accessors

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.

(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 on attachment 176447 [details] Patch Clearing flags on attachment: 176447 Committed r136001: <http://trac.webkit.org/changeset/136001>

All reviewed patches have been landed. Closing bug.