[webkit-reviews] review denied: [Bug 65668] Optimize floating elements lookup : [Attachment 103083] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Chiculita Alexandru
<achicu at adobe.com>'s request for review:
Bug 65668: Optimize floating elements lookup
https://bugs.webkit.org/show_bug.cgi?id=65668

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

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=103083&action=review


Overall this looks pretty good. I have one major issue with the patch though,
and that is that I would prefer it if the interval tree was built lazily if
possible. I could be wrong about this but if you have something like this:

<img style="float:left;width:10px;height:1000px">
<div><div><div>....lots more nested divs.... <div>Some text

We propagate floating objects into all of the nested divs. This duplication is
bad enough and is something we need to fix, but if you don't build the interval
tree lazily you will also have tons of duplication in each div, even though
nobody actually *tested* for overlap in any of the intermediate divs.

It seems like you could have a notion of whether or not a float's overlap has
been computed that is independent of being placed and add to the interval tree
lazily when someone actually does an overlap test.

I think I'd like to see a deeply nested div performance test in which many many
floats intrude into all of those divs to verify the potential performance gain
from the optimization I'm suggesting. That will also help us measure when we
remove all of the duplicate FloatingObject construction down the road as well.

> Source/WebCore/rendering/RenderBlock.cpp:94
> +#ifndef NDEBUG
> +// These helpers are only used by the PODIntervalTree for debugging
purposes.
> +String ValueToString<int>::string(const int value)
> +{
> +    return String::number(value);
> +}
> +
> +String ValueToString<RenderBlock::FloatingObject*>::string(const
RenderBlock::FloatingObject* floatingObject)
> +{
> +    return String::format("%p (%dx%d %dx%d)", floatingObject,
floatingObject->x(), floatingObject->y(), floatingObject->maxX(),
floatingObject->maxY());
> +}
> +#endif

Can we put these at the end of the file along with the renderName dumping.

> Source/WebCore/rendering/RenderBlock.cpp:1211
> +    if (m_floatingObjects)
> +	  
m_floatingObjects->setHorizontalWritingMode(isHorizontalWritingMode());

Seems like you could do this inside clearFloats.

> Source/WebCore/rendering/RenderBlock.cpp:3419
>	   floatingObject->setIsPlaced();
> +	   m_floatingObjects->addPlacedObject(floatingObject);

You could move setIsPlaced inside addPlacedObject.

> Source/WebCore/rendering/RenderBlock.cpp:3666
>	   floatingObject->setIsPlaced(true);
> +	   m_floatingObjects->addPlacedObject(floatingObject);

Again, you could move setIsPlaced inside addPlacedObject.