Last Comment Bug 664343 - SVG SMIL: Tear down of syncbase intervals is inefficient
: SVG SMIL: Tear down of syncbase intervals is inefficient
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Brian Birtles (:birtles)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-14 17:31 PDT by Brian Birtles (:birtles)
Modified: 2011-08-24 05:27 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1a -- breaks one of the crashtests (see comment 0) (3.07 KB, text/plain)
2011-06-14 17:31 PDT, Brian Birtles (:birtles)
no flags Details
Patch v2a - Batch interval updates (6.82 KB, patch)
2011-06-27 17:47 PDT, Brian Birtles (:birtles)
dholbert: review+
Details | Diff | Review
Patch v3a - Batch interval updates (7.10 KB, patch)
2011-06-30 17:50 PDT, Brian Birtles (:birtles)
dholbert: review+
Details | Diff | Review
Patch v3b - Batch interval updates, r=dholbert (8.24 KB, patch)
2011-06-30 18:55 PDT, Brian Birtles (:birtles)
bbirtles: checkin+
Details | Diff | Review

Description Brian Birtles (:birtles) 2011-06-14 17:31:51 PDT
Created attachment 539384 [details]
Patch v1a -- breaks one of the crashtests (see comment 0)

Currently when we remove a timed element with times and intervals involved in syncbase networks we do a lot of updating that probably isn't necessary.

The attached patch attempts to do things more efficiently but it's not quite right. It will cause a stack overflow in smil/crashtests/596796-1.svg as we constantly create and delete intervals.

I'm just filing this as something to come back to. The attached patch might be salvageable but I need to think through how this teardown ought to work--i.e. is there some way to ensure we won't end up chasing our tail as we modify intervals.
Comment 1 Brian Birtles (:birtles) 2011-06-27 17:47:03 PDT
Created attachment 542344 [details] [diff] [review]
Patch v2a - Batch interval updates

An alternative approach. This time we just batch updates to the interval.
Comment 2 Robert Longson 2011-06-28 00:41:12 PDT
PRPackedBool mDoDeferredUpdate:1;

You shouldn't use :1 unless you have to and there's no reason to here.
Comment 3 Daniel Holbert [:dholbert] 2011-06-30 15:57:22 PDT
Comment on attachment 542344 [details] [diff] [review]
Patch v2a - Batch interval updates

> //----------------------------------------------------------------------
>+// Helper class: AutoIntervalUpdateBatcher
>+
>+// RAII helper to update the mIntervalUpdateBlockCount flag on an

mIntervalUpdateBlockCount isn't really a flag -- it's a number. Maybe s/flag/var/?  Or just nix 'flag' altogether?  Or maybe mIntervalUpdateBlockCount was a typo there, and you meant to say "the mDoDeferredUpdate flag"?

>+class nsSMILTimedElement::AutoIntervalUpdateBatcher

Annotate this as NS_STACK_CLASS (to get a static-analysis assertion that we never heap-allocate this).

