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

RESOLVED FIXED in Firefox 57

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: smaug)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [qf])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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...
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.
(Reporter)

Comment 2

2 years ago
Fantastic!

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

Comment 3

2 years ago
This shows up pretty badly when profiling bug 1346723.
Whiteboard: [qf]
(Assignee)

Updated

2 years ago
Assignee: nobody → bugs
(Assignee)

Comment 4

2 years ago
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
Attachment #8900463 - Flags: review?(masayuki)
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.
Attachment #8900463 - Flags: review?(masayuki) → review+

Comment 7

2 years ago
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73e410c3bfec
add a faster variant of TextEditor::GetDocumentIsEmpty(), r=masayuki

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/73e410c3bfec
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.