Closed Bug 236244 Opened 21 years ago Closed 14 years ago

investigate removing members from svgelements

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: sicking, Assigned: alex)

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.
(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.
(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.
(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
Closed: 14 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.