Closed Bug 635511 Opened 9 years ago Closed 9 years ago

Improve performance getting parent SVG elements

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: longsonr, Assigned: longsonr)

Details

(Keywords: perf)

Attachments

(1 file, 2 obsolete files)

GetCtx uses the DOM GetOwnerSVGElement. It should be the other way round, we should have a fast non COM internal function and have GetOwnerSVGElement call that.

Also we should have a method to get the outer svg element iteratively rather then recursively getting the owner svg element when we need to find it.
Assignee: nobody → longsonr
Keywords: perf
Attached patch patch (obsolete) — Splinter Review
Attachment #513767 - Flags: review?(dholbert)
This is going to bitrot the patch in bug 484966.  :(
Sorry -- I haven't gotten to this yet, and I'm going to be traveling for the next ~30 hours, so if there's any rush on this, jwatt may be a better person to ping for review right now (as I think he just returned from vacation).

Otherwise, I'm happy to take a look as soon as I can, though!
Version: unspecified → Trunk
Attachment #513767 - Flags: review?(jwatt)
Comment on attachment 513767 [details] [diff] [review]
patch

Regarding GetCtx(PRBool *aIsInner, PRBool *aOK), it seems like aOK could go away. There's only one caller that cares (nsSVGElement::GetOwnerSVGElement) and it could just do:

  if (*aOwnerSVGElement || Tag() == nsGkAtoms::svg) {
    return NS_OK;
  }
  return NS_ERROR_FAILURE;

I'm also not all that comfortable with the aIsInner argument. The main thing is that it seems like the natural assumption for anyone reading calling code would be to assume that the argument in to tell whether the ctx returned is an inner-<svg>. Certainly I had to adjust the mental model of the patch after the first skim. A second (fixable) point is that the value that this out param is set to will not necessarily match the value returned by nsSVGElement::IsInner, which seems like something we should avoid.

I need to think about this bit some more, but ultimately it seems like what the callers really want to know is whether their element is as element in a CSS layout, or an element in a coordinate layout. Maybe a method called IsCSSBox would be more appropriate...? 


Just kill nsSVGUtils::GetFarthestViewportElement since the callers should only need:

 NS_IF_ADDREF(*aFarthestViewportElement = nsSVGUtils::GetOuterSVGElement(this));


I don't think it makes sense to call nsSVGUtils::GetOuterSVGElement on non-SVG elements, so add an NS_ABORT_IF_FALSE checking the namespace in there?


-   // We must not be the outermost SVG element, try to find it

Please restore this comment. Or better yet put in an NS_ABORT_IF_FALSE(!IsInner(), ...).
Attachment #513767 - Flags: review?(jwatt) → review-
Yeah. If the aIsInner perf optimization actually improved the perf of real content I'd be happy to have it, but I think it just muddies the code without actually making a measurable difference. I'll happily r+ a patch that just removes it in favor of IsInner(). Would you be okay with that, Robert?
s/without/most likely without/
Attached patch address review comments (obsolete) — Splinter Review
Attachment #513767 - Attachment is obsolete: true
Attachment #513767 - Flags: review?(dholbert)
Attachment #514881 - Flags: review?(jwatt)
(In reply to comment #5)
> Yeah. If the aIsInner perf optimization actually improved the perf of real
> content I'd be happy to have it, but I think it just muddies the code without
> actually making a measurable difference. I'll happily r+ a patch that just
> removes it in favor of IsInner(). Would you be okay with that, Robert?

I can always take that up in a new patch :-)

BTW

NS_IF_ADDREF(*aFarthestViewportElement =
nsSVGUtils::GetOuterSVGElement(this));

is bad as it will evaluate its arguments twice!
> is bad as it will evaluate its arguments twice!

No, it won't.  I suggest you look at the impl of NS_IF_ADDREF.... ;)
(In reply to comment #8)
> I can always take that up in a new patch :-)

Yeah, I'd rather consider that separately if you think it's worth doing.
Comment on attachment 514881 [details] [diff] [review]
address review comments

r=jwatt with the following changes:

Revert the change to nsSVGAnimationElement::GetTargetElement. I'll leave it up to you to decide about the other NS_IF_ADDREF cases since I don't really mind.

Change:

 +  // we don't have a parent SVG element...

to:

 +  // we don't have an ancestor <svg> element...

Change:

 +  // We must not be the outermost SVG element,

to:

 +  // We must not be the outermost <svg> element,

and add "// invalid structure" after the |return nsnull;|

Change:

 +  * Get the outer SVG element of an nsIContent

to:

 +  * Get the outer <svg> element for the given SVG element

and change the argument to nsSVGElement* rather than nsIContent*. (Then no point in the NS_ABORT_IF_FALSE I guess.)
Attachment #514881 - Flags: review?(jwatt) → review+
Attachment #514881 - Attachment is obsolete: true
Keywords: checkin-needed
backout strategy - backout patch.
Landed: http://hg.mozilla.org/mozilla-central/rev/23f36fd8b36e
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.