540090 - SMIL: Replace most calls to "doc->GetAnimationController()" with a method that avoids lazy initialization

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Smaug just pointed out to me that Shark was showing some nsSMILAnimationController activity in a Firefox session that had no SMIL animations at all.

We try to avoid this by making nsDocument::GetAnimationController lazily initialize the controller -- however, we have places that call GetAnimationController even when there aren't any animations.
 e.g. nsPresContext::SetSMILAnimations 
     nsSVGElement::FlushAnimations()

For these cases, we probably want a different method that skips the lazy initialization.  (e.g. HasAnimationController?)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix

Comment 2

[Daniel Holbert [:dholbert]]

There are four calls to GetAnimationController that this patch doesn't touch.  I list them below, with explanations for why these calls should remain unchanged (with lazy initialization).

{
>/content/svg/content/src/nsSVGAnimationElement.cpp
>    * line 275 -- nsSMILAnimationController *controller = aDocument->GetAnimationController();
>    * line 304 -- nsSMILAnimationController *controller = doc->GetAnimationController();
}

The above calls are for an <animate> element to register/unregister itself with the controller.  If we're in that situation, we're clearly doing SMIL animation, so it makes sense to go ahead with the lazy initialization.

{
>/content/svg/content/src/nsSVGSVGElement.cpp (View Hg log or Hg annotations)
>    * line 567 -- nsSMILAnimationController* smilController = doc->GetAnimationController();
>    * line 1020 -- smilController = aDocument->GetAnimationController();
}

The first call there is within a SMIL-related API method, SetCurrentTime, which needs an animation controller in order to work.

The second call is within nsSVGSVGElement::BindToTree, which just uses GetAnimationController() as a proxy for whether SMIL is enabled for the current document (i.e. it checks whether the pref is on, and whether this is a data-document.)

That last check (in nsSVGSVGElement::BindToTree) could probably be improved to avoid the unnecessary lazy-initialization -- perhaps with a method like nsIDocument::SupportsAnimation (which would be a one-liner that encompasses the checks at the beginning of GetAnimationController()).  I'll look into doing that in a separate patch.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to comment #2)
> The second call is within nsSVGSVGElement::BindToTree, which just uses
[...]
> 
> That last check (in nsSVGSVGElement::BindToTree) could probably be improved to
> avoid the unnecessary lazy-initialization [...]
> I'll look into doing that in a separate patch.

Just kidding -- that GetAnimationController() call is indeed necessary, I just didn't scroll down enough to see why.  Lower down in nsSVGSVGElement::BindToTree, we associate our animation controller with our TimeContainer, and we kick off the "document timeline".  This requires an animation controller. (We could still optimize this a bit by throttling the sample-frequency down to 0 until we actually get some animations registered... but that's a separate issue.)

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can we move mAnimationController up to nsIDocument and make HasAnimationController inline?

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v2

(In reply to comment #4)
> Can we move mAnimationController up to nsIDocument and make
> HasAnimationController inline?

Sure -- changed in this version (with some fixes for a result #include cycle).

Testing this, & then I'll re-request review.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: fix v2a

I ran this locally through SMIL reftests, and the SVG & SMIL mochitests, and it passed 'em all.

The nsIDocument uuid *just* changed out from under me on mozilla-central, so I fixed that in this new version (v2a).  :)

Comment 7

[Daniel Holbert [:dholbert]]

Oh, I suppose I need to rev NS_ISMILANIMATIONELEMENT_IID too.  I'll make sure to do that before checking in.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why did GetAnimAttr and HasAnimAttr have to move out of line?

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 429853 [details] [diff] [review]
fix v2a

>diff --git a/content/smil/nsISMILAnimationElement.h b/content/smil/nsISMILAnimationElement.h
>-  PRBool GetAnimAttr(nsIAtom* aAttName, nsAString &aResult) const
>-  {
>-    return Content().GetAttr(kNameSpaceID_None, aAttName, aResult);
>-  }
>+  virtual PRBool GetAnimAttr(nsIAtom* aAttName, nsAString& aResult) const = 0;
> 
>   /*
>    * Check for the presence of an attribute in the global namespace.
>    */
>-  PRBool HasAnimAttr(nsIAtom* aAttName) const
>-  {
>-    return Content().HasAttr(kNameSpaceID_None, aAttName);
>-  }
>+  virtual PRBool HasAnimAttr(nsIAtom* aAttName) const = 0;

FWIW: This quoted change is to keep us from directly using any "nsIContent" values in this header file, so that I can remove the "#include nsIContent.h" from this file. (and break an #include-cycle)

This change might appear to be (marginally) bad for performance, because I'm turning two non-virtual methods into virtual methods.  However, the old non-virtual versions made **calls** to a virtual method -- "Content()". So we haven't actually lost anything.

Comment 10

[Daniel Holbert [:dholbert]]

pushed: http://hg.mozilla.org/mozilla-central/rev/65d602fba99f