Eyes Above The Waves

Yesterday I ran into some really nasty, bloaty code. This isn't particularly unusual, but this time I was bothered by the fact that this code was mostly written after a lot of people started working hard to de-bloat core parts of Gecko. This wasn't the first time I noticed problems in this code either, but for various reasons I felt particularly bothered yesterday. So I'm taking the opportunity to make something positive out of this by letting off some steam and exposing some practices that absolutely must be stamped out.

I have some examples from the SVG code. I do not intend to defame our SVG contributors, without whom we simply wouldn't have SVG support. Yet the fact that SVG has a lot of problems suggests they needed external code review and encouragement well before now.

Let's take a look at something simple: rectangles. An interesting question to ask is what gets created for an SVG <rect> element? Here's an answer:

Type	Count	Object Size	Total Size
nsSVGRectElement	1	104	104
nsAttrAndChildArray::Impl	1	28	28
nsAttrAndChildArray::Impl	1	44	44
nsCSSStyleRule	1	28	28
nsCSSDeclaration	1	28	28
nsCSSCompressedDataBlock	1	8	8
PRUint8[]	1	8	8
nsSVGClassValue	1	24	24
nsSVGAnimatedTransformList	1	36	36
nsSVGAnimatedLength	6	36	216
nsSVGLength	6	44	264
nsSmallVoidArray::Impl	7	40	280
nsWeakReference	8	12	96
nsSVGTransformList	1	76	76
nsSVGRectFrame	1	116	116
nsSVGRendererPathGeometry	1	16	16

This is only approximate, since I computed it by hand, and it doesn't include some details like the storage for the DOM attributes that the rect would presumably have, nor does it account for the frame's style context and some other generic data (which might, however, be shared if you have a lot of similar elements). It makes some optimistic assumptions about the allocation behaviour of some of the dynamically allocating arrays. It also doesn't account for intra-object padding and inter-object overhead for the heap objects. I apologize in advance for any errors. But it's good enough to get an idea of what's going on, especially with respect to SVG-specific data.

Overall, that's a lower bound of 1372 bytes on a normal 32-bit machine. I won my bet with Boris that it wouldn't be under 1K! For a single rectangle element! Other interesting statistics to think about: that's 39 heap allocated objects, 26 of which are reference counted XPCOM objects. Many of the objects support multiple C++ interfaces, hence require multiple vtable pointers per object; in fact 320 of those 1372 bytes are for vtable pointers. On a 64-bit machine, which we can no longer consider pie in the sky, the numbers are worse as pointers grow to 8 bytes each: 2376 bytes, of which 640 bytes are vtable pointers. Eeeep!

So how did we get to here, and how can we get out of here? I'm afraid the main culprit is XPCOM and a design that leans heavily on it; the critical decision was to have every object in the SVG DOM implemented directly as an XPCOM object. One of the biggest issues is the way SVG handles coordinates. Following the SVG DOM spec, each coordinate (an SVG rect has 6, x/y/width/height plus two bonus corner-rounding values) has been made an XPCOM object, an nsSVGAnimatedLength. An nsSVGAnimatedLength has two nsSVGLength components in theory: the "base value" and an "animated value". Since we don't support animation yet (if we did, a straightforward extension would make the above numbers significantly worse) our nsSVGAnimatedLength objects have just a single nsSVGLength child object, so that's 12 XPCOM objects accounted for already.

Another big problem is that the SVG code relies on an observer pattern implemented through XPCOM. Changes to SVG values like the coordinates are propagated to objects which have explicitly attached themselves as listeners. To avoid strong-reference cycles, the listeners implement a weak-reference pattern, which is where our nsWeakReference objects come in (each of which is a ref-counted XPCOM object). As you might expect, the frame listens for changes to each of its SVG coordinates. For no reason I can see, each nsSVGAnimatedLength listens to its base value. There's 7 more XPCOM objects for their nsWeakReferences.

