Summary: | Enable matched declaration caching for elements with a style attribute | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Julien Chaffraix <jchaffraix> | ||||||||||
Component: | CSS | Assignee: | Julien Chaffraix <jchaffraix> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | dglazkov, kling, koivisto, macpherson, menard, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 78070 | ||||||||||||
Attachments: |
|
Description
Julien Chaffraix
2012-03-05 19:53:50 PST
Created attachment 130273 [details]
Early prototype: add an inline version to StylePropertySet, include this information to MatchedProperties
FYI this patch has its own issues that needs to be ironed out (like prevent people from abusing the logic for non-inline style StylePropertySet and causing perf regressions). I think however that the logic handles all cases of invalidation (JavaScript vs DOM). An earlier version shaved ~1% on Moz page cycler.
Comments welcome on the approach taken!
Created attachment 130395 [details]
another possible patch
This takes somewhat different approach by making inline style property sets immutable as long as there is no CSSOM wrapper (no one accesses element.style). Immutability means they are inherently cacheable.
Attachment 130395 [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/dom/ElementAttributeData.h:96: The parameter name "element" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 1 in 18 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 130395 [details] another possible patch Attachment 130395 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11834477 Comment on attachment 130395 [details] another possible patch View in context: https://bugs.webkit.org/attachment.cgi?id=130395&action=review > Source/WebCore/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=80370 > + Re-enable matched declaration caching for elements with a style attribute The order is wrong here... > Source/WebCore/dom/StyledElement.cpp:108 > + // Keep inline style property sets without CSSOM wrappers immutable. This way they can be cached. I'm confused by this comment. This isn't really a place where we make the property set immutable, or is it? Comment on attachment 130395 [details] another possible patch Attachment 130395 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11837531 New failing tests: editing/undo/remove-css-property-and-remove-style.html fast/css/rem-dynamic-scaling.html editing/execCommand/toggle-compound-styles.html editing/execCommand/5770834-1.html editing/style/push-down-font-styles.html editing/style/push-down-implicit-styles.html editing/execCommand/remove-format-multiple-elements.html editing/style/push-down-inline-styles.html fast/multicol/vertical-rl/break-properties.html editing/style/push-down-implicit-styles-around-list.html editing/style/inline-style-container.html editing/execCommand/empty-span-removal.html Comment on attachment 130395 [details] another possible patch View in context: https://bugs.webkit.org/attachment.cgi?id=130395&action=review A couple more comments but I agree that this approach is a more light-weight alternative to what I had in mind (and catch a few bugs at compile time). > Source/WebCore/dom/StyledElement.cpp:129 > + ensureAttributeData()->ensureMutableInlineStyle(this)->setProperty(propertyID, document()->cssValuePool()->createIdentifierValue(identifier), important); It really looks like ensureAttributeData()->ensureMutableInlineStyle(this) should have its own method. I wonder if the naming couldn't be improved to underline that we may copy our style as 'mutable' is fairly generic IMHO. I couldn't find a better naming unfortunately. > Source/WebCore/dom/StyledElement.h:43 > + const StylePropertySet* inlineStyle() const { return attributeData() ? attributeData()->inlineStyle() : 0; } > + const StylePropertySet* ensureInlineStyle() { return ensureAttributeData()->ensureInlineStyle(this); } Those renamings are making Qt sad: ../../../Source/WebKit/qt/Api/qwebelement.cpp: In member function 'QString QWebElement::styleProperty(const QString&, QWebElement::StyleResolveStrategy) const': ../../../Source/WebKit/qt/Api/qwebelement.cpp:847: error: 'class WebCore::StyledElement' has no member named 'ensureInlineStyleDecl' ../../../Source/WebKit/qt/Api/qwebelement.cpp: In member function 'void QWebElement::setStyleProperty(const QString&, const QString&)': ../../../Source/WebKit/qt/Api/qwebelement.cpp:912: error: 'class WebCore::StyledElement' has no member named 'ensureInlineStyleDecl' > Source/WebCore/editing/ApplyStyleCommand.cpp:512 > + RefPtr<StylePropertySet> inlineStyle = element-> ensureInlineStyle()->copy(); Extra space. > Source/WebCore/editing/ApplyStyleCommand.cpp:726 > + RefPtr<StylePropertySet> inlineStyle = element-> ensureInlineStyle()->copy(); Ditto. Comment on attachment 130395 [details] another possible patch Attachment 130395 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11834576 New failing tests: editing/undo/remove-css-property-and-remove-style.html fast/css/rem-dynamic-scaling.html editing/execCommand/toggle-compound-styles.html editing/execCommand/5770834-1.html editing/style/push-down-font-styles.html editing/style/push-down-implicit-styles.html editing/execCommand/remove-format-multiple-elements.html editing/style/push-down-inline-styles.html fast/multicol/vertical-rl/break-properties.html editing/style/push-down-implicit-styles-around-list.html editing/style/inline-style-container.html editing/execCommand/empty-span-removal.html Created attachment 130590 [details]
bug fixes, qt build fix
Comment on attachment 130590 [details] bug fixes, qt build fix Attachment 130590 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11834965 Comment on attachment 130590 [details] bug fixes, qt build fix Attachment 130590 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11849059 New failing tests: media/track/track-cue-rendering.html Created attachment 130609 [details]
another qt build fix attempt
Comment on attachment 130609 [details] another qt build fix attempt Attachment 130609 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11854049 New failing tests: media/track/track-cue-rendering.html Comment on attachment 130609 [details] another qt build fix attempt Attachment 130609 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11852093 New failing tests: media/track/track-cue-rendering.html Comment on attachment 130609 [details] another qt build fix attempt View in context: https://bugs.webkit.org/attachment.cgi?id=130609&action=review This looks pretty cool. r=me, assuming the EWS failure is a flake. > Source/WebCore/dom/ElementAttributeData.cpp:70 > + m_inlineStyleDecl = m_inlineStyleDecl->copy(); > + m_inlineStyleDecl->setStrictParsing(element->isHTMLElement() && !element->document()->inQuirksMode()); I feel like copy() should return a StylePropertySet with the same strictness setting as the original. http://trac.webkit.org/changeset/110188 (and http://trac.webkit.org/changeset/110190 to fix qt build) |