596501 - Make TextEditor::OutputToString faster

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Boris Zbarsky [:bzbarsky]]

For the typical case (not password input, not wrap="hard" textarea, etc), we should be able to just hand back the string inside our one textnode, I would think.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Ehsan, do you have time to take a look at this?

Comment 2

[(no longer active)]

I'm not sure I understand what you're proposing here.  Isn't that what we already do, through nsDocumentEncoder?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yes, with giant amounts of overhead.  I'm proposing we skip the overhead.

Comment 4

[(no longer active)]

(In reply to comment #3)
> Yes, with giant amounts of overhead.  I'm proposing we skip the overhead.

Do you have a profile on what takes too much time in this process?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Sure thing.  On the testcase in bug 190147, the time under OutputToString breaks down as follows (all percentages are of time under OutputToString; "in" means in the function, "under" means in the function or some function it calls):

4% setting up the document encoder, QIs, etc getting to call
   nsPlainTextSerializer::AppendText.
3% FindCharInset called from Appendtext
3% nsDependentSubstring::Rebind called from AppendText
2% nsString::AssignWithConversion called from AppendText
2% in AppendText itself
4% in nsPlainTextSerializer::DoAddLeaf.
48% under nsPlainTextSerializer::EndLine (calling Replace, calling SetLength,
    calling OutputQuotesAndIndent, etc) (why are the Replace and SetLength
    calls happening at all?)
4% in nsPlainTextSerializer::Write
3% FindCharInSet called from Write()
7% nsAString::Assign called from Write()
19% under nsPlaintextSerializer::AddToLine; almost all of this is calling
    Replace(); again not sure why we need this.

Comment 6

[(no longer active)]

OK, sold!

Comment 7

[(no longer active)]

I'm probably going to take a look at this post Firefox 4, so if someone else wants to steal it from me before then, I won't call the cops! ;-)

Comment 8

[Boris Zbarsky [:bzbarsky]]

It'd still be nice to understand why those Replace and so forth happen, btw... and especially why they actually resize the strings!

Comment 9

[(no longer active)]

I'm not actively working on this...

Comment 10

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ecbc3b280b47750ed80f36a2d642a9e8d96882f

Comment 11

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

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

Comment 12

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

I guess that this performance issue is not so appear in profiles dynamically. However, this is used by autocomplete at every type, caching the value in nsTextEditorState and caching change event when moving focus to editor.

So, some users could feel like this bug as "Firefox responds not so quickly at typing something". I'm not sure how to detect such similar issues.

Comment 13

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

Attachment: Bug 596501 TextEditRules::WillOutputText() should handle itself if it can return only the text of first text node

TextEditor::OutputToString() uses DocumentEncoder if TextEditRules::WillOutputText() doesn't handle it.  However, TextEditRules::WillOutputText() doesn't handle it even if it's really simple case of <input type="text"> and <textarea>.
This patch makes TextEditRules::WillOutputText() handle it if DOM tree in the editor root element is expected and there is no special flag which requires complicated handling.

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

Comment 14

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

The patch improves the score of attachment 8848015 [details] about 10%.

Comment 15

[Makoto Kato [:m_kato]]

Comment on attachment 8879213 [details]
Bug 596501 TextEditRules::WillOutputText() should handle itself if it can return only the text of first text node

https://reviewboard.mozilla.org/r/150536/#review155446

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36a05246c18c
TextEditRules::WillOutputText() should handle itself if it can return only the text of first text node r=m_kato

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/36a05246c18c