>+  AutoIntervalUpdateBatcher(nsSMILTimedElement& aTimedElement)
>+    : mTimedElement(aTimedElement)
>+  {
>+    mTimedElement.mDeferIntervalUpdates = PR_TRUE;

I think "mDeferIntervalUpdates" is a typo there -- that name isn't mentioned anywhere else in this patch.  I think this wants to be mDoDeferredUpdate?  (though you may want to keep this one and rename all the others, per my final comment below)

>+  }
>+
>+  ~AutoIntervalUpdateBatcher()
>+  {
>+    NS_ABORT_IF_FALSE(mTimedElement.mIntervalUpdateBlockCount > 0,
>+        "Unbalanced updates to interval update block count");
>+    mTimedElement.mIntervalUpdateBlockCount--;

Hmm... so we *decrement* the count here, but I don't see anywhere where we *increment* the count.  Maybe I'm overlooking something, but I would have assumed we'd be incrementing the count in the AutoIntervalUpdateBatcher constructor.

> nsSMILTimedElement::UpdateCurrentInterval(PRBool aForceChangeNotice)
> {
>+  // Check if updates are currently blocked (batched)
>+  if (mIntervalUpdateBlockCount) {
>+    mDoDeferredUpdate = PR_TRUE;
>+    return;

Do we really need to set this flag here?  We already set it in the AutoIntervalUpdateBatcher constructor, so I assume this function would just check the flag & return early if it's on.

>+  PRPackedBool mDoDeferredUpdate:1;

I think we shold probably rename "mDoDeferredUpdate" -- "Do" is very active-sounding, whereas this flag is really trying to promote (temporary) *inaction*. :)  Perhaps "mDeferringUpdate" would be better?  (Or "mDeferIntervalUpdates" is fine too)
Comment 4 Daniel Holbert [:dholbert] 2011-06-30 15:58:39 PDT
Comment on attachment 542344 [details] [diff] [review]
Patch v2a - Batch interval updates

r=me with the above fixes, assuming it passes tests.
Comment 5 Brian Birtles (:birtles) 2011-06-30 16:59:42 PDT
I made two versions of this: one which was stack-based (and didn't need a block count) and one which could be heap-allocated. The two got mixed up when I split this patch out from some other work I was doing.

Hence, all confusing comments, variable names, unnecessary bool packing etc. I'm going to redo this as a stack-only class which will eliminate the need for the block count.
Comment 6 Daniel Holbert [:dholbert] 2011-06-30 17:10:56 PDT
>-  count = mEndSpecs.Length();
>-  for (PRUint32 j = 0; j < count; ++j) {
>-    mEndSpecs[j]->ResolveReferences(aContextNode);
>+    // Resolve references to other parts of the tree
>+    PRUint32 count = mBeginSpecs.Length();
>+    for (PRUint32 i = 0; i < count; ++i) {
>+      mBeginSpecs[i]->ResolveReferences(aContextNode);
>+    }

Side note: We should nix the "count" variable, and just directly check "i < array.Length()" in the for condition.  The local 'count' variable doesn't actually buy us any perf win, and it introduces a place for us to make mistakes (e.g. if we use a stale value of 'count' or something).

That should perhaps happen in its own bug, though, since we have this pattern in a number of places in this file (and maybe in related files?), and it'd be nice to just fix 'em all at once.
Comment 7 Brian Birtles (:birtles) 2011-06-30 17:50:51 PDT
Created attachment 543311 [details] [diff] [review]
Patch v3a - Batch interval updates

Re-written to be a stack-based class. Fix confusing comments.
Comment 8 Daniel Holbert [:dholbert] 2011-06-30 18:26:28 PDT
Comment on attachment 543311 [details] [diff] [review]
Patch v3a - Batch interval updates

>+  ~AutoIntervalUpdateBatcher()
>+  {
[...]
>+    if (mTimedElement.mDoDeferredUpdate)
>+    {

Style nit: Bump up the open-brace on the if-check, like:
 if (mTimedElement.mDoDeferredUpdate) {

>+private:
>+  nsSMILTimedElement& mTimedElement;
>+  PRBool mDidSetFlag;

Nit: Always PRPackedBool for bools inside of a class. (I'm not sure it makes any actual difference in this case, but it's good for consistency)

Also -- it's important that nsSMILTimedElement never dies until after its last AutoIntervalUpdateBatcher dies. So -- add assertions to ~nsSMILTimedElement(), to verify that mDeferIntervalUpdates and mDoDeferredUpdate are both false at that point.


>+  {
>+    AutoIntervalUpdateBatcher updateBatcher(*this);

Maybe add a comment above the "{", to explain its presence and keep people from accidentally deleting them in code cleanup at some point.
Something like:
  // Scope updateBatcher to last only for the ResolveReferences calls:

>+++ b/content/smil/nsSMILTimedElement.h
>+  PRPackedBool mDeferIntervalUpdates:1;
>+  PRPackedBool mDoDeferredUpdate:1; // Set if an update to the current interval

These still shouldn't be :1 -- by default, PRPackedBool is 1 byte, so if you have fewer than 4 of them, they'll pack just fine on their own.  The ":1" is only helpful if you've got more than 4 of 'em.
Comment 9 Brian Birtles (:birtles) 2011-06-30 18:55:30 PDT
Created attachment 543323 [details] [diff] [review]
Patch v3b - Batch interval updates, r=dholbert

Address review feedback.

Thanks Daniel!
Comment 10 Brian Birtles (:birtles) 2011-07-01 20:45:30 PDT
Pushed to m-i:
http://hg.mozilla.org/integration/mozilla-inbound/rev/a29692a06cbd
Comment 11 Marco Bonardo [::mak] 2011-07-02 03:09:34 PDT
http://hg.mozilla.org/mozilla-central/rev/a29692a06cbd
Comment 12 Vlad [QA] 2011-08-24 05:27:59 PDT
Setting resolution to Verified Fixed on 
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0

Note You need to log in before you can comment on or make changes to this bug.