1390402 - Consider creating an inline and non-virtual (and fast) variant of TextEditor::GetDocumentIsEmpty()

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement, P3)

Description

[(no longer active)]

This function shows up in Speedometer under this call stack:

HTMLInputElement::IsValueEmpty()
  nsTextEditorState::HasNonEmptyValue()
    TextEditor::GetDocumentIsEmpty()

Right now this function is implemented in terms of the virtual nsIEditRules::DocumentIsEmpty(), and everything is non-inline, and there is needless overhead involved (for example, I suspect the AddRef/Release here is no longer necessary <https://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/editor/libeditor/TextEditor.cpp#909> since there is no call to IsPreformatted() down this call path any more.

We should be able to inline and devirtualize and optimize things a lot here...

Comment 1

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

Yeah, and a11y module will use concrete editor classes (bug 1388006). So, it'll be unnecessary to be a interface method or we can create new non-virtual method which is not in any interfaces.

Comment 2

[(no longer active)]

Fantastic!

BTW I mentioned a funny thing I encountered.  We also have BaseEditor::GetDocumentIsEmpty() which is dead code as far as I can tell.  :-)

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

This shows up pretty badly when profiling bug 1346723.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: editor_document_is_empty.diff

Quick and dirty approach to inline and non-virtualize some methods.
Seems to cut the time we spent checking emptyness to half.

remote: View the pushlog for these changes here:
remote:   https://hg.mozilla.org/try/pushloghtml?changeset=d04f2a15b41b3cff4d593117f2556b121c3623d7
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=d04f2a15b41b3cff4d593117f2556b121c3623d7

Comment 5

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

Comment on attachment 8900463 [details] [diff] [review]
editor_document_is_empty.diff

> uint32_t
> nsGenericDOMDataNode::TextLength() const
> {
>-  return mText.GetLength();
>+  return TextDataLength();

Really similar name and exactly same type method except if it's virtual... but I have no better idea.

>@@ -2790,17 +2790,17 @@ nsTextEditorState::SetValue(const nsAStr
> }
> 
> bool
> nsTextEditorState::HasNonEmptyValue()
> {
>   if (mTextEditor && mBoundFrame && mEditorInitialized &&
>       !mIsCommittingComposition) {
>     bool empty;
>-    nsresult rv = mTextEditor->GetDocumentIsEmpty(&empty);
>+    nsresult rv = mTextEditor->DocumentIsEmpty(&empty);
>     if (NS_SUCCEEDED(rv)) {

Hmm, currently, nsIEditor::GetDocumentIsEmpty() returns error only when the editor is not available or the document has no nodes. I think that DocumentIsEmpty() should return bool and such illegal state should be checked by the callers, but in this bug, we should take this code.

>diff --git a/editor/libeditor/EditorBase.h b/editor/libeditor/EditorBase.h
>--- a/editor/libeditor/EditorBase.h
>+++ b/editor/libeditor/EditorBase.h
>@@ -873,17 +873,20 @@ public:
>    */
>   static nsresult GetTagString(nsIDOMNode* aNode, nsAString& outString);
>   static nsIAtom* GetTag(nsIDOMNode* aNode);
> 
>   bool NodesSameType(nsIDOMNode* aNode1, nsIDOMNode* aNode2);
>   virtual bool AreNodesSameType(nsIContent* aNode1, nsIContent* aNode2);
> 
>   static bool IsTextNode(nsIDOMNode* aNode);
>-  static bool IsTextNode(nsINode* aNode);
>+  static bool IsTextNode(nsINode* aNode)
>+  {
>+    return aNode->NodeType() == nsIDOMNode::TEXT_NODE;
>+  }

If there were nsINode::IsTextNode()... Up to you.

>   /**
>    * Fast non-refcounting editor root element accessor
>    */
>-  Element* GetRoot();
>+  Element* GetRoot()
>+  {
>+    if (!mRootElement) {
>+      // Let GetRootElement() do the work
>+      nsCOMPtr<nsIDOMElement> root;
>+      GetRootElement(getter_AddRefs(root));
>+    }
>+
>+    return mRootElement;

Oh, you need to update your previous patch. Then, we'll be able to get rid of the virtual call of GetRootElement().

>+nsresult
>+TextEditor::DocumentIsEmpty(bool* aIsEmpty)
>+{
>+  NS_ENSURE_TRUE(mRules, NS_ERROR_NOT_INITIALIZED);
>+
>+  if (static_cast<TextEditRules*>(mRules.get())->HasBogusNode()) {

Hmm, we should get rid of nsIEditRules abstract class since it's not referred outside of editor/. Then, we could get rid of some virtual calls and avoid using such static_cast. I'll file a bug for it.

Comment 6

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

Filed bug 1393337 for getting rid of nsIEditRules.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73e410c3bfec
add a faster variant of TextEditor::GetDocumentIsEmpty(), r=masayuki

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/73e410c3bfec