Bug 10745 – SVGLength object needs some weight loss

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

SVG layout needs to move out of DOM and into rendering tree during layout() call

Right now SVG does all of it's viewport-releative length calculations (percentages) during creation and "update" (notifyAttributeChanged) time.  This is wrong for a host of reasons.  Mostly, it's just inefficient.

In order to correct this, several things need to be done.
1.  SVGLength needs to lose it's m_context pointer.
2.  SVGLength::value() should be passed a viewportElement pointer
3.  toPathData() and other calculations related to layout should be moved from creation/update time to layout() time.  (This means no newRenderPath->setPath(toPathData()) at creation time.)
4.  DOM elements should have a needsLayoutOnViewportChange() function which checks to see if any of the values (SVGLengths) on the element are viewport relative (percentage values).  This could allow us to avoid unnecessary layouts on those elements.  (We should probably talk to hyatt a bit more about this flag.)
I think that's it.  There are probably other little tweaks I've forgotten however.  This isn't super hard, but will take a little while to get right.

Implementing this will be a huge win for speed.

[Eric Seidel (no email)]

Blocks do have logic like this to prevent laying out their kids when not necessary.  layoutBlock has a relayoutChildren bool which controls if the kids get laid out.  Normally false is passed for this bool and the kids are only re-laid-out if the width changed.  in the case of SVG this would be width or height.  In our case, we could also check each kid to make sure it had percentage-relative values before calling layout.  That might be an unnecessary optimization at first however.

[Eric Seidel (no email)]

Turns out we already have bug 9346 which covers the optimized re-layout of children with percentage values.

[Eric Seidel (no email)]

As Rob points out, SVGLength should be gutted during this process.  SVGLengths are currently 28 bytes in size!  Compare that with 4 bytes for the Length class.  We have bajillions of these things floating around the SVG DOM.  Removing at least the m_context and m_viewportElement pointers while moving SVGLength from the DOM to the rendering tree will help SVG performance substantially.

[Eric Seidel (no email)]

Layout is already handled correctly.  I'm repurposing this bug to cover removing more bytes from SVGLength.

[Nikolas Zimmermann]

Created attachment 22317 [details]
Initial patch

[Nikolas Zimmermann]

Created attachment 22318 [details]
Updated patch

Fix some typos, and a little problem with a contextElement() function.

[Eric Seidel (no email)]

Comment on attachment 22318 [details]
Updated patch

Looks good.

Unfortunately if these are allocated on the heap this won't actually save memory (due to 16 byte alignment), however if SVGLength is ever used as a member, the owning object will shrink by 4 bytes. :)

Looks good though.

Could you please explain the results of:

$getterContentHead = "IMPL->value(0 /* FIXME */";

Perhaps you could even add a failing test for these two /* FIXME */ cases?

[Nikolas Zimmermann]

(In reply to comment #7)
> (From update of attachment 22318 [details] [edit])
> Looks good.
Thanks, this should have been done a long time ago.

> Unfortunately if these are allocated on the heap this won't actually save
> memory (due to 16 byte alignment), however if SVGLength is ever used as a
> member, the owning object will shrink by 4 bytes. :)
Wait, all SVGLength objects live on the stack, since ages.. just wrapped in SynchronizableTypeWrapper<> nowadays. So it's a benefit.

> Looks good though.
> 
> Could you please explain the results of:
> 
> $getterContentHead = "IMPL->value(0 /* FIXME */";
> 
> Perhaps you could even add a failing test for these two /* FIXME */ cases?
Test cases for ObjC? The ObjC bindings don't support custom code, and we don't have sth. like "animated tear-off" objects offering the desired SVG DOM functionality, unlike JS has.

Thanks for the quick review!
Niko

[Nikolas Zimmermann]

Landed in r35204.