Bug 74095 - Cache visited link hash
Summary: Cache visited link hash
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-08 08:33 PST by Antti Koivisto
Modified: 2011-12-12 10:58 PST (History)
3 users (show)

See Also:


Attachments
patch (5.09 KB, patch)
2011-12-08 08:51 PST, Antti Koivisto
darin: review+
kling: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2011-12-08 08:33:35 PST
Visited link hash is relatively expensive to compute. We can cache it with minimal memory cost for faster style resolve.
Comment 1 Antti Koivisto 2011-12-08 08:51:16 PST
Created attachment 118398 [details]
patch
Comment 2 Antti Koivisto 2011-12-08 08:51:53 PST
This trades 8 bytes per link for ~1% speed gain on HTML5 spec loading.
Comment 3 Antti Koivisto 2011-12-08 09:04:43 PST
The gains are in cases where the style gets recomputed for some reason obviously but that is common.
Comment 4 Andreas Kling 2011-12-08 09:11:47 PST
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 5 Darin Adler 2011-12-08 10:09:56 PST
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.
Comment 6 Antti Koivisto 2011-12-08 11:12:50 PST
(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.
Comment 7 Andreas Kling 2011-12-08 11:20:07 PST
(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 :)
Comment 8 Antti Koivisto 2011-12-12 10:58:12 PST
http://trac.webkit.org/changeset/102606

Added invalidation on base URL change and some cleanups.