562815 - SVG/SMIL: Don't always force-recompose animations of CSS properties (Slow perf w/ many frozen animations of CSS properties)

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Load URL. ( http://svg.kvalitne.cz/mousecatch/mousecatch.svg )
 2. Click to start animation.
 3. Wait until footprints start fading away.

ACTUAL RESULTS: As the footprint opacity animations become active, the demo begins to get very choppy, and CPU usage increases dramatically.  By the end of the animation, CPU usage is near 100% (and it stays there).

Basically, that demo generates a *lot* of animations, though at any given time, very few are actually interpolating. (the rest are either not-yet-started or are frozen at their final value)

We currently force-recompose all of the completed frozen animations every sample, since they're animations of CSS properties, and some CSS properties' values depend on things that could change from sample to sample (in particular, length-based properties depend on font & font-size, if they use 'em' or 'ex' units).  However, most of the time, we should be able to avoid recomposing animations on CSS properties.

The problem & a fix strategy is described in more detail in bug 533291 comment 19. (starting with the second paragraph there)

NOTE: The author of the demo, Marek Raida, was kind enough to provide an alternative version of the demo that has no opacity animations, for comparison:
   http://svg.kvalitne.cz/mousecatch/mousecatch-wo.svg
This version is much smoother and doesn't hog the CPU.

Comment 1

[Daniel Holbert [:dholbert]]

nemo pointed out to me in IRC yesterday that the latter testcase (mousecatch-wo.svg) uses up a lot of CPU even after the animation has finished playing and the testcase looks static.

I've reduced a testcase for that issue and filed it as bug 604557.

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part A: Refactor test_smilChangeAfterFrozen.xhtml v1a

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part B: Add further tests v1a

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part C: Add context-dependent flag to style animation resolution v1a

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part D: Use CSS parser to determine what values can be cached v1a

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attached now is a series of WIP patches.

Everything seems to be working as expected however, I'm not yet sure about Part D. The current approach is quite intrusive. If possible I'd like to avoid touching the CSS parser at all so I'm going to try an alternative approach to Part D to see if we can pull apart the parsed result inside nsStyleAnimation and set the context-dependent flag there.

Viewing the test URL from comment 0 I see this sort of behaviour (on a dual-core machine):

  W/o patches: CPU -> 50% during anim, then -> ~40% after it's finished
  W/  patches: CPU -> 50% during anim, then -> ~10% after it's finished

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 1: Refactor test_smilChangeAfterFrozen.xhtml v1b

This patch simplifies test_smilChangeAfterFrozen.xhtml so we can easily add further tests.

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 2a: Add further tests v1b

Add further tests

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 2b: Add even more tests v1a

And some more tests (new tests that are necessary since I will change how Part D/4 behaves).

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 3: Add context-dependent flag to style animation resolution v1b

Just some plumbing to get back extra information from nsStyleAnimation about whether the value is context-sensitive or not.

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 1: Refactor test_smilChangeAfterFrozen.xhtml v1c

Minor formatting tweaks

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 2b: Add even more tests v1b

Formatting tweaks

Comment 13

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 4: Test which values are context-sensitive v2

I've had another go at detecting which values are context-sensitive (for the sake determining which ones we can safely cache when animating). This approach isolates the code to nsStyleAnimation rather than touching the CSS parser.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 510858 [details] [diff] [review]
Part 4: Test which values are context-sensitive v2

There should be comments somewhere saying what it means for a value to be context-dependent.  (Also, you use the terms context-dependent and context-sensitive interchangeably -- maybe pick one of them?)  Do you mean "does this value result in a different computed value depending on the values of other CSS properties on the element or its ancestors?"  Or do you mean "does this value result in a different computed value depending on the values of other CSS properties on its ancestor?"  From the definition, I'd expect the first, but it looks like you're doing the second.)

The definition in nsStyleAnimation.h in patch three seems to be defining only by example.

Then there's the issue that nsRuleNode already knows how to determine both, and I'm not crazy about having this knowledge even more widely distributed than it already is.

Comment 15

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 2a: Add further tests v1c

s/context-dependent/context-sensitive/

Comment 16

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 3: Add context-sensitive flag to style animation resolution v1c

s/context-dependent/context-sensitive/

