Summary: | Cache visited link hash | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | kling, macpherson, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Antti Koivisto
2011-12-08 08:33:35 PST
Created attachment 118398 [details]
patch
This trades 8 bytes per link for ~1% speed gain on HTML5 spec loading. The gains are in cases where the style gets recomputed for some reason obviously but that is common. Comment on attachment 118398 [details] 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. Comment on attachment 118398 [details] 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. (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. (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 :) http://trac.webkit.org/changeset/102606 Added invalidation on base URL change and some cleanups. |