553075 - SVG/SMIL: Don't register for periodic samples until we have some SMIL animations in our document

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

As described in bug 541588 comment 27, we could get a perf win by not bothering to register for nsRefreshDriver callbacks until we actually have some SMIL animations in our document.

There's a minor (& workaroundable) downside, in bug 541588 comment #27:
> This would have the slightly bogus effect of making
> document.documentElement.getCurrentTime() always return the same value (0)
> until animations were added (or until the user does a manual setCurrentTime),
> at which point the current-time would jump up-to-date.  I think that's legal
> (if unexpected) behavior, though, with the whole "SMIL doesn't guarantee any
> particular framerate" thing.  And we could work around it, too, by making
> getCurrentTime update our time-counter if we have zero animations.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix: add flag 'mDeferredStartSampling'

This patch adds a flag 'mDeferredStartSampling' to the animation controller.  Whenever we would start sampling but have no animations registered, this instead sets the flag, which gets checked if & when we finally do register an animation.

I ran this through SVG & SMIL mochitests, as well as SMIL reftests, using a local debug build, with no issues.  I'm running it on tryserver right now.

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 433254 [details] [diff] [review]
fix: add flag 'mDeferredStartSampling'

Passed tryserver.  Requesting review.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 433254 [details] [diff] [review]
fix: add flag 'mDeferredStartSampling'

Canceling review -- I need to rebase this now that Bug 541588 is backed out. (I intend to land this before re-landing Bug 541588).

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix: add flag 'mDeferredStartSampling' (rebased)

I've actually now split bug 541588's patch into two subpatches, as described in bug 541588 comment 33. (and this patch layers on top of the first subpatch there)

This version is exactly the same as the previously-posted version, except that it no longer removes the Notify() method declaration (that change is now in bug 541588 where it belongs), and I made one bitrot-fix in contextual code.

Comment 5

[Daniel Holbert [:dholbert]]

Landed: http://hg.mozilla.org/mozilla-central/rev/665b48fbfd28

Comment 6

[Daniel Holbert [:dholbert]]

The m.d.tree-management bot thinks this patch caused a 1.01% slowdown in DHTML and a 1.21% slowdown in SVG, both on Windows 7:
http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/1ba3bf61d7f6b248

I think it's bogus, though... I put breakpoints in nsSMILAnimationController::Sample and StartSampling, and verified that, with current mozilla-central (including this patch)...
 - ...the "Sample()" breakpoint is hit exactly once for a simple SVG document with no animations
 - ...neither breakpoint is hit by the Talos DHTML pageset (which doesn't even include any SVG content)

Given the above and how minimal this patch is, I don't see how it could have caused a slowdown in either of these tests.  I'm guessing it's a random spike, or perhaps something else happened specifically on the Windows 7 boxes...  Anyway, as I noted in the m.d.tree-management thread, I can back out if this regression sticks, just to be sure.

Comment 7

[Daniel Holbert [:dholbert]]

I backed this out to test whether that makes the SVG/DHTML regressions (from comment 6) go away: http://hg.mozilla.org/mozilla-central/rev/09b395158b41

Comment 8

[Daniel Holbert [:dholbert]]

The backout didn't affect the Win7 stats.

However, the backout purportedly caused a 1.24% SVG improvement on WinXP:
 http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/f5f5f24757b2dc85
...though the initial landing didn't affect WinXP, so I'm not sure this is real.

Re-landed, and we'll see which numbers it affects this time. :) (hopefully none)
http://hg.mozilla.org/mozilla-central/rev/1d16de84e49c

Comment 9

[Daniel Holbert [:dholbert]]

No perf issues reported from this landing earlier today -- I think we're good!