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

RESOLVED INVALID

Status

()

Core
DOM
P2
normal
RESOLVED INVALID
8 months ago
8 months ago

People

(Reporter: Ehsan, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

8 months ago
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?
Flags: needinfo?(overholt)
(Reporter)

Updated

8 months ago
See Also: → bug 1390568
Maybe qDot has time for this (just tell me if you don't, please)?
Flags: needinfo?(overholt) → needinfo?(kyle)
(Reporter)

Comment 2

8 months ago
Nathan's patch in bug 1390568 will improve things by eliminating one of the virtual calls.
(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.
Flags: needinfo?(ehsan)

Updated

8 months ago
Depends on: 1393592
(Reporter)

Comment 4

8 months ago
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.
Flags: needinfo?(ehsan)
Clearing ni? 'til we get back timing info.
Flags: needinfo?(kyle)
Priority: -- → P2
(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?
Flags: needinfo?(kyle)
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?
Flags: needinfo?(kyle) → needinfo?(ehsan)
(Reporter)

Comment 8

8 months ago
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.)
Flags: needinfo?(ehsan)
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.
Status: NEW → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.