1395146 - Optimize nsTextEditorState::GetValue()

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

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

I still see nsTextEditorState::GetValue() when running attachment 8848015 [details]. Mainly, it's slow due to the cost of TextEditor::OutputToString() and number of calls of that. The reason is, nsTextEditorState doesn't use mCachedValue with <input>. I think that it's worthwhile to use it with any editable elements.

I wonder, IMEContentObserver may not need to observe mutations by itself since nsTextEditorState will observe all mutations of <input> and <textarea>. However, changing it needs more changes than I expected and it still needs to be a mutation observer for HTMLEditor. Therefore, even if we do this hack, we might be able to improve the performance of moving focus to/from <input> and <textarea> only a bit. So, I don't want to do that in this bug. If we'll find that it's a bottleneck of something, I'd work on it.

Comment 1

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=93080e6f531659277a704e2d74dd4c8de6f8de43

Comment 2

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

Attachment: Bug 1395146 - part1: Get rid of nsITextControlElement::IsPlainTextControl() and nsTextEditorState::IsPlainTextEditor()

nsTextEditorState::GetValue() refers nsITextControlElement::IsPlainTextControl()
via nsTextEditorState::IsPlainTextEditor().  However, it always returns true and
virtual call with QI.  So, we should get rid of these unnecessary methods.

Review commit: https://reviewboard.mozilla.org/r/174378/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174378/

Comment 3

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

Attachment: Bug 1395146 - part2: Make nsTextEditorState cache the value of TextEditor with mCachedValue even when the editor is single line text control

Currently, nsTextEditorState caches the value of TextEditor only when it's for
a multi-line text control, i.e., <textarea>.  However, using it for single-line
text control improves the score of attachment 8848015 [details].  Although this might
increase the cost of creating and registering mutation observer, but it may not
make damage to performance of loading pages since editor for each element won't
be initialized until each element gets focus.

Review commit: https://reviewboard.mozilla.org/r/174380/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174380/

Comment 4

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

Attachment: Bug 1395146 - part3: nsTextEditorState should cache empty value with mCachedValue

nsTextEditorState::GetValue() uses mCachedValue only when the value isn't empty
string.  However, with SetIsVoid() and IsVoid() of nsAString, nsTextEditorState
can cache empty value only with mCachedValue.

Review commit: https://reviewboard.mozilla.org/r/174382/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174382/

Comment 5

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

Comment on attachment 8902734 [details]
Bug 1395146 - part1: Get rid of nsITextControlElement::IsPlainTextControl() and nsTextEditorState::IsPlainTextEditor()

https://reviewboard.mozilla.org/r/174378/#review179624

Odd old code.

Comment 6

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

Comment on attachment 8902735 [details]
Bug 1395146 - part2: Make nsTextEditorState cache the value of TextEditor with mCachedValue even when the editor is single line text control

https://reviewboard.mozilla.org/r/174380/#review179630

We should probably merge nsAnonDivObserver to nsTextEditorState to reduce memory usage and extra new/delete, but that can happen in a separate bug.

::: commit-message-274f5:1
(Diff revision 1)
> +Bug 1395146 - part2: Make nsTextEditorState cache the value of TextEditor with mCachedValue even when the editor isn't single line text control r?ehsan

This comment talks about "even when ... isn't single line text control"

::: commit-message-274f5:3
(Diff revision 1)
> +Bug 1395146 - part2: Make nsTextEditorState cache the value of TextEditor with mCachedValue even when the editor isn't single line text control r?ehsan
> +
> +Currently, nsTextEditorState caches the value of TextEditor only when it's for

but this about multi-line.
I guess this latter is correct, and the first sentence in the comment should have
s/isn't/is/

Comment 7

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

Comment on attachment 8902736 [details]
Bug 1395146 - part3: nsTextEditorState should cache empty value with mCachedValue

https://reviewboard.mozilla.org/r/174382/#review179644

Comment 8

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

Comment on attachment 8902735 [details]
Bug 1395146 - part2: Make nsTextEditorState cache the value of TextEditor with mCachedValue even when the editor is single line text control

https://reviewboard.mozilla.org/r/174380/#review179630

> but this about multi-line.
> I guess this latter is correct, and the first sentence in the comment should have
> s/isn't/is/

Ah, yes. Exactly.

Comment 9

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

Comment on attachment 8902734 [details]
Bug 1395146 - part1: Get rid of nsITextControlElement::IsPlainTextControl() and nsTextEditorState::IsPlainTextEditor()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174378/diff/1-2/

Comment 10

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

Comment on attachment 8902735 [details]
Bug 1395146 - part2: Make nsTextEditorState cache the value of TextEditor with mCachedValue even when the editor is single line text control

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174380/diff/1-2/

Comment 11

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

Comment on attachment 8902736 [details]
Bug 1395146 - part3: nsTextEditorState should cache empty value with mCachedValue

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174382/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd95ed2a8abc
part1: Get rid of nsITextControlElement::IsPlainTextControl() and nsTextEditorState::IsPlainTextEditor() r=smaug
https://hg.mozilla.org/integration/autoland/rev/bd7325a8425e
part2: Make nsTextEditorState cache the value of TextEditor with mCachedValue even when the editor is single line text control r=smaug
https://hg.mozilla.org/integration/autoland/rev/929846a88603
part3: nsTextEditorState should cache empty value with mCachedValue r=smaug

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/bd95ed2a8abc
https://hg.mozilla.org/mozilla-central/rev/bd7325a8425e
https://hg.mozilla.org/mozilla-central/rev/929846a88603