92687 2012-07-30 15:32:31 -0700 StyleResolver::canShareStyleWithElement does not need to use getAttribute for classAttr in the non-SVG case 2012-07-31 16:33:25 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 92258 1 eric eric cmarcelo jchaffraix kling koivisto macpherson menard pdr simon.fraser webkit.review.bot zimmermann oldest_to_newest 682469 0 eric 2012-07-30 15:32:31 -0700 StyleResolver::canShareStyleWithElement does not need to use getAttribute for classAttr 682471 1 155385 eric 2012-07-30 15:34:52 -0700 Created attachment 155385 Patch 682540 2 155401 eric 2012-07-30 17:06:14 -0700 Created attachment 155401 Patch 683064 3 155401 koivisto 2012-07-31 07:56:10 -0700 Comment on attachment 155401 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. 683069 4 kling 2012-07-31 07:57:45 -0700 (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. 683286 5 eric 2012-07-31 11:25:37 -0700 (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. 683288 6 eric 2012-07-31 11:26:31 -0700 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. 683452 7 eric 2012-07-31 14:00:28 -0700 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. 683468 8 eric 2012-07-31 14:17:17 -0700 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(); 683469 9 155401 eric 2012-07-31 14:17:57 -0700 Comment on attachment 155401 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. 683483 10 155401 webkit.review.bot 2012-07-31 14:31:21 -0700 Comment on attachment 155401 Patch Rejecting attachment 155401 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 683492 11 155401 eric 2012-07-31 14:44:36 -0700 Comment on attachment 155401 Patch Confused bot... 683508 12 koivisto 2012-07-31 15:00:20 -0700 (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. 683564 13 155401 webkit.review.bot 2012-07-31 15:55:34 -0700 Comment on attachment 155401 Patch Clearing flags on attachment: 155401 Committed r124260: <http://trac.webkit.org/changeset/124260> 683565 14 webkit.review.bot 2012-07-31 15:55:40 -0700 All reviewed patches have been landed. Closing bug. 683621 15 eric 2012-07-31 16:33:25 -0700 (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. 155385 2012-07-30 15:34:52 -0700 2012-07-30 17:06:10 -0700 Patch bug-92687-20120730153436.patch text/plain 1904 eric U3VidmVyc2lvbiBSZXZpc2lvbjogMTI0MDc5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMjNkYzRjZGM4Y2FiMjY4 NGJiNGQwZWUzNGIzOTRmYjI1ZWIxZTI3Yy4uMTI2NGRhMjU2ZGZiZTNkNWViYjE3MWI1ODNhZTE5 MTZhZDhlZTRlZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE5IEBACiAyMDEyLTA3LTMwICBFcmlj IFNlaWRlbCAgPGVyaWNAd2Via2l0Lm9yZz4KIAorICAgICAgICBTdHlsZVJlc29sdmVyOjpjYW5T aGFyZVN0eWxlV2l0aEVsZW1lbnQgZG9lcyBub3QgbmVlZCB0byB1c2UgZ2V0QXR0cmlidXRlIGZv ciBjbGFzc0F0dHIKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTkyNjg3CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgQXMgZmFyIGFzIEkgY2FuIHRlbGwgZ2V0QXR0cmlidXRlIGlzIG5vdCByZXF1aXJlZCBmb3Ig Y2xhc3NBdHRyLgorICAgICAgICAxMCUgb2YgdGltZSBzcGVudCBpbiB0aGlzIGZ1bmN0aW9uIGlz IHNwZW50IGNhbGxpbmcgZ2V0QXR0cmlidXRlIGZvciBubyByZWFzb24sCisgICAgICAgIGZpeGlu ZyBpdCB0byBmYXN0R2V0QXR0cmlidXRlIGRyb3BzIHRoZSBwZXJjZW50IG9mIHRpbWUgc3BlbnQg dG8gMC4zJS4KKworICAgICAgICAqIGNzcy9TdHlsZVJlc29sdmVyLmNwcDoKKyAgICAgICAgKFdl YkNvcmU6OlN0eWxlUmVzb2x2ZXI6OmNvbGxlY3RNYXRjaGluZ1J1bGVzRm9yTGlzdCk6CisKKzIw MTItMDctMzAgIEVyaWMgU2VpZGVsICA8ZXJpY0B3ZWJraXQub3JnPgorCiAgICAgICAgIEdyaWQg RGVtbyBzcGVuZHMgMS41JSBvZiB0b3RhbCB0aW1lIGFsbG9jYXRpbmcgUGF0aCBvYmplY3RzIGlu IFJlbmRlckJveE1vZGVsT2JqZWN0OjpwYWludEJvcmRlclNpZGVzCiAgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD05MjI1MgogCmRpZmYgLS1naXQgYS9Tb3Vy Y2UvV2ViQ29yZS9jc3MvU3R5bGVSZXNvbHZlci5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9jc3MvU3R5 bGVSZXNvbHZlci5jcHAKaW5kZXggZDNiZjQxYjk0Mzg4MGQ4Nzc1MDVkZjZiOTA4MGY0OTdkOGRj NjdlZC4uZTc2OGU3ZDY0YjgzNGY2YjI4YTY5NjVmZWQ2ZDIwZjgwZWNmM2E0ZCAxMDA2NDQKLS0t IGEvU291cmNlL1dlYkNvcmUvY3NzL1N0eWxlUmVzb2x2ZXIuY3BwCisrKyBiL1NvdXJjZS9XZWJD b3JlL2Nzcy9TdHlsZVJlc29sdmVyLmNwcApAQCAtMTQ4Miw3ICsxNDgyLDcgQEAgYm9vbCBTdHls ZVJlc29sdmVyOjpjYW5TaGFyZVN0eWxlV2l0aEVsZW1lbnQoU3R5bGVkRWxlbWVudCogZWxlbWVu dCkgY29uc3QKICAgICBpZiAoZWxlbWVudEhhc0RpcmVjdGlvbkF1dG8oZWxlbWVudCkgfHwgZWxl bWVudEhhc0RpcmVjdGlvbkF1dG8obV9lbGVtZW50KSkKICAgICAgICAgcmV0dXJuIGZhbHNlOwog Ci0gICAgaWYgKGVsZW1lbnQtPmhhc0NsYXNzKCkgJiYgbV9lbGVtZW50LT5nZXRBdHRyaWJ1dGUo Y2xhc3NBdHRyKSAhPSBlbGVtZW50LT5nZXRBdHRyaWJ1dGUoY2xhc3NBdHRyKSkKKyAgICBpZiAo ZWxlbWVudC0+aGFzQ2xhc3MoKSAmJiBtX2VsZW1lbnQtPmZhc3RHZXRBdHRyaWJ1dGUoY2xhc3NB dHRyKSAhPSBlbGVtZW50LT5mYXN0R2V0QXR0cmlidXRlKGNsYXNzQXR0cikpCiAgICAgICAgIHJl dHVybiBmYWxzZTsKIAogICAgIGlmIChlbGVtZW50LT5hdHRyaWJ1dGVTdHlsZSgpICYmICFhdHRy aWJ1dGVTdHlsZXNFcXVhbChlbGVtZW50LT5hdHRyaWJ1dGVTdHlsZSgpLCBtX3N0eWxlZEVsZW1l bnQtPmF0dHJpYnV0ZVN0eWxlKCkpKQo= 155401 2012-07-30 17:06:14 -0700 2012-07-31 15:55:33 -0700 Patch bug-92687-20120730170558.patch text/plain 2372 eric U3VidmVyc2lvbiBSZXZpc2lvbjogMTI0MDc5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMjNkYzRjZGM4Y2FiMjY4 NGJiNGQwZWUzNGIzOTRmYjI1ZWIxZTI3Yy4uNGQ2MDhlM2M4ZTA5ZmYxODYyMmU2MmE2ODg5NmU1 OWVmNmVjOGY0NiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE5IEBACiAyMDEyLTA3LTMwICBFcmlj IFNlaWRlbCAgPGVyaWNAd2Via2l0Lm9yZz4KIAorICAgICAgICBTdHlsZVJlc29sdmVyOjpjYW5T aGFyZVN0eWxlV2l0aEVsZW1lbnQgZG9lcyBub3QgbmVlZCB0byB1c2UgZ2V0QXR0cmlidXRlIGZv ciBjbGFzc0F0dHIgaW4gdGhlIG5vbi1TVkcgY2FzZQorICAgICAgICBodHRwczovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9OTI2ODcKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBQcmV2aW91c2x5IDEwJSBvZiBzYW1wbGVzIGluIGNhblNo YXJlU3R5bGVXaXRoRWxlbWVudCBoaXQgdGhpcyBsaW5lLCBhZnRlciB0aGlzIGNoYW5nZSA8IDEl IGRvLgorICAgICAgICBUaGlzIGlzIGEgc21hbGwgc3BlZWR1cCBmb3IgdGhlIG5vbi1TVkcgY2Fz ZS4gIFNpbmNlIHRoaXMgY2FsbCBpcyBtYWRlIGZvciBldmVyeSBlbGVtZW50CisgICAgICAgIHdo ZW4gc3R5bGUgaXMgcmVzb2x2ZWQsIGFueSBzbWFsbCBzcGVlZHVwIGluIHRoaXMgZnVuY3Rpb24g bWF0dGVycyBvbiBsYXJnZSBkb2N1bWVudHMuCisKKyAgICAgICAgKiBjc3MvU3R5bGVSZXNvbHZl ci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpTdHlsZVJlc29sdmVyOjpjb2xsZWN0TWF0Y2hpbmdS dWxlc0Zvckxpc3QpOgorCisyMDEyLTA3LTMwICBFcmljIFNlaWRlbCAgPGVyaWNAd2Via2l0Lm9y Zz4KKwogICAgICAgICBHcmlkIERlbW8gc3BlbmRzIDEuNSUgb2YgdG90YWwgdGltZSBhbGxvY2F0 aW5nIFBhdGggb2JqZWN0cyBpbiBSZW5kZXJCb3hNb2RlbE9iamVjdDo6cGFpbnRCb3JkZXJTaWRl cwogICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9OTIyNTIK IApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvY3NzL1N0eWxlUmVzb2x2ZXIuY3BwIGIvU291 cmNlL1dlYkNvcmUvY3NzL1N0eWxlUmVzb2x2ZXIuY3BwCmluZGV4IGQzYmY0MWI5NDM4ODBkODc3 NTA1ZGY2YjkwODBmNDk3ZDhkYzY3ZWQuLjkxZjk5OWE0NzU0NjFmZGM0MzU2ODc0OTA5OWU1OWVh ODcwN2I2NTUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2Nzcy9TdHlsZVJlc29sdmVyLmNw cAorKysgYi9Tb3VyY2UvV2ViQ29yZS9jc3MvU3R5bGVSZXNvbHZlci5jcHAKQEAgLTE0ODIsOCAr MTQ4MiwxNyBAQCBib29sIFN0eWxlUmVzb2x2ZXI6OmNhblNoYXJlU3R5bGVXaXRoRWxlbWVudChT dHlsZWRFbGVtZW50KiBlbGVtZW50KSBjb25zdAogICAgIGlmIChlbGVtZW50SGFzRGlyZWN0aW9u QXV0byhlbGVtZW50KSB8fCBlbGVtZW50SGFzRGlyZWN0aW9uQXV0byhtX2VsZW1lbnQpKQogICAg ICAgICByZXR1cm4gZmFsc2U7CiAKLSAgICBpZiAoZWxlbWVudC0+aGFzQ2xhc3MoKSAmJiBtX2Vs ZW1lbnQtPmdldEF0dHJpYnV0ZShjbGFzc0F0dHIpICE9IGVsZW1lbnQtPmdldEF0dHJpYnV0ZShj bGFzc0F0dHIpKQotICAgICAgICByZXR1cm4gZmFsc2U7CisgICAgaWYgKGVsZW1lbnQtPmhhc0Ns YXNzKCkpIHsKKyNpZiBFTkFCTEUoU1ZHKQorICAgICAgICAvLyBTVkcgZWxlbWVudHMgcmVxdWly ZSBhIChzbG93ISkgZ2V0QXR0cmlidXRlIGNvbXBhcmlzaW9uIGJlY2F1c2UgImNsYXNzIiBpcyBh biBhbmltYXRhYmxlIGF0dHJpYnV0ZSBmb3IgU1ZHLgorICAgICAgICBpZiAoZWxlbWVudC0+aXNT VkdFbGVtZW50KCkpIHsKKyAgICAgICAgICAgIGlmIChlbGVtZW50LT5nZXRBdHRyaWJ1dGUoY2xh c3NBdHRyKSAhPSBtX2VsZW1lbnQtPmdldEF0dHJpYnV0ZShjbGFzc0F0dHIpKQorICAgICAgICAg ICAgICAgIHJldHVybiBmYWxzZTsKKyAgICAgICAgfSBlbHNlCisjZW5kaWYKKyAgICAgICAgaWYg KGVsZW1lbnQtPmZhc3RHZXRBdHRyaWJ1dGUoY2xhc3NBdHRyKSAhPSBtX2VsZW1lbnQtPmZhc3RH ZXRBdHRyaWJ1dGUoY2xhc3NBdHRyKSkKKyAgICAgICAgICAgIHJldHVybiBmYWxzZTsKKyAgICB9 CiAKICAgICBpZiAoZWxlbWVudC0+YXR0cmlidXRlU3R5bGUoKSAmJiAhYXR0cmlidXRlU3R5bGVz RXF1YWwoZWxlbWVudC0+YXR0cmlidXRlU3R5bGUoKSwgbV9zdHlsZWRFbGVtZW50LT5hdHRyaWJ1 dGVTdHlsZSgpKSkKICAgICAgICAgcmV0dXJuIGZhbHNlOwo=