Bug 90715 – Improve performance of RenderInline::absoluteQuads for deeply nested inlines

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Kiran Muppala]

Performance of RenderInline::absoluteQuads can be quite poor when the documents has deeply nested inline elements.  This is primarily because the method RenderInline::localToAbsoluteQuad, which is called for each child boundingBox to me mapped, walks the render tree to the root each and every
time to determine the transformation.

A better approach is to cache to transformation from local to absolute coordinates once using a RenderGeometryMap and then call RenderGeometryMap::absoluteRect for each child rectangle.  This avoids the traversal to the root for each call to map the coordinates.

[Kiran Muppala]

<rdar://problem/11791199>

[Kiran Muppala]

Created attachment 151127 [details]
Patch

[Maciej Stachowiak]

Comment on attachment 151127 [details]
Patch

r=me

[David Kilzer (:ddkilzer)]

Comment on attachment 151127 [details]
Patch

Clearing flags on attachment: 151127

Committed r122191: <http://trac.webkit.org/changeset/122191>

[David Kilzer (:ddkilzer)]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Comment on attachment 151127 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151127&action=review

I'm a bit confused by where there isn't a m_geometryMap.push for each RenderInline that you visit?

> Source/WebCore/rendering/RenderInline.cpp:653
> +        , m_geometryMap()

This doesn't need to be initialized explicitly.

> Source/WebCore/rendering/RenderInline.cpp:665
> -        m_quads.append(m_renderer->localToAbsoluteQuad(rect));
> +        m_quads.append(m_geometryMap.absoluteRect(rect));

What if you never called m_geometryMap.pushMappingsToAncestor() in the ctor because you didn't find the root?

[Kiran Muppala]

(In reply to comment #6)
> (From update of attachment 151127 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151127&action=review
> 
> I'm a bit confused by where there isn't a m_geometryMap.push for each RenderInline that you visit?
>

On closer inspection of the current code, i found that all the rectangles being mapped are using the mapping from the upper most inline object on which absoluteQuads method was called.  There was no change to the state of AbsoluteQuadsGeneratorContext prior to or after the recursive call in generateCulledLineBoxRects.  So, I did not add a push and pop to the geometry map either.
 
> > Source/WebCore/rendering/RenderInline.cpp:653
> > +        , m_geometryMap()
> 
> This doesn't need to be initialized explicitly.
> 

Agreed.  I had incorrectly assumed the convention is to explicitly initialize all members.

> > Source/WebCore/rendering/RenderInline.cpp:665
> > -        m_quads.append(m_renderer->localToAbsoluteQuad(rect));
> > +        m_quads.append(m_geometryMap.absoluteRect(rect));
> 
> What if you never called m_geometryMap.pushMappingsToAncestor() in the ctor because you didn't find the root?

Then the local coordinates will be returned unchanged.  This behavior is inherited from a default constructed geometry map and I think it is appropriate.

[Simon Fraser (smfr)]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 151127 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=151127&action=review
> > 
> > I'm a bit confused by where there isn't a m_geometryMap.push for each RenderInline that you visit?
> >
> 
> On closer inspection of the current code, i found that all the rectangles being mapped are using the mapping from the upper most inline object on which absoluteQuads method was called.  There was no change to the state of AbsoluteQuadsGeneratorContext prior to or after the recursive call in generateCulledLineBoxRects.  So, I did not add a push and pop to the geometry map either.

Couldn't you have just cached this offset, then, and not used the geometry map at all?

[Kiran Muppala]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (From update of attachment 151127 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=151127&action=review
> > > 
> > > I'm a bit confused by where there isn't a m_geometryMap.push for each RenderInline that you visit?
> > >
> > 
> > On closer inspection of the current code, i found that all the rectangles being mapped are using the mapping from the upper most inline object on which absoluteQuads method was called.  There was no change to the state of AbsoluteQuadsGeneratorContext prior to or after the recursive call in generateCulledLineBoxRects.  So, I did not add a push and pop to the geometry map either.
> 
> Couldn't you have just cached this offset, then, and not used the geometry map at all?

RenderObject::localToContainerQuad took a local quad as argument and the comments indicated that the transform could depend on the center point of the local quad.  That lead me to conclude the offset could depend on the actual quad being mapped.  So, I chose geometry map since it is intelligent to use offset where possible and fall back to renderers otherwise.