[Webkit-unassigned] [Bug 65668] Optimize floating elements lookup

[bugzilla-daemon at webkit.org]

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


Dave Hyatt <hyatt at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #103083|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #13 from Dave Hyatt <hyatt at apple.com>  2011-08-05 12:13:37 PST ---
(From update of attachment 103083)
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.

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