Right now there is a ton of overhead in doing this, I'm filing this bug for a number of ideas to make this less expensive. Here is a profile: https://perfht.ml/2vlfTbA

This doesn't need to be a weak reference, and can instead be a simple strong reference that we introduce to the cycle collector.

nsISelection is builtinclass, so this method doesn't need to be virtual.

This one also doesn't need to be a weak reference, and can be a strong reference that the cycle collector knows about instead.

This method can be extremely hot, so we need to remove all sources of XPCOM overhead from it. This includes the usages of weak pointers (thanks to the previous parts), refcounting, and QueryInterface. I kept the callers hold the selection controller alive by assigning the return value to an nsCOMPtr in places where the methods called on it could have a remote chance of messing with the lifetime of objects.

This API avoids needless refcounting and QueryInterface overhead.

After these patches, the test case in bug 1346723 runs about 10ms faster on my machine on average! EditorBase::GetSelection() doesn't show up in the profile any more after these changes \o/ Here is a profile of BeginPlaceholderTransaction after: https://perfht.ml/2vhb9nS

Comment on attachment 8892682 [details] [diff] [review] Part 3: Remove the unused ToChar function Hmm, this is useful when debugging. If this doesn't cause any problem, please keep it.

Comment on attachment 8892687 [details] [diff] [review] Part 8: Inline EditorBase::GetSelection() >diff --git a/editor/libeditor/EditorBase.h b/editor/libeditor/EditorBase.h >index 675f363468fa..f421d760c745 100644 >--- a/editor/libeditor/EditorBase.h >+++ b/editor/libeditor/EditorBase.h >@@ -9,16 +9,17 @@ > #include "mozilla/Assertions.h" // for MOZ_ASSERT, etc. > #include "mozilla/Maybe.h" // for Maybe > #include "mozilla/OwningNonNull.h" // for OwningNonNull > #include "mozilla/PresShell.h" // for PresShell > #include "mozilla/SelectionState.h" // for RangeUpdater, etc. > #include "mozilla/StyleSheet.h" // for StyleSheet > #include "mozilla/WeakPtr.h" // for WeakPtr > #include "mozilla/dom/Text.h" >+#include "mozilla/dom/Selection.h" nit: Odd order. Move it to above Text.h.

Comment on attachment 8892681 [details] [diff] [review] Part 2: Devirtualize and inline nsISelection::AsSelection() >+++ b/dom/base/nsISelection.idl So as a followup, we should consider killing off this IDL file. There is only one subclass, Selection uses WebIDL bindings so the scriptability of nsISelection is unused. So script doesn't really depend on it, I would think. Then we could jusr use dom::Selection as the selection API and not have these weird hoops. If we want to be very safe, we could do that followup post-57. Anyway, r=me for this patch, with that followup filed...

Comment on attachment 8892682 [details] [diff] [review] Part 3: Remove the unused ToChar function Per comment 10, add a comment about how this is meant to be called from a debugger and leave it? We could #ifdef DEBUG it if we really want, I guess.

Comment on attachment 8892683 [details] [diff] [review] Part 4: Inline some helper functions in Selection.cpp It's weird to have the decls in nsISelection_Controller_.idl but the impls in Selection.h and not a selection controller header.... At the very least the IDL should document what header needs to be included to get the function definitions. Better yet would be removing the declarations from the IDL altogether and ensuring that whoever wants the methods includes the right header. r=me with that.

Comment on attachment 8892686 [details] [diff] [review] Part 7: Add a more efficient nsISelectionController::GetSelection() API for retrieving native Selection objects Adding virtual stuff in C++ blocks in a scriptable interface is not ok. It'll cause xpconnect to be confused about the vtable layout, which is a terrible idea. Instead, do this at the top of the IDL file: [ptr] native SelectionPtr(mozilla::dom::Selection) And then in the IDL, not in a C++ block: [noscript,nostdcall,notxpcom] SelectionPtr getSelection(short aType); This will generate a virtual function declaration on nsISelectionController like so: virtual mozilla::dom::Selection* GetSelection(int16_t aType) = 0; which you can then override. And then you won't need the NS_IMETHODIMP noise in the impl. And xpconnect will know there's a thing there, because it's right there in the IDL. >+ nsISelection* selection = >+ frameSelection->GetSelection(ToSelectionType(aRawSelectionType)); >+ >+ return static_cast<Selection*>(selection); nsFrameSelection::GetSelection returns dom::Selection*. So cut out the middleman: return frameSelection->GetSelection(ToSelectionType(aRawSelectionType)); r=me with those two issues fixed, but please do fix the IDL/vtable issue; it's pretty critical.

