SVG:Cleanup

Some suggested improvements to make the SVG code smaller, simpler, and much more efficient. See http://weblogs.mozillazine.org/roc/archives/2006/02/anatomy_of_bloat.html for some context. These are roughly in order of apparent priority.

[tor] It would be good to have a design solution for <use> (SVG:Use) in hand when doing this work, to make sure we aren't going down the wrong path.

[roc] I believe all these suggestions are compatible with the multiple frames approach, modulo the fact that the multiple frames approach requires some work to allow for multiple primary frames per element (one per independent rendering), and stuff below would have to be modified to suit.

[DONE] Eliminate dynamic observers

Figure out what the observer structure actually is, and hardcode it with notification methods. It seems to me that most notifications flow from coordinate objects to their frames. My suggestion is to have the SVG content objects notify SVG frames directly on relevant attribute changes and DOM calls. Style changes already reach the frames. When irregular notifications are required (e.g., gradients notifying their users), we can have an explicit observer list for just those objects that need it. As long as observers are limited to frames, don't use XPCOM weak references for the observer pattern. Frames are explicitly created and destroyed, and you can remove the observer relationship whenever the observer or the observee is destroyed.

[tor] The patch in bug 301628 took a first step down this route by removing most explicit use of SVG observers in the SVG frames, replacing them where needed by the AttributeChanged mechanism. How do you intend that the content objects notify the SVG frames? Via GetPrimaryFrameFor(), which I've been warned away from in the past due to it being a relatively heavyweight operation?

[roc] That patch is very good stuff. I would use GetPrimaryFrameFor for whatever's left. The first use on a given element-frame pair is slow, but after that I believe it's just a hashtable lookup. I would want to see a detailed scenario where GetPrimaryFrameFor is too slow before trying someting bloatier/more complex.

[tor] bug 301628 and follow-up patches for gradients and patterns have been checked in, removing much of the use of observers in the frame code.

[tor] bug 332162 which rewrote SVGLength removed the observer system for those from the content side.

[longsonr] all done bug 395667 bug 397159 bug 397749 bug 437448 bug 374111 are some of the bugs.

[DONE] Fix (animated) lengths

Each animated length currently (actually animated or not) requires a bundle of XPCOM objects. I have a proposal to shrink this to a single 8-byte struct in the non-animated case. I have a proposal for this on the wiki at SVG:Notification_Mechanisms which I won't repeat here. The basic idea is to store coordinate data efficiently as fields of SVG content objects. DOM access is provided by creating tearoffs holding a reference to the content object holding the coordinate and the index of the coordinate within the content object.

As part of this, instead of having frames hold references to relevant coordinates, they would get the data through content every time it is needed, by casting their content element to the appropriate concrete SVG element class.

[tor] SVGLength deCOMtaminated and otherwise reworked as bug 332162.

[DONE] Don't create unnecessary CSS rules

Only create a CSS rule for an SVG element if the SVG element actually has at least one presentational attribute ... should be easy.

[tor] We'll need guidance from a style person regarding this.

[roc] I could be wrong but I think it's really simple. See [[1]]

[tor] Done - bug 331481.

[DONE] Eliminate PRBool fields

PRPackedBool should be used for fields, and fields should be ordered to minimize padding.

[jwatt] The PRBool to PRPackedBool changes are done (see bug 328571). More work could be done to audit the code for better ordering of fields.

[DONE] Fix nsAutoVoidArray field

Use nsVoidArray or nsSmallVoidArray in fields instead. Where this occurs in nsSVGTransformList, we want an nsSmallVoidArray.

[DONE] Don't create unnecessary objects

Currently we create an nsAnimatedSVGTransformList even for elements that don't have transforms. We should at least be able to leave it null for such elements. (See below for additional work to remove the field altogether.)

[tor] SVG transform list now created lazily as part of bug 334400

Eliminate duplicated code

There is much duplicate code. Stuff that I've noticed:

1. GetCanvasTM() implementations
2. Paint() implementations duplicate code for filters, clipping etc

These simply requiring moving code to nsSVGUtils or possible an SVG frame superclass.

[tor] Paint() cleaned up as part of the SVG effects (filter, mask, clipPath, opacity) unification bug 330498.

[tor] Some GetCanvasTM() cleanup done as part of bug 334400.

[DONE] deCOMtaminate core data structures

SVG points and matrices are currently always XPCOM objects, and even C++ code goes through the DOM interfaces to manipulate them. Consequently even something as simply as transforming a point with a matrix requires a heap allocation. SVG point and matrix types (and possibly others I haven't encountered yet) should either be converted to non-virtual C++ objects (possibly just using the Thebes gfxPoint and gfxMatrix types), and a DOM tearoff system used similar to my proposal for lengths, or else can remain XPCOM DOM objects but have SVG C++ code cast them to their concrete class for efficient access.

[tor] Path data storage rewrite done for bug 334999. DOM version of the list created lazily as requested.

[longsonr] bug 435356 did most of this.

[DONE] Eliminate extra vtable pointers by removing multiply inherited interfaces

The above changes should eliminate the need for many of the interfaces on frames and content. In many other cases we could use a concrete (super)class instead of an abstract interface. In some limited cases we might have to push a set of methods into a superclass even though some of the methods are not implemented in some subclasses; this violates XPCOM/OO purity but reducing the number of vtable-pointers in each object, and reducing the number of QIs, is probably worth it.

[tor] There have been a number of checkins to remove interfaces in svg layout, replacing them with a concrete heirarchy.

[DONE] Eliminate mostly-unused fields

Consider using a property system or some other technique so that SVG content elements and frames don't have to carry references to a tranform list, filter, fill gradient, etc unless they actually have one.

Elsewhere we use slots structs for this sort of thing. That may be the way to go here if it's rare for anything at all to be used. If it's common for _something_ to be there, just not all the things at once, then the property system may be better. --Bzbarsky 16:44, 15 Feb 2006 (PST)

[tor] Filter field moved to a property as part of bug 330498.

[tor] Mask field moved to a property in bug 338060.

[tor] Gradient/Pattern moved to a property in bug 339375.

DeCOMtaminate non-DOM interfaces

Interfaces that are not exposed to script need not, and should not, follow XPCOM method guidelines; their methods should be as close to C++ style as permits.

[tor] Ongoing...

[DONE] Remove GDI+/libart renderers

Trivial code removal, followed by conversion of renderer abstract interfaces to direct usage of the cairo renderer classes, followed by conversion of all code to thebes

[tor] GDI+ and libart renderers removed, bug 330516.

[tor] Many of the renderer interfaces have been removed, leaving mainly the surface and font related ones.

[DONE] DeCOMtaminate/devirtualize cairo/Thebes renderer

Self-explanatory.

[DONE] Reconsider nsSVGCairoRegion

nsSVGCairoRegion is just a rectangle; dump it unless a reason exists to fix it.

[tor] nsSVGCairoRegion removed, bug 340451.

[tor] Build up CTM incrementally during rendering

Currently the CTM for each object being rendered is calculated by walking recursively up the tree to an nsSVGOuterSVGFrame, then combining the matrixes. This made sense for the libart and GDI+ renderers which operated in retained mode so this calculation only needed to be redone when the object changed. In the cairo immediate mode world it makes more sense to build up the transform as we walk down the tree rendering.

[roc] yeah, this sounds logical to me. I was wondering why you don't do this already but I thought there might be a good reason. Glad I was wrong :-).