74095 2011-12-08 08:33:35 -0800 Cache visited link hash 2011-12-12 10:58:12 -0800 1 1 1 Unclassified WebKit CSS 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 koivisto webkit-unassigned kling macpherson webkit.review.bot oldest_to_newest 517439 0 koivisto 2011-12-08 08:33:35 -0800 Visited link hash is relatively expensive to compute. We can cache it with minimal memory cost for faster style resolve. 517447 1 118398 koivisto 2011-12-08 08:51:16 -0800 Created attachment 118398 patch 517449 2 koivisto 2011-12-08 08:51:53 -0800 This trades 8 bytes per link for ~1% speed gain on HTML5 spec loading. 517454 3 koivisto 2011-12-08 09:04:43 -0800 The gains are in cases where the style gets recomputed for some reason obviously but that is common. 517464 4 118398 kling 2011-12-08 09:11:47 -0800 Comment on attachment 118398 patch View in context: https://bugs.webkit.org/attachment.cgi?id=118398&action=review > Source/WebCore/ChangeLog:12 > + On my machine this speeds up style matching on full HTML spec by ~100ms or ~1% of the total > + CPU usage. It makes link elements 8 bytes larger, a relatively minor increase to their overall size. This sounds like a reasonable trade-off to me, though we should either finish the 64-bit LinkHash project, or go back to 32-bit. Adding 4 bytes of always-zero data to HTMLAnchorElement is sad. :( > Source/WebCore/html/HTMLAnchorElement.h:144 > + m_cachedVisitedLinkHash = WebCore::visitedLinkHash(document()->baseURL(), fastGetAttribute(HTMLNames::hrefAttr)); You need to use getAttribute() for hrefAttr, as it's an SVG animatable attribute. 517512 5 118398 darin 2011-12-08 10:09:56 -0800 Comment on attachment 118398 patch View in context: https://bugs.webkit.org/attachment.cgi?id=118398&action=review r=me assuming you fix the fastGetAttribute thing Andreas pointed out > Source/WebCore/ChangeLog:15 > + Note that technically we would need to invalidate these hashes also on dynamic document base URL change > + but that scenario seems to be unsupported by many other parts of the WebKit too. Is there some way we could avoid digging ourselves deeper on this one? Perhaps a base URL change is unusual enough that we could afford to iterate the document when it happens? > Source/WebCore/css/SelectorChecker.cpp:1321 > + else if (const AtomicString* attr = linkAttribute(node)) > + hash = visitedLinkHash(m_document->baseURL(), *attr); Since we’re touching this code I would have spelled out the word attribute. 517586 6 koivisto 2011-12-08 11:12:50 -0800 (In reply to comment #4) > You need to use getAttribute() for hrefAttr, as it's an SVG animatable attribute. The existing code (linkAttribute() in SelectorChecker.cpp) uses fastGetAttribute for all HTML elements. I think this is correct, XLinkNames::hrefAttr is SVG animatable but HTMLNames::hrefAttr is not. 517597 7 kling 2011-12-08 11:20:07 -0800 (In reply to comment #6) > (In reply to comment #4) > > You need to use getAttribute() for hrefAttr, as it's an SVG animatable attribute. > > The existing code (linkAttribute() in SelectorChecker.cpp) uses fastGetAttribute for all HTML elements. I think this is correct, XLinkNames::hrefAttr is SVG animatable but HTMLNames::hrefAttr is not. Arf! I was wrong indeed! My past self already made sure that only SVG elements are checked for animatable attributes in fastGetAttribute (and fastHasAttribute.) Sorry about that :) 519347 8 koivisto 2011-12-12 10:58:12 -0800 http://trac.webkit.org/changeset/102606 Added invalidation on base URL change and some cleanups. 118398 2011-12-08 08:51:16 -0800 2011-12-08 10:09:56 -0800 patch visited-hash-cache-3.patch text/plain 5217 koivisto SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEwMjM0MikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI4IEBACisyMDExLTEyLTA4ICBBbnR0aSBL b2l2aXN0byAgPGFudHRpQGFwcGxlLmNvbT4KKworICAgICAgICBDYWNoZSB2aXNpdGVkIGxpbmsg aGFzaAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NzQw OTUKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBWaXNp dGVkIGxpbmsgaGFzaCBpcyByZWxhdGl2ZWx5IGV4cGVuc2l2ZSB0byBjb21wdXRlLiBXZSBjYW4g Y2FjaGUgaXQgZm9yIGFuY2hvciBlbGVtZW50cworICAgICAgICB3aXRoIG1pbmltYWwgcmVsYXRp dmUgbWVtb3J5IGNvc3QgZm9yIGZhc3RlciBzdHlsZSByZXNvbHZlLgorICAgICAgICAKKyAgICAg ICAgT24gbXkgbWFjaGluZSB0aGlzIHNwZWVkcyB1cCBzdHlsZSBtYXRjaGluZyBvbiBmdWxsIEhU TUwgc3BlYyBieSB+MTAwbXMgb3IgfjElIG9mIHRoZSB0b3RhbAorICAgICAgICBDUFUgdXNhZ2Uu IEl0IG1ha2VzIGxpbmsgZWxlbWVudHMgOCBieXRlcyBsYXJnZXIsIGEgcmVsYXRpdmVseSBtaW5v ciBpbmNyZWFzZSB0byB0aGVpciBvdmVyYWxsIHNpemUuCisgICAgICAgIAorICAgICAgICBOb3Rl IHRoYXQgdGVjaG5pY2FsbHkgd2Ugd291bGQgbmVlZCB0byBpbnZhbGlkYXRlIHRoZXNlIGhhc2hl cyBhbHNvIG9uIGR5bmFtaWMgZG9jdW1lbnQgYmFzZSBVUkwgY2hhbmdlCisgICAgICAgIGJ1dCB0 aGF0IHNjZW5hcmlvIHNlZW1zIHRvIGJlIHVuc3VwcG9ydGVkIGJ5IG1hbnkgb3RoZXIgcGFydHMg b2YgdGhlIFdlYktpdCB0b28uCisKKyAgICAgICAgKiBjc3MvU2VsZWN0b3JDaGVja2VyLmNwcDoK KyAgICAgICAgKFdlYkNvcmU6OlNlbGVjdG9yQ2hlY2tlcjo6ZGV0ZXJtaW5lTGlua1N0YXRlU2xv d0Nhc2UpOgorICAgICAgICAoV2ViQ29yZTo6U2VsZWN0b3JDaGVja2VyOjp2aXNpdGVkU3RhdGVD aGFuZ2VkKToKKyAgICAgICAgKiBodG1sL0hUTUxBbmNob3JFbGVtZW50LmNwcDoKKyAgICAgICAg KFdlYkNvcmU6OkhUTUxBbmNob3JFbGVtZW50OjpIVE1MQW5jaG9yRWxlbWVudCk6CisgICAgICAg IChXZWJDb3JlOjpIVE1MQW5jaG9yRWxlbWVudDo6cGFyc2VNYXBwZWRBdHRyaWJ1dGUpOgorICAg ICAgICAqIGh0bWwvSFRNTEFuY2hvckVsZW1lbnQuaDoKKyAgICAgICAgKFdlYkNvcmU6OkhUTUxB bmNob3JFbGVtZW50Ojp2aXNpdGVkTGlua0hhc2gpOgorCiAyMDExLTEyLTA4ICBUYWthc2hpIFRv eW9zaGltYSAgPHRveW9zaGltQGNocm9taXVtLm9yZz4KIAogICAgICAgICBQcm92aWRlIG1vcmUg c3BlY2lmaWMgZXJyb3IgZGVzY3JpcHRpb24gZm9yIFNvY2tldFN0cmVhbUVycm9yLgpJbmRleDog U291cmNlL1dlYkNvcmUvY3NzL1NlbGVjdG9yQ2hlY2tlci5jcHAKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL1dlYkNvcmUvY3NzL1NlbGVjdG9yQ2hlY2tlci5jcHAJKHJldmlzaW9uIDEwMjExOSkKKysr IFNvdXJjZS9XZWJDb3JlL2Nzcy9TZWxlY3RvckNoZWNrZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBA IC0zNCw2ICszNCw3IEBACiAjaW5jbHVkZSAiRm9jdXNDb250cm9sbGVyLmgiCiAjaW5jbHVkZSAi RnJhbWUuaCIKICNpbmNsdWRlICJGcmFtZVNlbGVjdGlvbi5oIgorI2luY2x1ZGUgIkhUTUxBbmNo b3JFbGVtZW50LmgiCiAjaW5jbHVkZSAiSFRNTEZyYW1lRWxlbWVudEJhc2UuaCIKICNpbmNsdWRl ICJIVE1MSW5wdXRFbGVtZW50LmgiCiAjaW5jbHVkZSAiSFRNTE5hbWVzLmgiCkBAIC0yMzMsOCAr MjM0LDEzIEBAIEVJbnNpZGVMaW5rIFNlbGVjdG9yQ2hlY2tlcjo6ZGV0ZXJtaW5lTGkKICAgICAv LyB0aGF0IHZpc2l0ZWQgbGlua3MgY2FuIGJlIHRlc3RlZCBpbiBwbGF0Zm9ybSBpbmRlcGVuZGVu dCBtYW5uZXIsIHdpdGhvdXQgZXhwbGljaXQgc3VwcG9ydCBpbiB0aGUgdGVzdCBoYXJuZXNzLgog ICAgIGlmIChhdHRyLT5pc0VtcHR5KCkpCiAgICAgICAgIHJldHVybiBJbnNpZGVWaXNpdGVkTGlu azsKKyAgICAKKyAgICBMaW5rSGFzaCBoYXNoOworICAgIGlmIChlbGVtZW50LT5oYXNUYWdOYW1l KGFUYWcpKSAKKyAgICAgICAgaGFzaCA9IHN0YXRpY19jYXN0PEhUTUxBbmNob3JFbGVtZW50Kj4o ZWxlbWVudCktPnZpc2l0ZWRMaW5rSGFzaCgpOworICAgIGVsc2UKKyAgICAgICAgaGFzaCA9IHZp c2l0ZWRMaW5rSGFzaChtX2RvY3VtZW50LT5iYXNlVVJMKCksICphdHRyKTsKIAotICAgIExpbmtI YXNoIGhhc2ggPSB2aXNpdGVkTGlua0hhc2gobV9kb2N1bWVudC0+YmFzZVVSTCgpLCAqYXR0cik7 CiAgICAgaWYgKCFoYXNoKQogICAgICAgICByZXR1cm4gSW5zaWRlVW52aXNpdGVkTGluazsKIApA QCAtMTMwOCw4ICsxMzE0LDEyIEBAIHZvaWQgU2VsZWN0b3JDaGVja2VyOjp2aXNpdGVkU3RhdGVD aGFuZ2UKICAgICBpZiAoIW1fbGlua3NDaGVja2VkRm9yVmlzaXRlZFN0YXRlLmNvbnRhaW5zKHZp c2l0ZWRIYXNoKSkKICAgICAgICAgcmV0dXJuOwogICAgIGZvciAoTm9kZSogbm9kZSA9IG1fZG9j dW1lbnQ7IG5vZGU7IG5vZGUgPSBub2RlLT50cmF2ZXJzZU5leHROb2RlKCkpIHsKLSAgICAgICAg Y29uc3QgQXRvbWljU3RyaW5nKiBhdHRyID0gbGlua0F0dHJpYnV0ZShub2RlKTsKLSAgICAgICAg aWYgKGF0dHIgJiYgdmlzaXRlZExpbmtIYXNoKG1fZG9jdW1lbnQtPmJhc2VVUkwoKSwgKmF0dHIp ID09IHZpc2l0ZWRIYXNoKQorICAgICAgICBMaW5rSGFzaCBoYXNoID0gMDsKKyAgICAgICAgaWYg KG5vZGUtPmhhc1RhZ05hbWUoYVRhZykpCisgICAgICAgICAgICBoYXNoID0gc3RhdGljX2Nhc3Q8 SFRNTEFuY2hvckVsZW1lbnQqPihub2RlKS0+dmlzaXRlZExpbmtIYXNoKCk7CisgICAgICAgIGVs c2UgaWYgKGNvbnN0IEF0b21pY1N0cmluZyogYXR0ciA9IGxpbmtBdHRyaWJ1dGUobm9kZSkpCisg ICAgICAgICAgICBoYXNoID0gdmlzaXRlZExpbmtIYXNoKG1fZG9jdW1lbnQtPmJhc2VVUkwoKSwg KmF0dHIpOworICAgICAgICBpZiAoaGFzaCA9PSB2aXNpdGVkSGFzaCkKICAgICAgICAgICAgIG5v ZGUtPnNldE5lZWRzU3R5bGVSZWNhbGMoKTsKICAgICB9CiB9CkluZGV4OiBTb3VyY2UvV2ViQ29y ZS9odG1sL0hUTUxBbmNob3JFbGVtZW50LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29y ZS9odG1sL0hUTUxBbmNob3JFbGVtZW50LmNwcAkocmV2aXNpb24gMTAyMTE5KQorKysgU291cmNl L1dlYkNvcmUvaHRtbC9IVE1MQW5jaG9yRWxlbWVudC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTUw LDYgKzUwLDcgQEAgSFRNTEFuY2hvckVsZW1lbnQ6OkhUTUxBbmNob3JFbGVtZW50KGNvbgogICAg IDogSFRNTEVsZW1lbnQodGFnTmFtZSwgZG9jdW1lbnQpCiAgICAgLCBtX3dhc1NoaWZ0S2V5RG93 bk9uTW91c2VEb3duKGZhbHNlKQogICAgICwgbV9saW5rUmVsYXRpb25zKDApCisgICAgLCBtX2Nh Y2hlZFZpc2l0ZWRMaW5rSGFzaCgwKQogewogfQogCkBAIC0yMjEsNiArMjIyLDcgQEAgdm9pZCBI VE1MQW5jaG9yRWxlbWVudDo6cGFyc2VNYXBwZWRBdHRyaQogICAgICAgICAgICAgICAgIGF0dHIt PnNldFZhbHVlKG51bGxBdG9tKTsKICAgICAgICAgICAgIH0KICAgICAgICAgfQorICAgICAgICBt X2NhY2hlZFZpc2l0ZWRMaW5rSGFzaCA9IDA7CiAgICAgfSBlbHNlIGlmIChhdHRyLT5uYW1lKCkg PT0gbmFtZUF0dHIgfHwKICAgICAgICAgICAgICBhdHRyLT5uYW1lKCkgPT0gdGl0bGVBdHRyKSB7 CiAgICAgICAgIC8vIERvIG5vdGhpbmcuCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxB bmNob3JFbGVtZW50LmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MQW5j aG9yRWxlbWVudC5oCShyZXZpc2lvbiAxMDIxMTkpCisrKyBTb3VyY2UvV2ViQ29yZS9odG1sL0hU TUxBbmNob3JFbGVtZW50LmgJKHdvcmtpbmcgY29weSkKQEAgLTI1LDYgKzI1LDggQEAKICNkZWZp bmUgSFRNTEFuY2hvckVsZW1lbnRfaAogCiAjaW5jbHVkZSAiSFRNTEVsZW1lbnQuaCIKKyNpbmNs dWRlICJIVE1MTmFtZXMuaCIKKyNpbmNsdWRlICJMaW5rSGFzaC5oIgogCiBuYW1lc3BhY2UgV2Vi Q29yZSB7CiAKQEAgLTkyLDYgKzk0LDggQEAgcHVibGljOgogCiAgICAgYm9vbCBoYXNSZWwodWlu dDMyX3QgcmVsYXRpb24pIGNvbnN0OwogICAgIHZvaWQgc2V0UmVsKGNvbnN0IFN0cmluZyYpOwor ICAgIAorICAgIExpbmtIYXNoIHZpc2l0ZWRMaW5rSGFzaCgpIGNvbnN0OwogCiBwcm90ZWN0ZWQ6 CiAgICAgSFRNTEFuY2hvckVsZW1lbnQoY29uc3QgUXVhbGlmaWVkTmFtZSYsIERvY3VtZW50Kik7 CkBAIC0xMzEsOCArMTM1LDE2IEBAIHByaXZhdGU6CiAgICAgUmVmUHRyPEVsZW1lbnQ+IG1fcm9v dEVkaXRhYmxlRWxlbWVudEZvclNlbGVjdGlvbk9uTW91c2VEb3duOwogICAgIGJvb2wgbV93YXNT aGlmdEtleURvd25Pbk1vdXNlRG93biA6IDE7CiAgICAgdWludDMyX3QgbV9saW5rUmVsYXRpb25z IDogMzE7CisgICAgbXV0YWJsZSBMaW5rSGFzaCBtX2NhY2hlZFZpc2l0ZWRMaW5rSGFzaDsKIH07 CiAKK2lubGluZSBMaW5rSGFzaCBIVE1MQW5jaG9yRWxlbWVudDo6dmlzaXRlZExpbmtIYXNoKCkg Y29uc3QKK3sKKyAgICBpZiAoIW1fY2FjaGVkVmlzaXRlZExpbmtIYXNoKQorICAgICAgICBtX2Nh Y2hlZFZpc2l0ZWRMaW5rSGFzaCA9IFdlYkNvcmU6OnZpc2l0ZWRMaW5rSGFzaChkb2N1bWVudCgp LT5iYXNlVVJMKCksIGZhc3RHZXRBdHRyaWJ1dGUoSFRNTE5hbWVzOjpocmVmQXR0cikpOworICAg IHJldHVybiBtX2NhY2hlZFZpc2l0ZWRMaW5rSGFzaDsgCit9CisKIC8vIEZ1bmN0aW9ucyBzaGFy ZWQgd2l0aCB0aGUgb3RoZXIgYW5jaG9yIGVsZW1lbnRzIChpLmUuLCBTVkcpLgogCiBib29sIGlz RW50ZXJLZXlLZXlkb3duRXZlbnQoRXZlbnQqKTsK