investigate removing members from svgelements

RESOLVED WORKSFORME

Status

()

Core
SVG
RESOLVED WORKSFORME
14 years ago
7 years ago

People

(Reporter: sicking, Assigned: Alex Fritze)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Currently most svg-elements has members for all DOM properties, i.e. circle has
mR, mCx and mCy. It seems to me that since these are stored in mAttributes
anyway we might as well just store them there.

NS_IMETHODIMP nsSVGCircleElement::GetCx(nsIDOMSVGAnimatedLength * *aCx)
{
  *aCx = mCx;
  NS_IF_ADDREF(*aCx);
  return NS_OK;
}

would become something like:
NS_IMETHODIMP nsSVGCircleElement::GetCx(nsIDOMSVGAnimatedLength * *aCx)
{
  return mAttributes->GetAnimLengthAttr(nsSVGAtoms::cx, aCx);
}

where GetAnimLengthAttr would serach both mapped and 'normal' attributes.

While we're at it we could also change the code that munges these variables. So
The code in Init would go from:

  {
    nsCOMPtr<nsISVGLength> length;
    rv = NS_NewSVGLength(getter_AddRefs(length), 0.0f);
    NS_ENSURE_SUCCESS(rv,rv);
    rv = NS_NewSVGAnimatedLength(getter_AddRefs(mCx), length);
    NS_ENSURE_SUCCESS(rv,rv);
    rv = mAttributes->AddMappedSVGValue(nsSVGAtoms::cx, mCx);
    NS_ENSURE_SUCCESS(rv,rv);
  }

to:

  mAttributes->InitMappedAnimLengthAttr(nsSVGAtoms::cx, 0.0f);

And ParentChainChanged would go from:

  {
    nsCOMPtr<nsIDOMSVGLength> dom_length;
    mCx->GetBaseVal(getter_AddRefs(dom_length));
    nsCOMPtr<nsISVGLength> length = do_QueryInterface(dom_length);
    NS_ASSERTION(length, "svg length missing interface");

    nsCOMPtr<nsIDOMSVGRect> vp_dom;
    svg_elem->GetViewport(getter_AddRefs(vp_dom));
    nsCOMPtr<nsISVGViewportRect> vp = do_QueryInterface(vp_dom);
    nsCOMPtr<nsISVGViewportAxis> ctx;
    vp->GetXAxis(getter_AddRefs(ctx));
    
    length->SetContext(ctx);
  }

to:

  mAttributes->SetXAxisContextForLengthAttr(nsSVGAtoms::cx);
Or maybe
  mAttributes->SetContextForLengthAttr(nsSVGAtoms::cx,
                                       nsISVGViewportRect::GetXAxis);

And the code in the dtor could go away compleatly.

Alex: what do you think? Would this work? Or would it wreck some plans you have
for future changes?
Seems like this code could really benefit from some deCOMtamination. Relying on
nsIDOMSVG* for internals (i.e. SVG layout) seems far from optimal with all the
additional refcounting, QI'ing n' what not.
Actually, i'm not sure that's a good idea (and definitly something that would be
a separate bug). Not relying on the nsIDOMSVG* interfaces would certainly add
speed, but it would probably also add codesize (since we'd need to implement
internal interfaces) and possibly also heapbloat (due to more vtable pointers).

There is definetly stuff that could be done to the SVG code, but i'm not sure
deCOMtamination is one of them. At least not now when speed isn't a major focus.
deCOMtamination and the use of tearoffs for the DOM interfaces could reduce
codesize since callers would need less code by not needing to deal with nsIDOM*
interfaces. DOM performance would drop a bit, but I seriously don't see that
ever being a problem with SVG. Layout performance could be critical if people
get serious about using SVG.
Another thing that might be good to do is to rather then having a separate
Init() function for svg-elements just override the Init(nsINodeInfo*) function
in nsGenericElement. That way all the NS_NewSVGXXXElement functions would only
have to call one Init function.
(Assignee)

Comment 5

