1390527 - Make Selection use MFBT SupportsWeakPtr instead of the XPCOM weak reference machinery

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[(no longer active)]

The do_QueryReferent() calls from nsCaret code show up in profiles of bug 1346723 in Speedometer.  In general, the XPCOM weak reference machinery is extremely inefficient, resulting in two often mispredicted virtual calls per each check of the weak pointer (one for nsIWeakReference::QueryReferent() and one for nsQueryReferent::operator()).

It would be nice if we ported Selection to use MFBT's SupportsWeakPtr instead.  This is a fairly mechanical change.  The hard part of it is searching for all of the places in the code where we use selections as weak pointers and replacing them.  I suggest searching for things like do_GetWeakReference() carefully and looking at the call sites to see which ones pass in a selection object.

Andrew, any chance you can find an owner for this one please?

Comment 1

[Andrew Overholt [:overholt]]

Maybe qDot has time for this (just tell me if you don't, please)?

Comment 2

[(no longer active)]

Nathan's patch in bug 1390568 will improve things by eliminating one of the virtual calls.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #0)
> It would be nice if we ported Selection to use MFBT's SupportsWeakPtr
> instead.  This is a fairly mechanical change.  The hard part of it is
> searching for all of the places in the code where we use selections as weak
> pointers and replacing them.  I suggest searching for things like
> do_GetWeakReference() carefully and looking at the call sites to see which
> ones pass in a selection object.

Do we want to do this, or do we want to just get rid of nsIWeakReference entirely, by substituting nsWeakReference where appropriate?  Getting rid of nsIWeakReference entirely is probably easier than trying to do MFBT's SupportsWeakPtr, and then we don't have partially converted code in the tree.

Comment 4

[(no longer active)]

After discussing on IRC, we agreed to measure once after bug 1390568 lands, and once after bug 1393592 lands and re-evaluate whether this is still needed after those.

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Clearing ni? 'til we get back timing info.

Comment 6

[Andrew Overholt [:overholt]]

(In reply to Kyle Machulis [:qdot] [:kmachulis]  (if a patch has no decent commit message, automatic r-) (PTO Aug 24-28) from comment #5)
> Clearing ni? 'til we get back timing info.

Are you able to get that info?

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ran both the tests in bug 1346723 and a few runthroughs of speedometer on linux, no longer seeing nsCaret calls like we were in earlier profiles. Do I need to give these a shot on other platforms also?

Comment 8

[(no longer active)]

I suggest profiling with a high frequency on Linux or OSX.  I use 0.1ms as my go to frequency for micro optimization issues like this usually.  If you don't see anything under do_QueryReferent() coming from nsCaret then I think we can call this fixed.  On Windows, I don't know how to get to such high frequencies without disturbing the profiled code unfortunately... 

(BTW for profiling this I basically just reloaded attachment 8848015 [details] around 20 times or so and captured a profile.)

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Set to 0.1ms, ran 25 iterations of attachment 8848015 [details] on Linux release build using m-c from today, not seeing anything coming from nsCaret.

https://perfht.ml/2wWenhI

Marking invalid as this seems to be fixed.