There's also an nsSVGAnimatedTransformList for the rect, just in case it has a "transform" attribute I suppose, with a child nsSVGTransformList that it listens to. That's 3 XPCOM objects for those two plus the listener's nsWeakReference. The other XPCOM objects are the content element, the nsCSSStyleRule to hold a CSS rule expressing the style impact of any presentational attributes that might be on a <rect>, the nsSVGClassValue which seems to hold the element's class in case you want to animate it (!), and an nsSVGRendererPathGeometry which gets attached to the frame. I'm not sure why this last one needs to be a separate object.

Clearly it's imperative to simplify the coordinates, especially animated coordinates, so they're not XPCOM objects. I actually have a suggestion for how to do this that's been languishing for a while. It would shrink an animated coordinate down to 8 bytes for the common case where the value is not animated (compared to 92 bytes currently). The DOM interfaces would be implemented as "tearoffs" so we only allocate them if they're used.

Another imperative (actually a prerequisite for the above) is to eliminate the observer pattern and the use of weak references. We do not need to support an arbitrary graph of notifications in almost all cases; storing the explicit dependence graph is hugely wasteful. Attribute changes, style changes and DOM method calls already notify or are implemented by the content element, which can directly notify its frame. This can eliminate a huge amount of code and data. If we just do these two items I think we'll have made a huge step getting the code back under control.

While studying this code I discovered some interesting additional problems which I'd better mention before I forget. They may prove instructive.

• A CSS style rule is created for every SVG element that might have presentational attributes, even if there aren't any on the element. This is just silly and should be easy to fix (as a comment in the code mentions!).
  
• SVG values are expected to have zero or one observers, so nsSVGValue uses an nsSmallVoidArray to represent the observer list which is optimized for the 0-1 case. This is a pointer-sized field that can either store a direct pointer to an observer, or a heap-allocated array for the hopefully rare case of multiple observers. But it turns out the rectangle's six nsSVGLength values are observed by both the frame and its containing nsSVGAnimatedLength! So the common case is to have 2 elements, and each value has to heap-allocate an nsSmallVoidArray::Impl, and happens to allocate one with a default size of 8 pointer elements (plus overhead). nsSVGTransformList suffers the same fate. Welcome to 7 heap allocations and 280 bytes.
  
• nsSVGTransformList contains an nsAutoVoidArray member for its list of transforms. This array type is meant for use on the stack, and lets you avoid a heap allocation if the maximum count required is no more than 8 elements. Since most SVG elements don't have any transforms, we burn 8 pointer elements plus overhead for nothing most of the time. This is actually a good place to use nsSmallVoidArray. (But why can't nsSVGRectElement::mTransforms just be null if there are no transforms? Beats me.) Generally speaking one should not use nsAutoVoidArray (or similar classes such as nsAuto(C)String) as object fields.
  
• Not actually seen in this example, but seen in code nearby: some SVG classes contain PRBool fields. PRBool is a 4-byte type and should never be used in objects. Space could be saved just by making these fields PRPackedBool (or even PRPackedBool mField:1, for one bit!) and rearranging the fields to avoid padding.
  
• There are other things I could complain about, like the presence of "mFillGradient" and "mStrokeGradient" in every single rectangle frame regardless of whether the frame actually uses gradients, but those are small fry compared to the above.
  

I haven't said much about code size yet. Just constructing and maintaining the big graph of objects takes considerable code. Much of it is boilerplate code
repeated over and over with little or no sharing. There is other evidence of copy-paste coding. It's been said so many times, but I'll say it again: copy-paste coding is evil. It bloats source and object code, breeds inconsistency, discourages the creation of useful mental abstractions, and will inevitably make you fix the same bugs many times.

An overarching lesson is that XPCOM is a disease. It tempts people who should know better to create systems which are nasty, large, and inevitably, slow. It is a contagious disease; people acquire it by being exposed to infected code. Developers can be immunised, as many of us have, mostly by suffering through a bout of it. Perhaps diatribes like mine can help immunise others with less suffering. XPCOM does have its uses --- as a component mechanism in situations where one absolutely requires dynamic component loading and binding, and where we need to cross language boundaries --- but wherever it spreads beyond that, it must be stamped out.