14 years ago
(In reply to comment #0)
> Currently most svg-elements has members for all DOM properties, i.e. circle 
> has
> mR, mCx and mCy. It seems to me that since these are stored in mAttributes
> anyway we might as well just store them there.
[...]
> Alex: what do you think? Would this work? Or would it wreck some plans you have
> for future changes?

Yeah, this would work, but AFAICS you are just moving code from
nsSVGCircleElement to nsSVGAttributes and trading attribute array searches
against 3 pointers in nsSVGCircleElement. Whether the reduced memory footprint
translates into a performance improvement probably depends on your use-case.
(Assignee)

Comment 6

14 years ago
(In reply to comment #3)
> deCOMtamination and the use of tearoffs for the DOM interfaces could reduce
> codesize since callers would need less code by not needing to deal with 
> nsIDOM*
> interfaces. 

Hmm - I'm against any hasty deCOMtamination of the SVG code. I think while the
SVG is littered with XPCOM interfaces, the situation is different to that of
other parts of Mozilla. Most importantly, the SVG DOM interfaces are very well
documented and standardized. I think this more than outweighs the potential
performance gains that might be achieved by switching to custom/non-XPCOM
interfaces, at least at this relatively immature stage in development. 
At the moment, new developers should find it relatively easy to find their way
round the SVG code just by consulting the SVG specs. Once you introduce custom
interfaces and banish the SVG DOM into tearoffs this transparency is lost. Also
you then have to write/test/maintain the redundant external and internal APIs,
which is probably twice the work.

> DOM performance would drop a bit, but I seriously don't see that
> ever being a problem with SVG. 
> Layout performance could be critical if people
> get serious about using SVG.

I think SVG DOM usage will be more widespread than you acknowledge, especially 
for Mozilla-as-a-platform type applications. Layout performance is not
necessarily the bottleneck, especially once hardware-accelerated backends become
available. In my opinion, by making the SVG DOM a second-class citizen we'd be
optimizing against uncertain assertions. 
Please lets take the deCOMtamination discussion to another bug, this bug is
about some very specific changes that has nothing to do with deCOMtamination.
See comment 0.

Alex: Sorry, should have stated what the purpose of my suggested changes were.
There has been some concern expressed about the size of the svgcode. I.e. when
we turn on svg by default we don't want the download size to increase more then
needed. Also, if we can keep the binarysize small there's a better chance being
able to produce minimo builds that support svg is bigger.

So the reason i'm looking into the described changes is simply to reduce
binarysize. Though IMHO it's also nice to get rid of sourecode. For these
specific changes i'm not aiming to win any performance, only making a better
case for turning on svg by default.
(Assignee)

Comment 8

14 years ago
(In reply to comment #7)
> Alex: Sorry, should have stated what the purpose of my suggested changes were.
> There has been some concern expressed about the size of the svgcode. I.e. when
> we turn on svg by default we don't want the download size to increase more then
> needed. Also, if we can keep the binarysize small there's a better chance being
> able to produce minimo builds that support svg is bigger.
> 
> So the reason i'm looking into the described changes is simply to reduce
> binarysize. Though IMHO it's also nice to get rid of sourecode. For these
> specific changes i'm not aiming to win any performance, only making a better
> case for turning on svg by default.

Sorry, I was being unclear here. I think it is a good idea to factor the
initialization code into some baseclass - I'm just not sure it should be the
attribute array. Instead of nsAttrAndChildArray::InitMappedAnimLengthAttr() we
could have nsSVGElement::InitMappedAnimLengthAttr() and keep the mR, mCx, mCy
pointers in nsSVGCircleElement. While this would mean an extra 12bytes/instance
it wouldn't increase the download size and we wouldn't need any lookups when the
the r,cx,cy properties are being accessed. So you could say that the lookups are
negligible, but aren't 12bytes/instance equally negligable? I just don't know...
I suppose the best solution would be to find some way of obtaining static
compile-type indices into the attribute array.
QA Contact: general
The code changed over the years towards this model.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.