560503 - Does AddMutationObserver really need to use AppendElementUnlessExists?

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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?

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: 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.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/5c843ae12c40 with those inlines.