Bug 80370 - Enable matched declaration caching for elements with a style attribute
Summary: Enable matched declaration caching for elements with a style attribute
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks: 78070
  Show dependency treegraph
 
Reported: 2012-03-05 19:53 PST by Julien Chaffraix
Modified: 2012-03-08 12:18 PST (History)
6 users (show)

See Also:


Attachments
Early prototype: add an inline version to StylePropertySet, include this information to MatchedProperties (4.96 KB, patch)
2012-03-05 20:04 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
another possible patch (31.74 KB, patch)
2012-03-06 09:42 PST, Antti Koivisto
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
bug fixes, qt build fix (37.44 KB, patch)
2012-03-07 03:45 PST, Antti Koivisto
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
another qt build fix attempt (37.49 KB, patch)
2012-03-07 06:18 PST, Antti Koivisto
kling: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2012-03-05 19:53:50 PST
http://trac.webkit.org/changeset/106741 disabled this as there were some issues with properly invalidating our matched declaration cache entries.

Let's bring the cache back on!
Comment 1 Julien Chaffraix 2012-03-05 20:04:21 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!
Comment 2 Antti Koivisto 2012-03-06 09:42:21 PST
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.
Comment 3 WebKit Review Bot 2012-03-06 09:48:24 PST
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 4 Early Warning System Bot 2012-03-06 10:05:17 PST
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 5 Ryosuke Niwa 2012-03-06 11:12:41 PST
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 6 WebKit Review Bot 2012-03-06 12:36:25 PST
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 7 Julien Chaffraix 2012-03-06 13:14:04 PST
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 8 WebKit Review Bot 2012-03-06 13:20:36 PST
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
Comment 9 Antti Koivisto 2012-03-07 03:45:25 PST
Created attachment 130590 [details]
bug fixes, qt build fix
Comment 10 Early Warning System Bot 2012-03-07 04:07:37 PST
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 11 WebKit Review Bot 2012-03-07 06:03:43 PST
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
Comment 12 Antti Koivisto 2012-03-07 06:18:54 PST
Created attachment 130609 [details]
another qt build fix attempt
Comment 13 WebKit Review Bot 2012-03-07 07:25:55 PST
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 14 WebKit Review Bot 2012-03-07 08:15:02 PST
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 15 Andreas Kling 2012-03-07 10:52:22 PST
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.
Comment 16 Antti Koivisto 2012-03-08 12:18:16 PST
http://trac.webkit.org/changeset/110188

(and http://trac.webkit.org/changeset/110190 to fix qt build)