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.
<rdar://problem/11791199>
Created attachment 151127 [details] Patch
Comment on attachment 151127 [details] Patch r=me
Comment on attachment 151127 [details] Patch Clearing flags on attachment: 151127 Committed r122191: <http://trac.webkit.org/changeset/122191>
All reviewed patches have been landed. Closing bug.
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?
(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.
(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?
(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.