Bug 70931 – Matched declaration cache

• Component: CSS
• Status: RESOLVED FIXED

[Antti Koivisto]

Sets of style declarations are applied repeatedly for different elements when calculating the document style. The same set off applied declarations result in the same non-inherited style, independent of the element and its context. We can use this to build a cache to speed up style applying and to share more style data for reduced memory usage.

[Antti Koivisto]

Created attachment 112550 [details]
patch

[Gyuyoung Kim]

Comment on attachment 112550 [details]
patch

Attachment 112550 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10220257

[Darin Adler]

Comment on attachment 112550 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112550&action=review

> Source/WebCore/css/CSSProperty.cpp:384
> +    COMPILE_ASSERT(349 == numCSSProperties, _when_adding_new_properties_add_the_inherited_here_and_bump_the_property_count);

Looks like this assertion is failing on various platforms. Maybe the number of properties varies based on feature defines?

> Source/WebCore/css/CSSProperty.h:73
> +    signed m_id : 14;
> +    signed m_shorthandID : 14; // If this property was set as part of a shorthand, gives the shorthand.

Is there a good way to do a runtime check to assert we have enough bits here? Does this really need to be signed rather than unsigned?

> Source/WebCore/css/CSSStyleSelector.cpp:2153
> +    unsigned hash = 0;
> +    for (unsigned i = 0; i < size; ++i) {
> +        unsigned ptrHash = PtrHash<CSSMutableStyleDeclaration*>::hash(declarations[i].styleDeclaration);
> +        ptrHash ^= IntHash<unsigned>::hash(declarations[i].linkMatchType);
> +        // Make the position matter.
> +        hash ^= (ptrHash << i) | (ptrHash >> (32 - i));
> +    }
> +    return hash;

I think a better way to compute this hash is to just compute a hash across each bit of data using the algorithm like StringHasher::hashMemory. Combining multiple hashes into a new one is typically not as good as that function. I believe that would be both faster and a better algorithm than this. The function that is needed to make StringHasher usable for this would be StringHasher::addMemory that would let you add things other than characters to a hash. Should be really easy to write.

> Source/WebCore/css/CSSStyleSelector.cpp:2186
> +        if (!(m_matchedDecls[i] == cacheItem.matchedStyleDeclarations[i]))

I think this would be easier to read with (a != b) rather than (!(a == b)).

> Source/WebCore/css/CSSStyleSelector.cpp:2189
> +    if (!(cacheItem.matchResult == matchResult))

Ditto.

> Source/WebCore/css/CSSStyleSelector.cpp:2264
> +    if (m_style->unique() || m_style->hasAppearance() || (m_style->styleType() != NOPSEUDO && m_parentStyle->unique()) 
> +        || m_style->zoom() != RenderStyle::initialZoom())
> +        return;

You could break this up into two or more return statements to avoid having the awkwardly broken long line.

> Source/WebCore/css/CSSStyleSelector.h:230
> +        bool operator==(const MatchResult&) const;

Usually as a matter of default C++ style operator== should be a free function, not a member function. In cases where type conversions are possible the free function allows type conversions on the left side. But I guess that might be a little tricky to do inside a class definition.

We should add an operator != that just calls operator== and does the ! part.

> Source/WebCore/css/CSSStyleSelector.h:333
> +        bool operator==(const MatchedStyleDeclaration&) const;

Same as above.

[WebKit Review Bot]

Comment on attachment 112550 [details]
patch

Attachment 112550 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10220259

[WebKit Review Bot]

