Bug 70931 - Matched declaration cache
Summary: Matched declaration cache
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-26 08:37 PDT by Antti Koivisto
Modified: 2011-11-30 10:32 PST (History)
13 users (show)

See Also:


Attachments
patch (27.24 KB, patch)
2011-10-26 09:31 PDT, Antti Koivisto
darin: review+
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
for bots (38.60 KB, patch)
2011-10-27 01:31 PDT, Antti Koivisto
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
for bots 2 (39.02 KB, patch)
2011-10-27 02:05 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2011-10-26 08:37:20 PDT
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.
Comment 1 Antti Koivisto 2011-10-26 09:31:04 PDT
Created attachment 112550 [details]
patch
Comment 2 Gyuyoung Kim 2011-10-26 09:45:08 PDT
Comment on attachment 112550 [details]
patch

Attachment 112550 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10220257
Comment 3 Darin Adler 2011-10-26 09:47:48 PDT
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.
Comment 4 WebKit Review Bot 2011-10-26 09:53:01 PDT
Comment on attachment 112550 [details]
patch

Attachment 112550 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10220259
Comment 5 WebKit Review Bot 2011-10-26 09:55:59 PDT
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.
Comment 6 Gustavo Noronha (kov) 2011-10-26 10:15:10 PDT
Comment on attachment 112550 [details]
patch

Attachment 112550 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10214548
Comment 7 Dave Hyatt 2011-10-26 10:59:38 PDT
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.
Comment 8 Early Warning System Bot 2011-10-26 11:01:40 PDT
Comment on attachment 112550 [details]
patch

Attachment 112550 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10214564
Comment 9 Antti Koivisto 2011-10-27 00:47:06 PDT
(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.
Comment 10 Antti Koivisto 2011-10-27 01:31:40 PDT
Created attachment 112650 [details]
for bots
Comment 11 WebKit Review Bot 2011-10-27 01:33:47 PDT
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.
Comment 12 Early Warning System Bot 2011-10-27 01:52:19 PDT
Comment on attachment 112650 [details]
for bots

Attachment 112650 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10228274
Comment 13 Antti Koivisto 2011-10-27 02:05:19 PDT
Created attachment 112653 [details]
for bots 2
Comment 14 Antti Koivisto 2011-10-27 02:40:09 PDT
http://trac.webkit.org/changeset/98542
Comment 16 David Levin 2011-10-27 13:58:09 PDT
(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.
Comment 17 Antti Koivisto 2011-10-27 14:07:12 PDT
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?
Comment 18 David Levin 2011-10-27 14:18:09 PDT
(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.
Comment 19 Pavel Feldman 2011-11-07 10:06:05 PST
It seems like it also makes CSS render on the page:

https://bugs.webkit.org/show_bug.cgi?id=71703
Comment 20 Eric Seidel (no email) 2011-11-08 12:00:51 PST
Sad that we're getting slower on our HTML5-spec loading benchmark.  But a 40% memory win is nice. :)
Comment 21 Antti Koivisto 2011-11-08 13:04:20 PST
(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?
Comment 22 Antti Koivisto 2011-11-08 13:08:13 PST
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.
Comment 23 Eric Seidel (no email) 2011-11-08 13:20:48 PST
(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!
Comment 24 David Barr 2011-11-09 20:45:34 PST
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
Comment 25 David Kilzer (:ddkilzer) 2011-11-30 10:32:59 PST
(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.