1944426 - nsObserverList::RemoveObserver() does an addref and release inside the comparator

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Andrew McCreight [:mccr8]]

The scenario in bug 1943729 is not very realistic, but it does highlight that nsMaybeWeakPtrArray::RemoveWeakElement, which is used by nsObserverList, does a lot of work that it doesn't really need to. Specifically, the line if (MaybeWeakArray::RemoveElement(aElement)) { ends up calling nsDefaultComparator<nsMaybeWeakPtr, nsObserver*>::Equals() a bunch of times, presumably for every element in the array, which, thanks to the implicit constructor for nsMaybeWeakPtr ends up constructing and destroying a nsMaybeWeakPtr, which does an addref and release.

Surely we can do something better here? Can we just explicitly construct a nsMaybeWeakPtr for aElement like we do everywhere else?

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Here's a profile that shows this having a bad time: https://share.firefox.dev/3Q0fNOK

Comment 2

[Andrew McCreight [:mccr8]]

I was hoping somebody else would pick this up, but I guess I'll take a shot at it.

Comment 3

[Andrew McCreight [:mccr8]]

Here is a profile of the "reload" test case from bug 1943729, without a fix for the addref/release stuff, in a local opt build.

Here is a profile of the "reload" test case from bug 1943729, WITH the fix.

In the before case, the CC takes 46 seconds. It then spends about 50 seconds in nsObserverList::RemoveObserver.

In the after case, the CC takes 51 seconds (more or less the same). It then only spends about 8.6 seconds in nsObserverList::RemoveObserver, which is a reduction of around 82%.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Bug 1944426, part 1 - Minor fixups.

Mostly suggested by clang-tidy.

• Rename an argument to aOther.
• Returning a const from GetValue() doesn't add much.
• mWeak should be initialized.
• using is better than typedef.
• should have parens around uses of macro args

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Bug 1944426, part 2 - Use operator== instead of implicit conversion for nsMaybeWeakPtr.

The existing code was using implicit conversion on every comparison, requiring
an addref and release. This was showing up in profiles when removing a large number
of observers. Instead, we define an equality comparison directly for the two
relevant cases. This reduced the time spent removing a large number of observers
in an artificial test case by about 80%.

Weak and strong pointers can still be turned into nsMaybeWeakPtr, but
only via operator=.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ffed2f6abe1
part 1 - Minor fixups. r=xpcom-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/389dd2fb925b
part 2 - Use operator== instead of implicit conversion for nsMaybeWeakPtr. r=xpcom-reviewers,emilio

Comment 7

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/8ffed2f6abe1
https://hg.mozilla.org/mozilla-central/rev/389dd2fb925b