Bug 92687 – StyleResolver::canShareStyleWithElement does not need to use getAttribute for classAttr in the non-SVG case

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

StyleResolver::canShareStyleWithElement does not need to use getAttribute for classAttr

[Eric Seidel (no email)]

Created attachment 155385 [details]
Patch

[Eric Seidel (no email)]

Created attachment 155401 [details]
Patch

[Antti Koivisto]

Comment on attachment 155401 [details]
Patch

Seems fine as is. However I suspect SVG elements rarely end up sharing style anyway. It would be good to collect some statistics. If this is indeed the case we could just do if (element->isSVGElement()) return false; in the beginning, simplifying and speeding up the code.

[Andreas Kling]

(In reply to comment #3)
> (From update of attachment 155401 [details])
> Seems fine as is. However I suspect SVG elements rarely end up sharing style anyway. It would be good to collect some statistics. If this is indeed the case we could just do if (element->isSVGElement()) return false; in the beginning, simplifying and speeding up the code.

Right. And if it's a helpful optimization for SVG, we could consider splitting the function into SVG and HTML versions to only check the things that make sense for either element type.

[Eric Seidel (no email)]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 155401 [details] [details])
> > Seems fine as is. However I suspect SVG elements rarely end up sharing style anyway. It would be good to collect some statistics. If this is indeed the case we could just do if (element->isSVGElement()) return false; in the beginning, simplifying and speeding up the code.
> 
> Right. And if it's a helpful optimization for SVG, we could consider splitting the function into SVG and HTML versions to only check the things that make sense for either element type.

I really like that idea.

[Eric Seidel (no email)]

Is there something you'd like changed Antti, or can I just cq+ this?  I'm confused by your comment.  I'm happy to file bugs about future potential optimizations to this function if that's what you're looking for.

[Eric Seidel (no email)]

http://trac.webkit.org/browser/trunk/PerformanceTests/PageLoad/svg/files/33041-Samurai.svg - 38 styles, 12 shared.

http://trac.webkit.org/browser/trunk/PerformanceTests/PageLoad/svg/files/42450-under%20the%20see.svg - 906 styles, 432 shared.

http://trac.webkit.org/browser/trunk/PerformanceTests/PageLoad/svg/files/42470-flower_from_my_garden_v2.svg 536 styles, 208 shared.

Seems the sharing percentage is quite high for the 3 SVGs I checked.

[Eric Seidel (no email)]

For the record, this was the diff I used to collect my numbers:

diff --git a/Source/WebCore/css/StyleResolver.cpp b/Source/WebCore/css/StyleResolver.cpp
index 91f999a..55825f4 100644
--- a/Source/WebCore/css/StyleResolver.cpp
+++ b/Source/WebCore/css/StyleResolver.cpp
@@ -1740,10 +1740,13 @@ PassRefPtr<RenderStyle> StyleResolver::styleForElement(Element* element, RenderS
     initElement(element);
     initForStyleResolve(element, defaultParent);
     m_regionForStyling = regionForStyling;
+    printf("STYLE\n");
     if (sharingBehavior == AllowStyleSharing) {
         RenderStyle* sharedStyle = locateSharedStyle();
-        if (sharedStyle)
+        if (sharedStyle) {
+            printf("YES\n");
             return sharedStyle;
+        }
     }
 
     m_style = RenderStyle::create();

[Eric Seidel (no email)]

Comment on attachment 155401 [details]
Patch

I believe I've answered everyone's questions.  Going to cq+ this.  If there are any issues, or you'd like follow-up bugs I'm happy to do more here.

[WebKit Review Bot]

Comment on attachment 155401 [details]
Patch

Rejecting attachment 155401 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13411123

[Eric Seidel (no email)]

Comment on attachment 155401 [details]
Patch

Confused bot...

[Antti Koivisto]

(In reply to comment #6)
> Is there something you'd like changed Antti, or can I just cq+ this?  I'm confused by your comment.  I'm happy to file bugs about future potential optimizations to this function if that's what you're looking for.

I just wanted to give you chance to consider alternatives before landing. As I said I think the patch is fine as it is.

[WebKit Review Bot]

Comment on attachment 155401 [details]
Patch

Clearing flags on attachment: 155401

Committed r124260: <http://trac.webkit.org/changeset/124260>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

(In reply to comment #12)
> (In reply to comment #6)
> > Is there something you'd like changed Antti, or can I just cq+ this?  I'm confused by your comment.  I'm happy to file bugs about future potential optimizations to this function if that's what you're looking for.
> 
> I just wanted to give you chance to consider alternatives before landing. As I said I think the patch is fine as it is.

Ah.  Thank you.  I agree with you that we should split canShareStyleWithElement into SVG and non-SVG variants in the future.