The default bug view has changed. See this FAQ.

SVG SMIL: Tear down of syncbase intervals is inefficient

VERIFIED FIXED in mozilla7

Status

()

Core
SVG
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

Trunk
mozilla7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Assignee: nobody → birtles
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
Created attachment 542344 [details] [diff] [review]
Patch v2a - Batch interval updates

An alternative approach. This time we just batch updates to the interval.
Attachment #539384 - Attachment is obsolete: true
Attachment #542344 - Flags: review?(dholbert)

Comment 2

6 years ago
PRPackedBool mDoDeferredUpdate:1;

You shouldn't use :1 unless you have to and there's no reason to here.
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 on attachment 542344 [details] [diff] [review]
Patch v2a - Batch interval updates

r=me with the above fixes, assuming it passes tests.
Attachment #542344 - Flags: review?(dholbert) → review+
(Assignee)

Comment 5

6 years ago
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.
>-  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.
(Assignee)

Comment 7

6 years ago
Created attachment 543311 [details] [diff] [review]
Patch v3a - Batch interval updates

Re-written to be a stack-based class. Fix confusing comments.
Attachment #542344 - Attachment is obsolete: true
Attachment #543311 - Flags: review?(dholbert)
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.
Attachment #543311 - Flags: review?(dholbert) → review+
(Assignee)

Comment 9

6 years ago
Created attachment 543323 [details] [diff] [review]
Patch v3b - Batch interval updates, r=dholbert

Address review feedback.

Thanks Daniel!
Attachment #543311 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
Pushed to m-i:
http://hg.mozilla.org/integration/mozilla-inbound/rev/a29692a06cbd
Whiteboard: [inbound]
(Assignee)

Updated

6 years ago
Attachment #543323 - Flags: checkin+
http://hg.mozilla.org/mozilla-central/rev/a29692a06cbd
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
(Assignee)

Updated

6 years ago
Whiteboard: [inbound]

Comment 12

6 years ago
Setting resolution to Verified Fixed on 
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.