Bug 74095 – Cache visited link hash

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Antti Koivisto]

Visited link hash is relatively expensive to compute. We can cache it with minimal memory cost for faster style resolve.

[Antti Koivisto]

Created attachment 118398 [details]
patch

[Antti Koivisto]

This trades 8 bytes per link for ~1% speed gain on HTML5 spec loading.

[Antti Koivisto]

The gains are in cases where the style gets recomputed for some reason obviously but that is common.

[Andreas Kling]

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.

[Darin Adler]

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.

[Antti Koivisto]

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

[Andreas Kling]

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

[Antti Koivisto]

http://trac.webkit.org/changeset/102606

Added invalidation on base URL change and some cleanups.