635511 - Improve performance getting parent SVG elements

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

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.

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

This is going to bitrot the patch in bug 484966.  :(

Comment 3

[Daniel Holbert [:dholbert]]

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!

Comment 4

[Jonathan Watt [: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(), ...).

Comment 5

[Jonathan Watt [:jwatt]]

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?

Comment 6

[Jonathan Watt [:jwatt]]

s/without/most likely without/

Comment 7

[Robert Longson [:longsonr]]

Attachment: address review comments

Comment 8

[Robert Longson [:longsonr]]

(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!

Comment 9

[Boris Zbarsky [:bzbarsky]]

> is bad as it will evaluate its arguments twice!

No, it won't.  I suggest you look at the impl of NS_IF_ADDREF.... ;)

Comment 10

[Jonathan Watt [:jwatt]]

(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 11

[Jonathan Watt [:jwatt]]

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.)

Comment 12

[Robert Longson [:longsonr]]

Attachment: hg changeset patch

Comment 13

[Robert Longson [:longsonr]]

backout strategy - backout patch.

Comment 14

[Daniel Holbert [:dholbert]]

Landed: http://hg.mozilla.org/mozilla-central/rev/23f36fd8b36e