Does AddMutationObserver really need to use AppendElementUnlessExists?

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
DOM
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.9.3a5
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

I was playing around with a (admittedly somewhat synthetic) performance test which ended up creating lots of content lists.  About 10% of the time for the test ended up being the "is this element in the array already?" check that AppendElementUnlessExists performs.

Maybe we shouldn't worry about that, but it might be worth eliminating the check.  I thought we'd just done it to be safe, right?
I can't remember why we put that in. I suspect largely because existing code did that or some such.

I'd say lets remove it and add an assertion.
Created attachment 440777 [details] [diff] [review]
Proposed fix

We had one consumer that was relying on the current behavior (based on running all tests and looking at the asserts we hit).  This patch just adds a way to keep the old behavior for this one consumer.  The other options are to change the consumer (e.g. to not use an nsIMutationObserver at all) or to do more fundamental changes to the mutation observer storage (e.g. a hashtable, unless we need to keep track of order, in which case we need to be more clever).

Note that this solution makes us still have slow observer _removal_, unfortunately...  The hashtable would deal with that.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #440777 - Flags: review?(jonas)
Comment on attachment 440777 [details] [diff] [review]
Proposed fix

>diff --git a/layout/svg/base/src/nsSVGOuterSVGFrame.cpp b/layout/svg/base/src/nsSVGOuterSVGFrame.cpp

>-    doc->AddMutationObserver(&sSVGMutationObserver);
>+    doc->AddMutationObserverUnlessExists(&sSVGMutationObserver);

Feel free to add a comment saying that it would be nice if we never double-added. That way we could kill that code.

For what it's worth I'd be ok with inlining those Add/RemoveMutationObserver.

r=me
Attachment #440777 - Flags: review?(jonas) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/5c843ae12c40 with those inlines.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Depends on: 564291
You need to log in before you can comment on or make changes to this bug.