Summary: | Enable style sharing for elements with a style attribute | ||
---|---|---|---|
Product: | WebKit | Reporter: | Julien Chaffraix <jchaffraix> |
Component: | CSS | Assignee: | Julien Chaffraix <jchaffraix> |
Status: | REOPENED --- | ||
Severity: | Normal | CC: | dglazkov, hyatt, kling, koivisto, macpherson, menard, webkit.review.bot |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | All | ||
OS: | All | ||
Bug Depends on: | 82060 | ||
Bug Blocks: | |||
Attachments: |
Description
Julien Chaffraix
2012-03-19 10:19:42 PDT
Created attachment 132606 [details]
Proposed change.
Comment on attachment 132606 [details] Proposed change. Attachment 132606 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11984592 New failing tests: compositing/checkerboard.html compositing/geometry/limit-layer-bounds-clipping-ancestor.html css1/text_properties/vertical_align.html compositing/iframes/iframe-in-composited-layer.html compositing/layer-creation/fixed-position-out-of-view.html css3/unicode-bidi-isolate-basic.html compositing/direct-image-compositing.html compositing/geometry/empty-embed-rects.html http/tests/inspector/inspect-element.html compositing/repaint/overflow-into-content.html css1/box_properties/float_elements_in_series.html accessibility/aria-disabled.html css1/units/color_units.html css1/box_properties/clear_float.html compositing/overflow/ancestor-overflow.html css3/bdi-element.html compositing/layer-creation/overflow-scroll-overlap.html compositing/repaint/content-into-overflow.html css1/text_properties/text_decoration.html compositing/iframes/iframe-content-flipping.html css3/zoom-coords.xhtml compositing/overflow/repaint-after-losing-scrollbars.html compositing/layer-creation/rotate3d-overlap.html compositing/overflow/overflow-scroll.html compositing/iframes/composited-iframe-scroll.html http/tests/inspector/compiler-source-mapping.html compositing/overflow/clip-descendents.html accessibility/dimensions-include-descendants.html css1/formatting_model/floating_elements.html css2.1/20110323/abspos-containing-block-initial-001.htm Created attachment 132609 [details]
Archive of layout-test-results from ec2-cr-linux-04
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 132606 [details]
Proposed change.
/me needs to investigate the failures.
Off the cuff it looks like you never compare the style attributes, just check that they are both present (or not present) on both elements. Comment on attachment 132606 [details] Proposed change. View in context: https://bugs.webkit.org/attachment.cgi?id=132606&action=review Re-testing the good patch to see if the results are still matching. > Source/WebCore/css/CSSStyleSelector.cpp:1385 > + if (!!element->getAttribute(styleAttr) != !!m_styledElement->getAttribute(styleAttr)) > + return false; > + Spot on Andreas, this should be: if (element->inlineStyle() && element->getAttribute(styleAttr) != m_styledElement->getAttribute(styleAttr)) return false; Bad last minute copy and paste here :( Created attachment 132869 [details]
Fixed bad copy 'n' paste, redid some benchmarkings.
Comment on attachment 132869 [details] Fixed bad copy 'n' paste, redid some benchmarkings. View in context: https://bugs.webkit.org/attachment.cgi?id=132869&action=review > Source/WebCore/css/CSSStyleSelector.cpp:1382 > + if (element->inlineStyle() && element->getAttribute(styleAttr) != m_styledElement->getAttribute(styleAttr)) > + return false; This means we'll end up serializing the elements' inline styles every time we get here. (StyledElement::updateStyleAttribute) It might be more efficient to simply compare the StylePropertySets for strict equality. Comment on attachment 132869 [details] Fixed bad copy 'n' paste, redid some benchmarkings. View in context: https://bugs.webkit.org/attachment.cgi?id=132869&action=review >> Source/WebCore/css/CSSStyleSelector.cpp:1382 >> + return false; > > This means we'll end up serializing the elements' inline styles every time we get here. (StyledElement::updateStyleAttribute) > It might be more efficient to simply compare the StylePropertySets for strict equality. Yes, I haven't measured a definitive performance hit due to the serialization though. I will try that and see if it makes sense. Created attachment 132947 [details]
Propsed refactoring 3: did not see any performance with this approach but it is cleaner.
Comment on attachment 132947 [details] Propsed refactoring 3: did not see any performance with this approach but it is cleaner. View in context: https://bugs.webkit.org/attachment.cgi?id=132947&action=review > Source/WebCore/css/CSSStyleSelector.cpp:1253 > +enum StyleComparison { > + AttributeStyle, > + InlineStyle > +}; > + > +template<StyleComparison> static bool propertiesEqual(const CSSProperty&, const CSSProperty&); I don't understand the need for these templates. It seems to me that regular functions would work equally well and be less confusing. Comment on attachment 132947 [details] Propsed refactoring 3: did not see any performance with this approach but it is cleaner. View in context: https://bugs.webkit.org/attachment.cgi?id=132947&action=review > Source/WebCore/css/CSSStyleSelector.cpp:1261 > + if (aProperty.isImportant() != bProperty.isImportant() > + || aProperty.isInherited() != bProperty.isInherited() || aProperty.isImplicit() != bProperty.isImplicit()) > + return false; I think you should do these comparisons in all cases. Then you can eliminate the confusing templates. Cost of unnecessary boolean comparisons in attribute style case is likely to be meaningless. Created attachment 133299 [details]
Simplified the logic per antti's discussion.
Comment on attachment 133299 [details]
Simplified the logic per antti's discussion.
r=me
Comment on attachment 133299 [details] Simplified the logic per antti's discussion. Clearing flags on attachment: 133299 Committed r111751: <http://trac.webkit.org/changeset/111751> All reviewed patches have been landed. Closing bug. It looks like DHTML page cycler regressed on linux following this change: http://build.chromium.org/f/chromium/perf/linux-release/dhtml/report.html?history=150&rev=128488 Reopening to investigate why this happened. (In reply to comment #17) > It looks like DHTML page cycler regressed on linux following this change: http://build.chromium.org/f/chromium/perf/linux-release/dhtml/report.html?history=150&rev=128488 > > Reopening to investigate why this happened. DHTML pages are more or less all based on using tons inline style. That explains why the benchmark was impacted so much by this change: it represents the worst case scenario (a lot of different inline styles). (In reply to comment #18) > (In reply to comment #17) > > It looks like DHTML page cycler regressed on linux following this change: http://build.chromium.org/f/chromium/perf/linux-release/dhtml/report.html?history=150&rev=128488 > > > > Reopening to investigate why this happened. > > DHTML pages are more or less all based on using tons inline style. That explains why the benchmark was impacted so much by this change: it represents the worst case scenario (a lot of different inline styles). Yep, the issue is that we don't bail out in locateSharedStyle now. This makes the whole style sharing logic hotter but doesn't increase the sharing on those pages. This change helps our sharing in some context but it seems to add a lot of overhead in the context of the DHTML page cycler. I feel like solving bug 81865 would help. |