[Webkit-unassigned] [Bug 98694] [Performance] Speed-up DOM tree traversal on ARM platforms

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=98694





--- Comment #6 from Darin Adler <darin at apple.com>  2012-10-08 18:33:44 PST ---
(From update of attachment 167661)
View in context: https://bugs.webkit.org/attachment.cgi?id=167661&action=review

Thanks for submitting this. It’s cool that this can make DOM traversal much faster. I’m sure we can figure out a form of this that can go into the tree.

There are lots of design decisions that are not explained in this code. Need more comments explaining why, not just mystery code.

Clearly this prefetching code could make some operations slower. It’s good that it makes benchmarks faster, but we should also construct the pathological cases that become slower so we can see how much slower.

> Source/WTF/wtf/Compiler.h:222
> +#define PREFETCH(x)

This isn’t valuable. Seems highly unlikely we will want to compile code that uses the PREFETCH macro without also having an ENABLE conditional around it. I think we can just leave this out.

> Source/WTF/wtf/Platform.h:1033
> +/* Prefetch to minimize cache-miss latency by moving data into a cache before it is accessed */
> +#if !defined(BUILTIN_PREFETCH) && CPU(ARM) && COMPILER_SUPPORTS(BUILTIN_PREFETCH)
> +#define ENABLE_PREFETCH 1
> +#endif

I don’t understand the !defined(BUILTIN_PREFETCH) in this expression.

I don’t like the use of CPU(ARM) here, without a comment mentioning why it’s appropriate.

I don’t think ENABLE(PREFETCH) is specific enough. There are so many things that prefetch could mean. We should chose a name that is specific enough to make it clear that this is memory prefetching.

> Source/WebCore/dom/ContainerNode.h:279
> +    prefetch();

This function name is unclear. It doesn’t say what is being fetched. It’s not appropriate to have a function named just “prefetch” on a node. Need a human-comprehensible name.

At each call site we need to say why it’s right to call prefetch there. Otherwise it just looks like you made magic decisions about where to put these calls, and no one in the future can figure out whether it’s safe to remove them and where they need to be added then.

> Source/WebCore/dom/ContainerNode.h:287
> +    prefetch();

Not good to put this above the comment below, which should be at the top of the function.

> Source/WebCore/dom/Node.h:103
> +const int prefetchStride = 6;

The mystery magic constant needs a comment explaining where it came from. If you determined the value by testing, it needs to explain that. If you determined it by some theoretical means it needs to explain that.

> Source/WebCore/dom/Node.h:268
> +    void updatePrefetchTarget();
> +    void setPrefetchTarget(Node *prefetch) { m_prefetchTarget = prefetch; }

These new functions shouldn’t be crammed in with the setPrevious/NextSibling call. They should go in their own paragraph with comments that help explain the mechanism.

> Source/WebCore/dom/Node.h:269
> +    void setNextSibling(Node* next) { m_next = next; updatePrefetchTarget(); }

This is an inappropriate level to add the prefetch updating. It should be done at call sites, not in this setter, unless that’s completely impractical. This low level setter needs to remain a low level setter.

> Source/WebCore/dom/Node.h:815
> +#if ENABLE(PREFETCH)
> +    Node* m_prefetchTarget;
> +#endif

Is it important to have this data member here? Could it be put at the end of the data members instead where it would not be in the middle of others, making them confusing.

I don’t think m_prefetchTarget is the right name for this. Instead I would like it to be something that expresses more clearly what it’s supposed to represent, rather than how it’s used.

> Source/WebCore/dom/Node.h:909
> +    if (m_next) {
> +        Node* from = this;
> +        Node* n = from->traversePreviousNodePostOrder();
> +        for (int skew = prefetchStride; skew && n; skew--) {
> +            from = n;
> +            n = n->traversePreviousNodePostOrder();
> +        }
> +        from->setPrefetchTarget(m_next);
> +    }

This function is completely mysterious to me. There’s no why comment explaining why going backwards is what we want to do. No explanation of what “from” means as a variable name, or what “skew” means.

I gather that m_prefetchTarget is a sort of inaccurate “skip” pointer that contains an often incorrect, often correct pointer to the sixth sibling from the current sibling.

I can make guesses about why it does what it does, but I should not have to guess. You should be explaining this in comments.

> Source/WebCore/dom/Node.h:916
> +    if (LIKELY(m_prefetchTarget != 0))

Need a comment here explaining it’s OK to have a bad pointer as the prefetch target; otherwise this code looks like a bug because it uses a dangling pointer.

Do we really want this branch? If it’s OK to prefetch with any pointer, then why not initialize m_prefetchTarget to this rather than having 0 as a magic value?

> Source/WebCore/dom/Node.h:917
> +        PREFETCH(((char *) m_prefetchTarget));

The cast to (char *) should be something that’s part of the PREFETCH macro rather than done at each call site, since it’s part of the idiosyncrasy of a particular compiler’s feature and might be different in other compilers.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.