Bug 10745 - SVGLength object needs some weight loss
Summary: SVGLength object needs some weight loss
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P4 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 20051
  Show dependency treegraph
 
Reported: 2006-09-06 00:50 PDT by Eric Seidel (no email)
Modified: 2008-07-16 16:37 PDT (History)
2 users (show)

See Also:


Attachments
Initial patch (84.48 KB, patch)
2008-07-16 14:19 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch (87.07 KB, patch)
2008-07-16 14:54 PDT, Nikolas Zimmermann
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2006-09-06 00:50:55 PDT
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.
Comment 1 Eric Seidel (no email) 2006-09-06 01:09:55 PDT
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.
Comment 2 Eric Seidel (no email) 2006-09-06 01:12:03 PDT
Turns out we already have bug 9346 which covers the optimized re-layout of children with percentage values.
Comment 3 Eric Seidel (no email) 2006-09-06 03:13:29 PDT
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.
Comment 4 Eric Seidel (no email) 2007-06-12 11:34:05 PDT
Layout is already handled correctly.  I'm repurposing this bug to cover removing more bytes from SVGLength.
Comment 5 Nikolas Zimmermann 2008-07-16 14:19:19 PDT
Created attachment 22317 [details]
Initial patch
Comment 6 Nikolas Zimmermann 2008-07-16 14:54:45 PDT
Created attachment 22318 [details]
Updated patch

Fix some typos, and a little problem with a contextElement() function.
Comment 7 Eric Seidel (no email) 2008-07-16 15:27:46 PDT
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?
Comment 8 Nikolas Zimmermann 2008-07-16 16:14:47 PDT
(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

Comment 9 Nikolas Zimmermann 2008-07-16 16:35:22 PDT
Landed in r35204.