Bug 92687

Created attachment 155385 [details] Patch

Created attachment 155401 [details] Patch

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.

(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.

(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.

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.

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.

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();

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.

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

Comment on attachment 155401 [details] Patch Confused bot...

(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.

Comment on attachment 155401 [details] Patch Clearing flags on attachment: 155401 Committed r124260: <http://trac.webkit.org/changeset/124260>

All reviewed patches have been landed. Closing bug.

(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.