Closed Bug 883416 Opened 7 years ago Closed 7 years ago

Simplify the coalescing of invalidation notifications in VectorImage

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox23 + fixed
firefox24 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(3 files, 1 obsolete file)

Bug 846028 (and the followup bug 851416) make VectorImage coalesce invalidation notifications, which can improve the performance of pages that use SVG images. This is a good thing, but the implementation strategy used was chosen based on performance assumptions that didn't pan out: I thought it'd be faster to remove the VectorImage's SVGRootRenderingObserver from the observer lists when it didn't need to receive notifications, but the bookkeeping turns out to prevent any benefit.

This means that we'd be better off with a simpler approach: stay in the observer lists, but use a flag to decide whether to notify. This should make the code simpler to reason about with no real downside.
We start by doing some reverting.
Attachment #765673 - Flags: review?(dholbert)
At this point we're back to the situation before bug 846028.
Attachment #765674 - Flags: review?(dholbert)
Here's the meat of the patch. We just use a flag to decide when to ignore invalidations. We start in listening mode. When we send out an invalidation, we switch to not listening mode until VectorImage::Draw runs. Draw calls ResumeListening which puts us back in listening mode. This code ends up being simpler because we never leave the observer lists, so we don't have to do any bookkeeping of them.

(Note that I've replaced the original code for adding us back to the rendering observer list with a call to GetReferencedElement, which we agreed in bug 846028 was a better approach.)

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=c49151e96829
Attachment #765681 - Flags: review?(dholbert)
Comment on attachment 765681 [details] [diff] [review]
(Part 3) - Coalesce invalidations in RasterImage using a listening flag.

>   virtual ~SVGRootRenderingObserver()
>   {
>     StopListening();
>   }
> 
>+  void ResumeListening()
>+  {
>+    mListening = true;
>+  }
>+

mListening and ResumeListening might be confusing names for these things, because we've also got StartListening() / StopListening() methods, which we inherit, and which are pretty much completely unrelated to your new "Listening" variable/function.

How about naming these new things "mHonoringUpdates" and "ResumeHonoringUpdates()" (since this all revolves around the "DoUpdate" function)?  Or s/Updates/Invalidations/ is fine, too.


>   virtual void DoUpdate() MOZ_OVERRIDE
>   {
[...]
>-    // Our caller might've removed us from rendering-observer list.
>-    // Add ourselves back!
>-    if (!mInObserverList) {
>-      nsSVGEffects::AddRenderingObserver(elem, this);
>-      mInObserverList = true;
>-    } 
>+    // Our caller might've removed us from rendering-observer list. Add
>+    // ourselves back! (GetReferencedElement does this internally.)
>+    GetReferencedElement();

I don't think this change is strictly necessary, right?

I'd rather hold off on this (maybe splitting it into its own bug?) for 3 reasons:
 a) It'd be nice to keep this bug's patches targeted (and IIUC this change isn't a necessary part of that)

 b) It's not a clear benefit -- the code that you're removing is strictly cheaper. GetReferencedElement involves a virtual function-call (to GetTarget), which the existing code doesn't have, with because it already knows the target.

 c) The code that you're removing exists in the constructor, too -- so just removing it here leaves us in a somewhat-inconsistent state. If you want to replace it, you should probably replace it in both spots.
Thanks for the review!

(In reply to Daniel Holbert [:dholbert] from comment #4)
> mListening and ResumeListening might be confusing names for these things,

No prob. I'll change them.

> I don't think this change is strictly necessary, right?

That's fine with me. I was restoring it because it was a change we made in bug 846028, which this bug conceptually replaces. (I forgot to update the constructor there, and then ended up copying that mistake into this patch, actually!) We can go ahead and leave it out. I'll update the patch to remove this change.
Comment on attachment 765681 [details] [diff] [review]
(Part 3) - Coalesce invalidations in RasterImage using a listening flag.

Sounds good! r=me with those fixes.

(Looking back at bug 846028 comment 3, it looks like I'd suggested GetReferencedElement over there because the ResumeListening() impl (including its virtual method call to GetTarget) could be *entirely* replaced with a call to GetReferencedElement -- but that's not the case here in DoUpdate(), because we already need to invoke GetTarget() for other reasons, and we might as well re-use its result instead of invoking it again.)
Attachment #765681 - Flags: review?(dholbert) → review+
Yeah, that makes sense.
Attachment #765681 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/32e0e0bb5cc5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Can we get an uplift nomination for Beta here since this fixes the FF23 tracked bug 879139?
Flags: needinfo?(seth)
Comment on attachment 765673 [details] [diff] [review]
(Part 1) - Revert bug 851416.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 846028
User impact if declined: See bug 879139.
Testing completed (on m-c, etc.): Two weeks on m-c with no issues.
Risk to taking this patch (and alternatives if risky): Low.
String or IDL/UUID changes made by this patch: None.
Attachment #765673 - Flags: approval-mozilla-beta?
Comment on attachment 765674 [details] [diff] [review]
(Part 2) - Revert bug 846028.

[Approval Request Comment]
See above.
Attachment #765674 - Flags: approval-mozilla-beta?
Comment on attachment 766053 [details] [diff] [review]
(Part 3) - Coalesce invalidations in RasterImage using a listening flag.

[Approval Request Comment]
See above.
Attachment #766053 - Flags: approval-mozilla-beta?
(In reply to lsblakk@mozilla.com [:lsblakk] from comment #11)
> Can we get an uplift nomination for Beta here since this fixes the FF23
> tracked bug 879139?

You got it. This change doesn't seem to have caused any regressions, and as you state, it has an additional benefit as discussed in bug 879139.
Flags: needinfo?(seth)
Attachment #765673 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #765674 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #766053 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Thanks Lukas and Ryan!
You need to log in before you can comment on or make changes to this bug.