Closed
Bug 1390527
Opened 7 years ago
Closed 7 years ago
Make Selection use MFBT SupportsWeakPtr instead of the XPCOM weak reference machinery
Categories
(Core :: DOM: Core & HTML, enhancement, P2)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
INVALID
People
(Reporter: ehsan.akhgari, Unassigned)
References
(Blocks 1 open bug)
Details
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•7 years ago
|
Comment 1•7 years ago
|
||
Maybe qDot has time for this (just tell me if you don't, please)?
Flags: needinfo?(overholt) → needinfo?(kyle)
Reporter | ||
Comment 2•7 years ago
|
||
Nathan's patch in bug 1390568 will improve things by eliminating one of the virtual calls.
Comment 3•7 years ago
|
||
(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)
Reporter | ||
Comment 4•7 years 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)
Updated•7 years ago
|
Priority: -- → P2
Comment 6•7 years ago
|
||
(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)
Comment 7•7 years ago
|
||
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•7 years 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)
Comment 9•7 years ago
|
||
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
Closed: 7 years ago
Resolution: --- → INVALID
Assignee | ||
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•