input.value = '' might not have to remove text node for perfomance

RESOLVED FIXED in Firefox 56

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

(Blocks 1 bug)

55 Branch
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [qf])

Attachments

(2 attachments)

Posted file test.html
When profiling BackboneJS-TodoMVC/Adding100Items in SpeedMeter, it seems to use input.value = 'xxx' and input.value = '' into loop from profiling.

Our implementation of input.value = '' is that text node into editor is removed and insert bogus node.  So it causes additional node creation / removed.


Then, when running attached text.html on my MacBook 2016,

Nightly 56 ... 650ms
Safari TP ... 13ms

This might be slow case against other browsers.  (~10% of BackboneJS-TodoMVC/Adding100Items seems to be set_value at least)
Blocks: 1346723
Summary: input.value = '' might not remove text node → input.value = '' might not have to remove text node for perfomance
(In reply to Makoto Kato [:m_kato] from comment #0)
> When profiling BackboneJS-TodoMVC/Adding100Items in SpeedMeter, it seems to
> use input.value = 'xxx' and input.value = '' into loop from profiling.
> 
> Our implementation of input.value = '' is that text node into editor is
> removed and insert bogus node.  So it causes additional node creation /
> removed.

Yes I think there are other speedometer tests doing this "clear and then set to a new value" thing. Definitely worth fixing.
Whiteboard: [qf]
Assignee: nobody → m_kato
Priority: -- → P2
Priority: P2 → P1
FWIW, on my machine I got 0.5 point improvement when dummy node wasn't created (a simple hacky patch).
That is enough a lot to try to fix this, even though I do believe this happens to be rather tricky bug.
Comment on attachment 8881594 [details]
Bug 1375910 - Don't remove text node when setting value is empty string.

https://reviewboard.mozilla.org/r/152752/#review157916

::: editor/libeditor/TextEditRules.cpp:353
(Diff revision 1)
>        return NS_OK;
>    }
>  }
>  
>  NS_IMETHODIMP_(bool)
>  TextEditRules::DocumentIsEmpty()

Now, this returns true even if there are a lot of empty text nodes. (Although, we *might* need to check if comment node, etc, for HTMLEditor, it's really another bug.)

nsIEditor.documentIsEmpty is declared as "Returns true if the document has no *meaningful* content". I think that nIEditRules should explain this more. E.g.,

/**
 * Return false if the editor has non-empty text nodes or non-text
 * nodes.  Otherwise, i.e., there is no meaningful content,
 * return true.
 */

I think that we need to clean up nsIEditor.documentIsEmpty() in follow up bug. (Looks like that EditorBase::GetDocumentIsEmpty() is never used.)

::: editor/libeditor/TextEditRules.cpp:361
(Diff revision 1)
> +    return true;
> +  }
> +
> +  // Even if there is no bogus node, we should detect as empty document
> +  // if all children are text node and these are no content.
> +  // For performance, we don't sometimes remove node even if text is empty node.

// For performance reason, we won't remove last one text node even when it
// becomes empty.

?

::: editor/libeditor/TextEditRules.cpp:366
(Diff revision 1)
> +  // For performance, we don't sometimes remove node even if text is empty node.
> +  if (NS_WARN_IF(!mTextEditor)) {
> +    return true;
> +  }
> +  Element* rootElement = mTextEditor->GetRoot();
> +  if (NS_WARN_IF(!rootElement)) {

Don't use NS_WARN_IF() here since this is possible case. For example, if the editor is HTMLEditor and its mode is set to plaintext, it uses TextEditoRules (e.g., plaintext email composer of Thunderbird).  Then, like crash test, HTML editor may be alive with a document node which doesn't have children but in designMode.
Attachment #8881594 - Flags: review?(masayuki) → review+
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/c46231bce5bc
Don't remove text node when setting value is empty string. r=masayuki
https://hg.mozilla.org/mozilla-central/rev/c46231bce5bc
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1377242
Depends on: 1415416
You need to log in before you can comment on or make changes to this bug.