Add documentation to describe the meaning of "context-sensitive"

Comment 17

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 4: Test which values are context-sensitive v2b

s/context-dependent/content-sensitive/ and tweak documentation to fit with updates to part 3

Comment 18

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Thanks very much for the quick feedback!

(In reply to comment #14)
> There should be comments somewhere saying what it means for a value to be
> context-dependent.  (Also, you use the terms context-dependent and
> context-sensitive interchangeably -- maybe pick one of them?)  Do you mean
> "does this value result in a different computed value depending on the values
> of other CSS properties on the element or its ancestors?"  Or do you mean "does
> this value result in a different computed value depending on the values of
> other CSS properties on its ancestor?"  From the definition, I'd expect the
> first, but it looks like you're doing the second.)
> 
> The definition in nsStyleAnimation.h in patch three seems to be defining only
> by example.

Yes, that's a great idea. I did notice at one point that I was mixing those terms up and I should have fixed that already. Thanks!

I've now stuck with "context-sensitive" and added a description.

Regarding the definition, as far as this patch is concerned it doesn't matter where the change takes place. If a change of any kind on even a *descendent* element were able to influence the computed value then we would consider the input to be context-sensitive. For that reason I've adopted the first proposed definition (i.e. ancestor OR the element itself) since it is more inclusive.

> Then there's the issue that nsRuleNode already knows how to determine both, and
> I'm not crazy about having this knowledge even more widely distributed than it
> already is.

Ok, I wasn't aware of that. I'll see if we can extract the information needed from the nsRuleNode objects--any pointers would be much appreciated.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #18)
> Ok, I wasn't aware of that. I'll see if we can extract the information needed
> from the nsRuleNode objects--any pointers would be much appreciated.

It's not extractable in any useful way; it's tracked via the canStoreInRuleTree variable in the various Compute* functions.  (It's initialized based on whether there are any dependencies on the parent -- that's what the CheckSpecifiedProperties and Check*Callback code does -- and then it's modified by whether there are any dependencies on other properties on the same element -- but only if those properties are in other style structs.  We don't track dependencies on other properties in the same style struct.)

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 510856 [details] [diff] [review]
Part 1: Refactor test_smilChangeAfterFrozen.xhtml v1c

Part 1 looks good, with one caveat:

> function testCurrentColorChange()
> {
>   gCircle.setAttribute("color", "red"); // At first: currentColor=red
>-  var anim = createAnimFromTo("fill", "yellow", "currentColor");
>+  var anim = createAnimSetTo("fill", "currentColor");
> 
>   gSvg.setCurrentTime(TIME_AFTER_ANIM_END);

The seek to TIME_AFTER_ANIM_END is a bit misleading here -- the <set> element you're creating has an indefinite duration, so
 (a) there's no "time after its animation ends" for it
 (b) in fact, there aren't really any special times at all, so any seeking at all is a little confusing.

Given that  could you perhaps replace this with:
>  gSvg.setCurrentTime(0); // no-op, aside from triggering synchronous sample
or something like that?
 
(This also applies to the other chunks where you've substituted "createAnimSetTo")

Comment 21

[Daniel Holbert [:dholbert]]

Also: Continuing the quoted chunk from previous comment...
>   gSvg.setCurrentTime(TIME_AFTER_ANIM_END);
>   is(SMILUtil.getComputedStyleSimple(gCircle, "fill"), "rgb(255, 0, 0)",
>      "Checking animated fill=currentColor after anim ends");

The message there is incorrect -- now that you're using <set>, the animation hasn't ended.  Replace with something like
  "Checking that <set to="currentColor"> gets updated after 'color' is tweaked"

Comment 22

[Daniel Holbert [:dholbert]]

(er, sorry - s/after/before/ in prev comment's suggested message)

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 510857 [details] [diff] [review]
Part 2b: Add even more tests v1b

>+function testCurrentColorChangeOnFallback()
>+{
[...]
>+  var anim = createAnimSetTo("fill", "url(#missingGrad) currentColor");
[...]
>+  is(fallback, "rgb(255, 0, 0)",
>+     "Checking animated fallback fill=currentColor after anim ends");

As above, "after anim ends" is incorrect here.  Maybe replace "ends" with "takes effect"?  (That'd be fine in the chunk highlighted by Comment 21, too.)

>+  gSvg.setCurrentTime(TIME_AFTER_ANIM_END);

As above, replace with 0 (or any other fixed time-constant with a less-misleading-for-<set> variable name).

>+function testClip()
[...]
>+  var anim = document.createElementNS(SVGNS,"set");
>+  anim.setAttribute("attributeName", "clip");
>+  anim.setAttribute("to", "rect(1em 1em 1em 1em)");
>+  nestedSVG.appendChild(anim);

Could you add a brief comment noting why we aren't using 'createAnimSetTo' in this case?   (I think it's because that would automatically append <set> to gCircle, which we don't want in this case -- right?)

>+  gSvg.setCurrentTime(TIME_AFTER_ANIM_END);
>+  is(SMILUtil.getComputedStyleSimple(nestedSVG, "clip"),
>+     "rect(20px, 20px, 20px, 20px)",
>+     "Checking animated clip rect after anim ends");

See above RE TIME_AFTER_ANIM_END & "after anim ends" text.

(r=dholbert with that)

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 510885 [details] [diff] [review]
Part 2a: Add further tests v1c

Above notes on TIME_AFTER_ANIM_END & the string "anim ends" apply to all instances of those strings throughout this patch. (Except for those in "testEmExUnitChangeOnPropBase", which uses a 'by' animation (with a duration), and hence for which 'anim end' is meaningful.)

>diff --git a/content/smil/test/test_smilChangeAfterFrozen.xhtml b/content/smil/test/test_smilChangeAfterFrozen.xhtml
>   const tests =
>     [ testBaseValueChange,
[...]
>+      testEmExUnitChangeOnProp,
>+      testEmExUnitChangeOnPropBase,

The ordering of new functions in this array *almost* matches the ordering of their definitions, with one exception: testEmExUnitChangeOnPropBase -- it's the second one you added to the array, but its definition is the absolute last.  Would you mind shifting its definition upwards, to match the array-ordering?

Also: Why are these methods named "EmEx"? That initially made me expect that they'd use "ex" units at some point, but they don't. Maybe just remove the "Ex" from their names?

>+function testPercentUnitChangeOnLength()
>+{
[...]
>+  // Due to bug 627594 (SVGLength.value for percent % value lengths doesn't

s/percent %/percent/

[continuing that chunk...]:
>+  // reflect updated viewport until reflow) the following will fail.
>+  // Check that it does indeed fail so that when that bug is fixed this test
>+  // can be updated.
>+  todo_is(gCircle.cy.animVal.value, 100,
>+     "Checking animated length=100% after anim ends but before reflow");
>+  gSvg.forceRedraw();
>+  // Even after doing a reflow though we'll still fail due to bug 508206
>+  // (Relative units used in animation don't update immediately)
>+  todo_is(gCircle.cy.animVal.value, 100,
>+     "Checking animated length=100% after anim ends but before resampling");

So right now, if bug 627594 were fixed, I think this test wouldn't be able to tell -- the todo_is() statements would all behave exactly the same as they do now, because (as you note) nothing will work until we do a synchronous sample, which we do *last*.

So, it seems like it'd be helpful to add one more forced-resample, *before* the reflow, with a "todo_is" to assert that it doesn't work then, either.  Something like:

  todo_is(gCircle.cy.animVal.value, 100,
     "Checking animated length=100% after anim ends, after a resample,
     but before reflow");

>+  ok(weight > 800 && weight < 1000,
>+     "Checking animated font-weight > 800 after updating context");

AFAIK, non-"hundred"-valued numeric font-weights aren't valid - so based on your "ok()", there's only one possible value here: 900.  So, can this just be:
  ok(weight == 900, ...)
?

>+  // seem to store them in their relative form. If, however, we change the way
>+  // we store shorthand font properties in the future this will serve as
>+  // a useful regression test.

s/future/future,/

r=dholbert with the above.

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 510886 [details] [diff] [review]
Part 3: Add context-sensitive flag to style animation resolution v1c

>+++ b/layout/style/nsStyleAnimation.cpp
In documentation for "StyleWithDeclarationAdded":
>+ * @param [out] aIsContextSensitive An optional flag which will be set if
>+ *                        |aSpecifiedValue| is context-sensitive (see
>+ *                        description of this parameter in nsComputeValue).

s/nsComputeValue/nsStyleAnimation::ComputeValue/,  I think.

Aside from that, part 3 looks good -- r=dholbert

Comment 26

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 1: Refactor test_smilChangeAfterFrozen.xhtml v1d; r=dholbert

Comment 27

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 2a: Add further tests v1d; r=dholbert

Comment 28

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 2b: Add even more tests v1c; r=dholbert

Comment 29

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 3: Add context-sensitive flag to style animation resolution v1d; r=dholbert

Comment 30

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Thanks for all the review feedback Daniel. I've addressed it in these updated patches with the exception of the following:

(In reply to comment #24)
> >+  // Due to bug 627594 (SVGLength.value for percent % value lengths doesn't
...
> >+  // reflect updated viewport until reflow) the following will fail.
> >+  // Check that it does indeed fail so that when that bug is fixed this test
> >+  // can be updated.
> >+  todo_is(gCircle.cy.animVal.value, 100,
> >+     "Checking animated length=100% after anim ends but before reflow");
> >+  gSvg.forceRedraw();
> >+  // Even after doing a reflow though we'll still fail due to bug 508206
> >+  // (Relative units used in animation don't update immediately)
> >+  todo_is(gCircle.cy.animVal.value, 100,
> >+     "Checking animated length=100% after anim ends but before resampling");
> 
> So right now, if bug 627594 were fixed, I think this test wouldn't be able to
> tell -- the todo_is() statements would all behave exactly the same as they do
> now, because (as you note) nothing will work until we do a synchronous sample,
> which we do *last*.
> 
> So, it seems like it'd be helpful to add one more forced-resample, *before* the
> reflow, with a "todo_is" to assert that it doesn't work then, either. 
> Something like:
> 
>   todo_is(gCircle.cy.animVal.value, 100,
>      "Checking animated length=100% after anim ends, after a resample,
>      but before reflow");

The line before the quoted chunk is:

gSvg.setCurrentTime(0);

so we're already doing a synchronous sample there. I think if bug 627594 were fixed we'd pick it up from that previous synchronous sample. So I haven't changed that part for now.

Comment 31

[Daniel Holbert [:dholbert]]

Ah, ok -- good.

Perhaps you could move that "gSvg.setCurrentTime(0);" line downwards then, to be just before the first "todo_is", and add a comment like
 // force synchronous sample
next to it?  Right now, it looks a bit like it's part of the set-up chunk of the test, when really it's part of the "ok now we're checking things" chunk.

Comment 32

[Daniel Holbert [:dholbert]]

Comment on attachment 510887 [details] [diff] [review]
Part 4: Test which values are context-sensitive v2b

>+PRBool
>+IsContextSensitive(nsCSSProperty aProperty, nsICSSStyleRule& aStyleRule)
>+{
[...]
>+  if (nsCSSProps::IsShorthand(aProperty) ||
>+      nsCSSProps::kAnimTypeTable[aProperty] == eStyleAnimType_None)
>+    return PR_FALSE;
>@@ -1904,34 +2005,35 @@ nsStyleAnimation::ComputeValue(nsCSSProp
[...]
>+  if (nsCSSProps::IsShorthand(aProperty) ||
>+      nsCSSProps::kAnimTypeTable[aProperty] == eStyleAnimType_None) {
>+    // Just capture the specified value
>+    aComputedValue.SetUnparsedStringValue(nsString(aSpecifiedValue));
>+    return PR_TRUE;

So ComputeValue() already screens out shorthands & non-animatable properties, well before we enter "IsContextSensitive".  So IIRC, the matching clause in "IsContextSensitive()" is never visited.

I think we should either replace that clause (in IsContextSensitive) with an NS_ABORT_IF_FALSE, or else we should add a NS_NOTREACHED() to it. (just before the |return PR_FALSE|)  I think I lean towards the latter, but I'd be fine with either.

Other than that, this looks good to me, modulo dbaron's notes about nsRuleNode already having similar logic for this sort of thing.  (I don't know that code as well, though, so I can't comment on that aspect.)

So, r=dholbert, with the above note (& assuming the final patch satisfies dbaron w.r.t. logic duplication)
(sorry for the delay on this patch)

Comment 33

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 2a: Add further tests v1e; r=dholbert

(In reply to comment #31)
> Perhaps you could move that "gSvg.setCurrentTime(0);" line downwards then, to
> be just before the first "todo_is", and add a comment like
>  // force synchronous sample
> next to it?  Right now, it looks a bit like it's part of the set-up chunk of
> the test, when really it's part of the "ok now we're checking things" chunk.

Ok, fixed.

Comment 34

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 4: Test which values are context-sensitive v2c; r=dholbert

(In reply to comment #32)
> So ComputeValue() already screens out shorthands & non-animatable properties,
> well before we enter "IsContextSensitive".  So IIRC, the matching clause in
> "IsContextSensitive()" is never visited.
> 
> I think we should either replace that clause (in IsContextSensitive) with an
> NS_ABORT_IF_FALSE, or else we should add a NS_NOTREACHED() to it. (just before
> the |return PR_FALSE|)  I think I lean towards the latter, but I'd be fine with
> either.

Yep, good idea. Actually I was going to do that and that's why I moved the check in ComputeValue (I also moved it because it saves us some busy work for short-hand/non-animatable properties). In the end I left the redundant check for the sake of making IsContextSensitive more self-contained and generally useable, but that's probably not necessary.

I've gone with NS_ABORT_IF_FALSE since we really don't want to proceed if that condition is violated. nsCSSCompressedDataBlock::ValueFor which will call later will do an NS_ABORT_IF_FALSE if we have a shorthand property anyway.

> Other than that, this looks good to me, modulo dbaron's notes about nsRuleNode
> already having similar logic for this sort of thing.  (I don't know that code
> as well, though, so I can't comment on that aspect.)

Yeah, I'm currently digging through this part seeing if there's any way we can avoid the duplication but so far I can't see it without hacking nsRuleNode which I'm not confident to do.

> (sorry for the delay on this patch)

Not at all, I assumed you were waiting to see what we end up doing about the duplication since we may end up throwing this patch away if there's a better approach.

In this updated patch I've also removed the comment about nsStyleAnimation::ComputeValue and nsSMILCSSValueType::ValueFromString not touching the aIsContextSensitive out param in the case of failure since I've identified a couple of code paths where we might set that value and later fail.

Obviously it's quite easy to ensure we don't set the value in case of failure but it's extra work and no-one's relying on that behaviour so I've simply left it undefined for now.

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I think the relevant data can be exposed from nsRuleNode as follows:

 (1) in one patch, expose the types PropertyCheckData, CheckCallbackFn, StructCheckData and the global gCheckProperties as public (static, for the global) members of nsRuleNode.  (This is best done in a separate patch because it's mostly moving code, which is easiest to review when it's alone.)

 (2) In the next patch, refactor nsStyleAnimation::ComputeValue's helper function called StyleWithDeclarationAdded into two functions, one (RuleForDeclaration) that constructs and returns the style rule (as already_AddRefed<nsICSSStyleRule), and the other (StyleWithRuleAdded) that does the RuleMatched() and ResolveStyleByAddingRules stuff at the end.

 (3) In the main patch, add an _optional_ PRBool* out-parameter to nsStyleAnimation::ComputeValue, indicating whether the computed result can be cached in the rule tree.  If this PRBool* is null, ComputeValue should work as it does today (and the documentation should be clear that null is fastest).  If it's non-null, then instead of doing what it does today, it instead adds *two* style rules:  the first would be a style rule that is a C++-implemented style rule (i.e., nsIStyleRule implementation), lazily instantiated as a member of nsStyleAnimation (since caching it is good).  (See AnimValuesStyleRule in nsTransitionManager.cpp for an example of a C++-implemented style rule.)  However, this style rule would, in its MapRuleInfoInto method, use the data exposed in patch (1) to set every *unset* property on all requested structs to eCSSUnit_Initial.  Then, you'd change what's currently the StyleWithDeclarationAdded call in ComputeValue (though split by patch (2)) to be:
  nsCOMPtr<nsICSSStyleRule> declRule = RuleWithDeclarationAdded(...);
  if (aCachedInRuleTree) {

    // compute style context with two rules added (or, alternately and probably
    // slightly better though also a little riskier, only those two rules):
    //   (1) the new C++-implemented rule and
    //   (2) declRule

    // set *aCachedInRuleTree (which requires exposing a method on
    // nsStyleContext to find out if something was cached in the rule tree)

    // NOTE: If we set it to true, we need to throw the data out because
    // the fact that it's true tells us it may have been biased by the
    // 'initial' values.
  }
  if (!aCachedInRuleTree || !*aCachedInRuleTree) {

    // compute style context with only one rule added, like now

  }

(There's one potential slight issue with this, which is that there might be a case or two, where we actually avoid setting canStoreInRuleTree to false in cases when there's a dependency on another value in the same style struct.  If we do, it's probably pretty obscure... but I'm thinking mainly about the hacks in ComputeFontData.)

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #35)
> (There's one potential slight issue with this, which is that there might be a
> case or two, where we actually avoid setting canStoreInRuleTree to false in
> cases when there's a dependency on another value in the same style struct.  If
> we do, it's probably pretty obscure... but I'm thinking mainly about the hacks
> in ComputeFontData.)

(And if this is the case, we can just make the second if() unconditional, and always throw away the data from the first pass.)

Comment 37

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Thanks David for super useful help on this. I'll have a go at implementing this. However, as discussed with roc on IRC it's too late to land this for FF4 so it's low priority.

Comment 38

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 3: Add context-sensitive flag to style animation resolution v1e; r=dholbert

Just a few refactorings to Part 3 to fit with the changed approach we're trying for the subsequent patches. Carrying forward r=dholbert.

Comment 39

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 4: Expose nsRuleNode members v3

This patch corresponds to the first patch proposed in comment 35 to expose members of nsRuleNode.

Comment 40

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 5: Split StyleWithDeclarationAdded

Second patch proposed in comment 35. This patch splits StyleWithDeclarationAdded out.

Comment 41

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 6: Check if value is cached in the rule tree

Final part, see if the value is cached in the rule tree.

I've tried to follow the proposal in comment 35 but I've come a bit unstuck since I haven't got a really good grasp on the internals of the style system. Any feedback would be most appreciated.

Some of the many issues with this patch:

* I'm not sure how I'm supposed to check if something is cached in the rule tree. I hacked in a 'NodeHasCachedData' method to nsRuleNode for now, but I'm sure it's wrong.

* I wrote the parameters as suggested, i.e. 'aIsCachedInRuleTree' and comments to match however I found this a little hard to understand since I think callers of nsStyleAnimation::ComputeValue are really just interested in whether the computed value itself is cache-able by the caller (and perhaps shouldn't care about the style module's internal data structures?). For example, if we get a short-hand property, we just return the value as-is. In that case it's not obvious what the return value 'aIsCachedInRuleTree' should be. For that reason I switched back to 'aIsContextSensitive'. But I can be easily convinced otherwise.

* It currently spits out assertions about MathML because we're blindly setting all unset properties to initial but it seems like we need to special case to make sure MathML properties are untouched if MathML is disabled.

Comment 42

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to comment #41)
> * I'm not sure how I'm supposed to check if something is cached in the rule
> tree. I hacked in a 'NodeHasCachedData' method to nsRuleNode for now, but I'm
> sure it's wrong.

I just saw this:

> // set *aCachedInRuleTree (which requires exposing a method on
> // nsStyleContext to find out if something was cached in the rule tree)

Sorry, I missed that before. All the same, any pointers on this would be appreciated.

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 521101 [details] [diff] [review]
Part 4: Expose nsRuleNode members v3

>-static const StructCheckData gCheckProperties[] = {
>+const nsRuleNode::StructCheckData nsRuleNode::gCheckProperties[] = {

Put /* static */ in front of the const.

> inline nsRuleNode::RuleDetail
> nsRuleNode::CheckSpecifiedProperties(const nsStyleStructID aSID,
>                                      const nsRuleDataStruct& aRuleDataStruct)
> {
>-  const StructCheckData *structData = gCheckProperties + aSID;
>+  const nsRuleNode::StructCheckData *structData = gCheckProperties + aSID;

This shouldn't be needed since you're in an nsRuleNode method.

>-  for (const PropertyCheckData *prop = structData->props,
>+  for (const nsRuleNode::PropertyCheckData *prop = structData->props,

same here

r=dbaron with that

Comment 44

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 521102 [details] [diff] [review]
Part 5: Split StyleWithDeclarationAdded

r=dbaron

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 521103 [details] [diff] [review]
Part 6: Check if value is cached in the rule tree

Merging with the change in bug 636039 will make 
InitialStyleRule::MapRuleInfoInto a lot simpler.

In InitialStyleRule, I wouldn't make SetInitial a separate function;
it should just be a nested loop.  

(It would also actually make more sense to put the initial style rule
on the style set; I forgot nsStyleAnimation was static members only.)


nsRuleNode::NodeHasCachedData is sort of the right idea, except you
only want to check for the one struct you care about, so it should
take an SID argument.  (You want to use the SID of the property you
care about.)  And then you just want to return whether
mStyleData.GetStyleData(aSID) is non-null.

Other than that, I think this is mostly good, though I'll want to
do another pass (and I'll probably have a few more nits then).

Sorry for taking so long to get back to this.

Comment 46

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And where are those MathML-related assertions?

Comment 47

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 1: Refactor test_smilChangeAfterFrozen.xhtml v1e; r=dholbert

Just adding author info

Comment 48

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 2a: Add further tests v1f; r=dholbert

Adds author info.

Comment 49

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 2b: Add even more tests v1d; r=dholbert

Adds author info

Comment 50

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 3: Add context-sensitive flag to style animation resolution v1f; r=dholbert

Update to tip.

Comment 51

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 521101 [details] [diff] [review]
Part 4: Expose nsRuleNode members v3

Patch 4 is no longer required due to changes in bug 636039.

Comment 52

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 5: Split StyleWithDeclarationAdded, r=dbaron

Update to tip.

Comment 53

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 6: Check if value is cached in the rule tree

Address review feedback (comment 45) and merge with changes in bug 636039.

(In reply to comment #45)
> (It would also actually make more sense to put the initial style rule
> on the style set; I forgot nsStyleAnimation was static members only.)

Sorry, I just noticed this now. Is this worth doing?

Comment 54

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to comment #46)
> And where are those MathML-related assertions?

http://mxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.cpp#1632

In the latest patch for part 6 I've added code to avoid setting MathML properties if MathML is not enabled.

Comment 55

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #53)
> (In reply to comment #45)
> > (It would also actually make more sense to put the initial style rule
> > on the style set; I forgot nsStyleAnimation was static members only.)
> 
> Sorry, I just noticed this now. Is this worth doing?

I think it is, if you don't mind.

Comment 56

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 6: Check if value is cached in the rule tree

(In reply to comment #55)
> (In reply to comment #53)
> > (In reply to comment #45)
> > > (It would also actually make more sense to put the initial style rule
> > > on the style set; I forgot nsStyleAnimation was static members only.)
> > 
> > Sorry, I just noticed this now. Is this worth doing?
> 
> I think it is, if you don't mind.

I went to do this and but I wasn't exactly sure what you had in mind here. Is it:

a) Moving the definition to nsStyleSet?
b) Moving the static (singleton) member, sInitialStyleRule, there too?
c) Getting rid of the static (singleton) member and adding an extra mInitialStyleRule member to every nsStyleSet object?
d) Getting rid of the static (singleton) member and just allocating a new initial style rule member every time we need one?

For now I've just done (a).

Comment 57

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Option (c) was what I was thinking.

Comment 58

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 6: Check if value is cached in the rule tree

Address review feedback

Comment 59

[Brian Birtles (:birtles, Feb 26~ parental leave)]

A TryServer run with this patch queue turned up problems with /content/smil/crashtests/572938-1.svg (<use> with animate display="none") on Windows builds. We seem to get into an infinite update situation. Currently debugging.

Comment 60

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 548696 [details] [diff] [review]
Part 6: Check if value is cached in the rule tree

+  // Style rule which sets all unset properties set to their initial values to
+  // be used to determine when context-sensitive values are in use.
+  nsRefPtr<nsInitialStyleRule> mInitialStyleRule;

"all unset properties" -> "all properties", since the test for being
unset is a function of how rule mapping works, not part of the
underlying concept.  (In particular, we implement rule mapping by
walking the rules whose styles win first.)


In nsInitialStyleRule::MapRuleInfoInto, please replace the 3 variable
names with "values" -> "value".

Also, these bits:
+  for (PRUint32 sid = nsStyleStructID_Inherited_Start;
+       sid < nsStyleStructID_Length; ++sid) {

+      // Iterate over nsCSSValues within the property group
+      nsStyleStructID sidAsId = static_cast<nsStyleStructID>(sid);

should just use one sid variable; construct the for loop similar to the
way the constructors of nsInheritedStyleData and nsResetStyleData (in
nsRuleNode.h) do.  Also, start from 0 rather than
nsStyleStructID_Inherited_Start.

In nsStyleAnimation::ComputeValue -- I think moving the bit that does
the SetUnparsedStringValue earlier in the function is ok, except I think
you've moved it too far.  I'd prefer if it were still after the
BuildStyleRule call (and following if (!styleRule) { return PR_FALSE }),
since I'd prefer that the function continues to return false when the
value is syntactically incorrect, rather than allowing syntax errors
into unparsed string values.  In other words, move the first inserted
chunk in that function down to be contiguous with the second.

r=dbaron with that

Comment 61

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to comment #59)
> A TryServer run with this patch queue turned up problems with
> /content/smil/crashtests/572938-1.svg (<use> with animate display="none") on
> Windows builds. We seem to get into an infinite update situation. Currently
> debugging.

I'm still not really sure what's going on here. It seems like it's bug 536660.

It's not the fault of these patches. Without these patches applied if we simply set aPreventCachingOfSandwich to PR_TRUE in nsSMILCSSProperty, the test case mentioned (and also the newly attached test case for bug 536660), will cause memory to be used up very quickly and the UI to become unresponsive.

However, we can't land these patches until the underlying problem is resolved. :(

Comment 62

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Brian Birtles (:birtles) from comment #61)
> It's not the fault of these patches. Without these patches applied if we
> simply set aPreventCachingOfSandwich to PR_TRUE in nsSMILCSSProperty, the
> test case mentioned (and also the newly attached test case for bug 536660),
> will cause memory to be used up very quickly and the UI to become
> unresponsive.

Sorry, that should be "set aPreventCachingOfSandwich to PR_FALSE"

Comment 63

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 1: Refactor test_smilChangeAfterFrozen.xhtml v1f; r=dholbert

Fix bitrot

Comment 64

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 2a: Add further tests v1g; r=dholbert

Fix bitrot.

Comment 65

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 2b: Add even more tests v1e; r=dholbert

Fix bitrot.

Comment 66

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 6: Check if value is cached in the rule tree; r=dbaron

Address review feedback (comment 60).

Comment 67

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 6: Check if value is cached in the rule tree; r=dbaron

Missed part of the patch last time.

Comment 68

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Part 7 - Don't cache display animation values

As discussed with dholbert on irc, the test failures noted in comment 59 seem to be related to bug 536660. For now we'll just disable this optimisation for animation on the display property until the underlying bug is fixed.

Comment 69

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Pushed to m-i:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d38070172b15
http://hg.mozilla.org/integration/mozilla-inbound/rev/cfdaadaff7c6
http://hg.mozilla.org/integration/mozilla-inbound/rev/c75d1fd80110
http://hg.mozilla.org/integration/mozilla-inbound/rev/fa81eb3743d7
http://hg.mozilla.org/integration/mozilla-inbound/rev/d571c0e5cf18
http://hg.mozilla.org/integration/mozilla-inbound/rev/621f3dd51ed2
http://hg.mozilla.org/integration/mozilla-inbound/rev/f385c77d177e

Comment 70

[Mounir Lamouri (:mounir)]

http://hg.mozilla.org/mozilla-central/rev/d38070172b15
http://hg.mozilla.org/mozilla-central/rev/cfdaadaff7c6
http://hg.mozilla.org/mozilla-central/rev/c75d1fd80110
http://hg.mozilla.org/mozilla-central/rev/fa81eb3743d7
http://hg.mozilla.org/mozilla-central/rev/d571c0e5cf18
http://hg.mozilla.org/mozilla-central/rev/621f3dd51ed2
http://hg.mozilla.org/mozilla-central/rev/f385c77d177e