(In reply to Boris Zbarsky [:bz] from comment #13) > Comment on attachment 8892682 [details] [diff] [review] > Part 3: Remove the unused ToChar function > > Per comment 10, add a comment about how this is meant to be called from a > debugger and leave it? We could #ifdef DEBUG it if we really want, I guess. I just came across this, I will keep it.

(In reply to Boris Zbarsky [:bz] from comment #15) > Adding virtual stuff in C++ blocks in a scriptable interface is not ok. > It'll cause xpconnect to be confused about the vtable layout, which is a > terrible idea. Oh you're right, I just remembered I have shot myself in the foot like this before! > Instead, do this at the top of the IDL file: > > [ptr] native SelectionPtr(mozilla::dom::Selection) > > And then in the IDL, not in a C++ block: > > [noscript,nostdcall,notxpcom] SelectionPtr getSelection(short aType); > > This will generate a virtual function declaration on nsISelectionController > like so: > > virtual mozilla::dom::Selection* GetSelection(int16_t aType) = 0; > > which you can then override. And then you won't need the NS_IMETHODIMP > noise in the impl. And xpconnect will know there's a thing there, because > it's right there in the IDL. But it won't do that since there is a getSelection() already, right? Is there a way to get an overload and not have to rename all callers?

The problem with overloading virtual functions is that MSVC will then reorder stuff in the vtable in various not-easily-predictable (unless you go and pore over their documentation) ways to put the overloads next to each other in the vtable. In particular, the actual vtable won't match what xpconnect thinks is going on. You have a few options: 1) If you're worried about only JS callers, you could [binaryname] the existing getSelection to something like GetSelectionXPCOM, and call the new thing getDOMSelection in the IDL, with [binaryname] making it GetSelection on the C++ side. 2) If you're worried about C++ callers too, just call the new thing getDOMSelection() for now.

I am only going to expose the new method to C++, not to JS, as it returns a Selection*, so I think I will rename to getDOMSelection()... :-/

(In reply to Boris Zbarsky [:bz] from comment #12) > Comment on attachment 8892681 [details] [diff] [review] > Part 2: Devirtualize and inline nsISelection::AsSelection() > > >+++ b/dom/base/nsISelection.idl > > So as a followup, we should consider killing off this IDL file. There is > only one subclass, Selection uses WebIDL bindings so the scriptability of > nsISelection is unused. So script doesn't really depend on it, I would > think. Then we could jusr use dom::Selection as the selection API and not > have these weird hoops. > > If we want to be very safe, we could do that followup post-57. Bug 1387143 filed.

> I am only going to expose the new method to C++, not to JS No, I meant whether you're worried about existing callers of getSelection coming from JS or just C++. Put another way, comment 18 option 1 makes it possible to call getSelection() in JS to get a Selection and GetSelection() in C++ to get a dom::Selection. Option 2 is getSelection() in JS to get a Selection and GetDOMSelection() in C++ to get a dom::Selection.

Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8e16aa741451 Part 1: Don't store the selection controller as a weak reference on EditorBase; r=masayuki https://hg.mozilla.org/integration/mozilla-inbound/rev/61b7d37ac269 Part 2: Devirtualize and inline nsISelection::AsSelection(); r=bzbarsky https://hg.mozilla.org/integration/mozilla-inbound/rev/99684f0b3ed6 Part 3: Inline some helper functions in Selection.cpp; r=bzbarsky https://hg.mozilla.org/integration/mozilla-inbound/rev/cb876b926c32 Part 4: Don't store the document as a weak reference on EditorBase; r=masayuki https://hg.mozilla.org/integration/mozilla-inbound/rev/f570c6739e48 Part 5: Make BaseEditor::GetSelectionController() return nsISelectionController*, and inline it; r=masayuki https://hg.mozilla.org/integration/mozilla-inbound/rev/17155d1c2ca7 Part 6: Add a more efficient nsISelectionController::GetSelection() API for retrieving native Selection objects; r=bzbarsky https://hg.mozilla.org/integration/mozilla-inbound/rev/7290c51efe98 Part 7: Inline EditorBase::GetSelection(); r=masayuki

Oops, looks like I messed up the rebase somewhat, https://hg.mozilla.org/integration/mozilla-inbound/rev/7290c51efe98 includes parts of me addressing the review comments for comment 6. :-( Instead of polluting the history even further I decided to let this stand since the code ultimately checked in was the correct version. Sorry about this.

https://hg.mozilla.org/mozilla-central/rev/8e16aa741451 https://hg.mozilla.org/mozilla-central/rev/61b7d37ac269 https://hg.mozilla.org/mozilla-central/rev/99684f0b3ed6 https://hg.mozilla.org/mozilla-central/rev/cb876b926c32 https://hg.mozilla.org/mozilla-central/rev/f570c6739e48 https://hg.mozilla.org/mozilla-central/rev/17155d1c2ca7 https://hg.mozilla.org/mozilla-central/rev/7290c51efe98