1386411 - Make retrieving the selection from the editor more efficient

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

[(no longer active)]

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

Comment 1

[(no longer active)]

Attachment: Part 1: Don't store the selection controller as a weak reference on EditorBase

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

Comment 2

[(no longer active)]

Attachment: Part 2: Devirtualize and inline nsISelection::AsSelection()

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

Comment 3

[(no longer active)]

Attachment: Part 3: Remove the unused ToChar function

Comment 4

[(no longer active)]

Attachment: Part 4: Inline some helper functions in Selection.cpp

Comment 5

[(no longer active)]

Attachment: Part 5: Don't store the document as a weak reference on EditorBase

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

Comment 6

[(no longer active)]

Attachment: Part 6: Make BaseEditor::GetSelectionController() return nsISelectionController*, and inline it

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.

Comment 7

[(no longer active)]

Attachment: Part 7: Add a more efficient nsISelectionController::GetSelection() API for retrieving native Selection objects

This API avoids needless refcounting and QueryInterface overhead.

Comment 8

[(no longer active)]

Attachment: Part 8: Inline EditorBase::GetSelection()

Comment 9

[(no longer active)]

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 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 12

[Boris Zbarsky [:bzbarsky]]

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 13

[Boris Zbarsky [:bzbarsky]]

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 14

[Boris Zbarsky [:bzbarsky]]

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 15

[Boris Zbarsky [:bzbarsky]]

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.

Comment 16

[(no longer active)]

(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.

Comment 17

[(no longer active)]

(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?

Comment 18

[Boris Zbarsky [:bzbarsky]]

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.

Comment 19

[(no longer active)]

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()...  :-/

Comment 20

[(no longer active)]

(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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

Comment 23

[(no longer active)]

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.

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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