[webkit-reviews] review denied: [Bug 100057] Replace NodeRareData hash map with a union on m_renderer : [Attachment 170032] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Elliott Sprehn
<esprehn at chromium.org>'s request for review:
Bug 100057: Replace NodeRareData hash map with a union on m_renderer
https://bugs.webkit.org/show_bug.cgi?id=100057

Attachment 170032: Patch
https://bugs.webkit.org/attachment.cgi?id=170032&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=170032&action=review


Looks like a good change, but requires some additional performance testing and
tuning, I suspect.

Mac build failed because what was an inline function is now non-inline. You’ll
have to update the export file because of that.

Not sure why the GTK and Windows builds failed.

> Source/WebCore/ChangeLog:15
> +	   Use a union on Node::m_renderer between NodeRareData* and
RenderObject*. This removes
> +	   the overhead of accessing rare data and the memory from the map. We
now get the 5%
> +	   performance increase observed in Bug 90059 but when accessing node
lists on any node.
> +	   This should be a perf win on node-list-access.html of a similar
percentage.
> +
> +	   The extra pointer and conditional does not regress performance when
accessing node
> +	   lists through methods like getElementsByClassName on the document. I
also removed
> +	   multiple accesses down hot code paths like recalcStyle.

How did you measure performance outside of those DOM benchmarks? Saying “I
removed multiple accesses down hot code paths” sounds like optimization by code
inspection, but we need to actually test whether this measurably slows
performance in code that was heavily accessing the renderer. Did you do some
kind of testing?

> Source/WebCore/dom/Node.cpp:483
> +    data->setRenderer(renderer());

Really should be:

    data->setRenderer(m_data.m_renderer);

No reason to check HasRareDataFlag an extra time here, and the more verbose
version also has the advantage of being slightly clearer about what’s going on.


> Source/WebCore/dom/Node.cpp:502
> +    RenderObject* renderer = this->renderer();

Really should be:

    RenderObject* renderer = m_data.m_rareData->renderer();

No reason to check HasRareDataFlag an extra time here, and the more verbose
version also has the advantage of being slightly clearer about what’s going on.


> Source/WebCore/dom/Node.cpp:511
> +RenderObject* Node::renderer() const
> +{
> +    return hasRareData() ? m_data.m_rareData->renderer() :
m_data.m_renderer;
> +}

Why isn’t this inlined? It’s really OK to have function overhead every time
this is called? I’d expect that we’d at least put the !hasRareData() case in
the header and inline it.

> Source/WebCore/dom/Node.cpp:1427
> +    RenderObject* renderer = this->renderer();
> +    if (renderer)
> +	   renderer->setAnimatableStyle(s);

Our usual style would be to define the variable inside the if statement.

> Source/WebCore/dom/Node.cpp:2842
> +    RenderObject* renderer = this->renderer();
> +    if (renderer)
> +	   info.addMember(renderer->style());

Our usual style would be to define the variable inside the if statement.

> Source/WebCore/dom/NodeRareData.h:183
> +	   : m_renderer(0)

A shame to set m_renderer here since it will always get set again as soon as
the createRareData function returns.

> Source/WebCore/dom/NodeRenderStyle.h:39
> -    if (m_renderer) 
> -	   return m_renderer->style();
> +    if (renderer())
> +	   return renderer()->style();

Strange that you changed this, but did not put the renderer into a local
variable.