Attachment 112550 [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/css/CSSStyleSelector.h:338:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

Comment on attachment 112550 [details]
patch

Attachment 112550 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10214548

[Dave Hyatt]

Comment on attachment 112550 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112550&action=review

Looks good. One thing I think we should be doing is sampling the Web to try to find out how to break up our data structs these days. I think we may get a big win from having more smaller structs (especially with the gigantic RareXXX structs we've got going on now).

> Source/WebCore/css/CSSProperty.cpp:381
> +    default:

What about removing this default and instead listing all the other properties? Then you'll get a compile error that the case isn't handled by the switch for any new props added.

[Early Warning System Bot]

Comment on attachment 112550 [details]
patch

Attachment 112550 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10214564

[Antti Koivisto]

(In reply to comment #3)
> (From update of attachment 112550 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112550&action=review
> 
> > Source/WebCore/css/CSSProperty.cpp:384
> > +    COMPILE_ASSERT(349 == numCSSProperties, _when_adding_new_properties_add_the_inherited_here_and_bump_the_property_count);
> 
> Looks like this assertion is failing on various platforms. Maybe the number of properties varies based on feature defines?

As Hyatt suggested, I just added all properties to the switch instead (with some feature ifdefs).

> > Source/WebCore/css/CSSProperty.h:73
> > +    signed m_id : 14;
> > +    signed m_shorthandID : 14; // If this property was set as part of a shorthand, gives the shorthand.
> 
> Is there a good way to do a runtime check to assert we have enough bits here? Does this really need to be signed rather than unsigned?

I don't know any good way to do that. Switched to unsigned.

> I think a better way to compute this hash is to just compute a hash across each bit of data using the algorithm like StringHasher::hashMemory. Combining multiple hashes into a new one is typically not as good as that function. I believe that would be both faster and a better algorithm than this. The function that is needed to make StringHasher usable for this would be StringHasher::addMemory that would let you add things other than characters to a hash. Should be really easy to write.

True. Will leave this as a future excercise.

> > Source/WebCore/css/CSSStyleSelector.cpp:2186
> > +        if (!(m_matchedDecls[i] == cacheItem.matchedStyleDeclarations[i]))
> 
> I think this would be easier to read with (a != b) rather than (!(a == b)).

Done.

> You could break this up into two or more return statements to avoid having the awkwardly broken long line.

Done.

> > Source/WebCore/css/CSSStyleSelector.h:230
> > +        bool operator==(const MatchResult&) const;
> 
> Usually as a matter of default C++ style operator== should be a free function, not a member function. In cases where type conversions are possible the free function allows type conversions on the left side. But I guess that might be a little tricky to do inside a class definition.

Made these free functions, just had to make them friends too.

> We should add an operator != that just calls operator== and does the ! part.

Done.

[Antti Koivisto]

Created attachment 112650 [details]
for bots

[WebKit Review Bot]

Attachment 112650 [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/css/CSSStyleSelector.h:336:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 112650 [details]
for bots

Attachment 112650 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10228274

[Antti Koivisto]

Created attachment 112653 [details]
for bots 2

[Antti Koivisto]

http://trac.webkit.org/changeset/98542

[Ryosuke Niwa]

This patch may have broken tables/mozilla_expected_failures/bugs/bug14007-2.html on SL:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r98564%20(34231)/results.html

Blame list: http://trac.webkit.org/log/?action=stop_on_copy&mode=stop_on_copy&rev=98563&stop_rev=98534&limit=100&verbose=on

[David Levin]

(In reply to comment #15)
> This patch may have broken tables/mozilla_expected_failures/bugs/bug14007-2.html on SL:
> http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r98564%20(34231)/results.html
> 
> Blame list: http://trac.webkit.org/log/?action=stop_on_copy&mode=stop_on_copy&rev=98563&stop_rev=98534&limit=100&verbose=on

Looks like https://bugs.webkit.org/show_bug.cgi?id=71019 tracks one regression caused by this change. I'm suspecting that there may be others.

[Antti Koivisto]

http://trac.webkit.org/changeset/98615 fixed tables/mozilla_expected_failures/bugs/bug14007-2.html

https://bugs.webkit.org/show_bug.cgi?id=71019 may be fixed by https://bugs.webkit.org/attachment.cgi?id=112694&action=review .

What else do you suspect?

[David Levin]

(In reply to comment #17)
> http://trac.webkit.org/changeset/98615 fixed tables/mozilla_expected_failures/bugs/bug14007-2.html
> 
> https://bugs.webkit.org/show_bug.cgi?id=71019 may be fixed by https://bugs.webkit.org/attachment.cgi?id=112694&action=review .
> 
> What else do you suspect?

We have a regression on Linux x64 which happened in the range r98537:r98546

For some reason, we're having some non-deterministic behavior in the output of various worker tests and it started in this range. http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests/builds/15333/steps/ui_tests/logs/WorkerContextMultiPort

This change was the only one that seemed to do something interesting that range. I'll like have to see if I can repro the failure locally and if I can, then try to narrow down which patch caused this.

I was just looking forward to a fix to the other issues to see if this issue went away at the same time.

[Pavel Feldman]

It seems like it also makes CSS render on the page:

https://bugs.webkit.org/show_bug.cgi?id=71703

[Eric Seidel (no email)]

Sad that we're getting slower on our HTML5-spec loading benchmark.  But a 40% memory win is nice. :)

[Antti Koivisto]

(In reply to comment #20)
> Sad that we're getting slower on our HTML5-spec loading benchmark.  But a 40% memory win is nice. :)

Really? How much?

[Antti Koivisto]

As I stated in the ChangeLog, I saw about 10% progression in HTML5-spec loading benchmark so I'm suprised you are seeing the opposite.

[Eric Seidel (no email)]

(In reply to comment #22)
> As I stated in the ChangeLog, I saw about 10% progression in HTML5-spec loading benchmark so I'm suprised you are seeing the opposite.

My sincere apologies, I misread your ChangeLog!

[David Barr]

http://code.google.com/p/chromium/issues/detail?id=102521 Google maps fails to render on the latest build
I've traced this bug to http://trac.webkit.org/changeset/98542

[David Kilzer (:ddkilzer)]

(In reply to comment #24)
> http://code.google.com/p/chromium/issues/detail?id=102521 Google maps fails to render on the latest build
> I've traced this bug to http://trac.webkit.org/changeset/98542

Bug 71996.