Closed Bug 1390402 Opened 5 years ago Closed 5 years ago

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


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




Performance Impact ?
Tracking Status
firefox57 --- fixed


(Reporter: ehsan.akhgari, Assigned: smaug)


(Blocks 1 open bug)



(1 file)

This function shows up in Speedometer under this call stack:


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

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
This shows up pretty badly when profiling bug 1346723.
Whiteboard: [qf]
Assignee: nobody → bugs
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: Follow the progress of your build on Treeherder:
Attachment #8900463 - Flags: review?(masayuki)
Comment on attachment 8900463 [details] [diff] [review]

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

>+TextEditor::DocumentIsEmpty(bool* aIsEmpty)
>+  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+
Filed bug 1393337 for getting rid of nsIEditRules.
Pushed by
add a faster variant of TextEditor::GetDocumentIsEmpty(), r=masayuki
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Performance Impact: --- → ?
Whiteboard: [qf]
You need to log in before you can comment on or make changes to this bug.