[webkit-reviews] review denied: [Bug 36564] Performance regression for setting content of <text> in SVG : [Attachment 55314] Attempted patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Steven Lai
<s3lance at hotmail.com>'s request for review:
Bug 36564: Performance regression for setting content of <text> in SVG
https://bugs.webkit.org/show_bug.cgi?id=36564

Attachment 55314: Attempted patch
https://bugs.webkit.org/attachment.cgi?id=55314&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Hi Steven,

patch looks partly good! In fact I wanted to try a similar approach, though
some comments:
The best approach would be to actually _recalculate_ the repaintRect in
layout() - through a helper function like updateCachedBoundaries() - what
RenderPath is doing.
The reason for that is that you can inline repaintRectInLocalCoordinates()
which should give an even more performance improvement, as the function can
just return the m_cachedLocalRepaintRect.

Regarding Dirks concerns about when layout() is called:
Indeed it wasn't possible to cache these rects some weeks ago, when we still
had the SVGResource vs. RenderSVGResource confusion. In case Steven doesn't
know we recently converted all "resources"
(masker/clipper/gradients/patterns/markers) into the render tree to avoid a
special concept SVGResource/SVGPaintServer, dating back to old ksvg2 times in
2004. The issue was that relayouting wasn't handled properly. Nowadays the
resources inform all clients when they have changed (eg. changing
patternTransform attribute of a <pattern> notifies all clients to relayout).
Furthermore the clients invalidate their resources, when they have changed (eg.
rect changed size, tell it's RenderSVGResourcePattern, to update the cached
data, as example..). Until ~ 2 weeks ago, RenderPath did NOT know why a layout
occoured: path data changed / transform changed / boundaries changed? This is
all handled properly now, and I think we're ready to move the caching up to the
next level: the RenderContainer.

Some IMHO this approach should work. Can you try implementing the suggestion
above, and then run "run-webkit-tests --tolerance 0 -p svg" and watch careful
if anything changes. If not I'm happy to r+ your patch!
(Note: You may see some "noise", tests failing with 0.01% difference, because
of CoreGraphics differences across the Mac platforms, Leopard/SnowLeopard,
etc..)

.