Bug 49040 – Pasting large amounts of plain text in a text area is very slow

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Enrica Casucci]

Select a large amount of plain text (~500k) from a file and copy
Open the attached file.
Paste into the text area.

Result: the browser hangs for several seconds.

[Enrica Casucci]

Created attachment 73006 [details]
Test

[Enrica Casucci]

Created attachment 73007 [details]
Patch

[Adele Peterson]

Comment on attachment 73007 [details]
Patch

Would we also want this optimization if we're pasting into any element that has user-modify: read-write-plaintext-only set?

[Alexey Proskuryakov]

See also: bug 34237 (a case of slow pasting that Enrica fixed a few months ago).

[Ryosuke Niwa]

Comment on attachment 73007 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73007&action=review

> WebCore/ChangeLog:11
> +        pasting into a text area. What we really avoid is iterating through
> +        a large number of paragraph when no style change is needed.

"What we really avoid is iterating through..." sounds confusing to me.  Maybe just "We avoid iterating through..."?

> WebCore/editing/ReplaceSelectionCommand.cpp:1164
> -        if (m_matchStyle) {
> +        if (m_matchStyle && !isNodeInTextFormControl(start.node())) {

Isn't better to set m_matchStyle=false right at the front if we're in a text form control?  Is there a case we need m_matchStyle=true even when we're in a text form control?

[Tony Chang]

It might be possible to write a perf test for this (seems like the time should be constant or linear).  See LayoutTests/resources/magnitude-perf.js and LayoutTests/perf.

[Enrica Casucci]

(In reply to comment #3)
> (From update of attachment 73007 [details])
> Would we also want this optimization if we're pasting into any element that has user-modify: read-write-plaintext-only set?

I'll look into it.

[Enrica Casucci]

(In reply to comment #5)
> (From update of attachment 73007 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73007&action=review
> 
> > WebCore/ChangeLog:11
> > +        pasting into a text area. What we really avoid is iterating through
> > +        a large number of paragraph when no style change is needed.
> 
> "What we really avoid is iterating through..." sounds confusing to me.  Maybe just "We avoid iterating through..."?
> 
Not sure that what you suggest is less confusing. I can rephrase this.
The point I'm trying to make is that the performance hit comes from the paragraph iteration.

> > WebCore/editing/ReplaceSelectionCommand.cpp:1164
> > -        if (m_matchStyle) {
> > +        if (m_matchStyle && !isNodeInTextFormControl(start.node())) {
> 
> Isn't better to set m_matchStyle=false right at the front if we're in a text form control?
I believe it makes no difference, this is where we apply the style, if we check here we skip applying the style.
  Is there a case we need m_matchStyle=true even when we're in a text form control?
No. In general we wouldn't need to apply the style when pasting plain text except for the case where we want to preserve the typing style.

[Enrica Casucci]

(In reply to comment #6)
> It might be possible to write a perf test for this (seems like the time should be constant or linear).  See LayoutTests/resources/magnitude-perf.js and LayoutTests/perf.

This fix doesn't change the performance from non-linear to linear. The performance is linear already.
Do you think this is still worth?

[Ryosuke Niwa]

(In reply to comment #8)
> No. In general we wouldn't need to apply the style when pasting plain text except for the case where we want to preserve the typing style.

Ok, in that case, it's cleaner to set m_matchStyle false if we're in a text form control.  When we don't need this extra condition in the if statement.  I think setting this either when we instantiates ReplaceSelectionCommand or right at the beginning of doApply will also prevent us from needlessly obtaining the computed style.

[Tony Chang]

(In reply to comment #9)
> (In reply to comment #6)
> > It might be possible to write a perf test for this (seems like the time should be constant or linear).  See LayoutTests/resources/magnitude-perf.js and LayoutTests/perf.
> 
> This fix doesn't change the performance from non-linear to linear. The performance is linear already.
> Do you think this is still worth?

It might be fast enough to show up as constant even though it's technically linear.  Another way to think of it is testing to make sure that pasting 500k of text always takes less than X ms.

I'm not sure that magnitude-perf.js will handle this type of test well, Ojan, what do you think?

[Ryosuke Niwa]

(In reply to comment #11)
> It might be fast enough to show up as constant even though it's technically linear.  Another way to think of it is testing to make sure that pasting 500k of text always takes less than X ms.

I'm afraid setting a hard time limit will make this test flaky. I'm okay with not having a test for this change.

[Ojan Vafai]

(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #6)
> > > It might be possible to write a perf test for this (seems like the time should be constant or linear).  See LayoutTests/resources/magnitude-perf.js and LayoutTests/perf.
> > 
> > This fix doesn't change the performance from non-linear to linear. The performance is linear already.
> > Do you think this is still worth?
> 
> It might be fast enough to show up as constant even though it's technically linear.  Another way to think of it is testing to make sure that pasting 500k of text always takes less than X ms.
> 
> I'm not sure that magnitude-perf.js will handle this type of test well, Ojan, what do you think?

The magnitude perf stuff is still pretty experimental (e.g. these tests are skipped on non-chromium bots right now). I welcome people using it and writing new tests, but it's still on my todo list to make the tests more reliable. Given that this patch is not changing the order of magnitude, I probably wouldn't bother in this case.

(In reply to comment #12)
> (In reply to comment #11)
> > It might be fast enough to show up as constant even though it's technically linear.  Another way to think of it is testing to make sure that pasting 500k of text always takes less than X ms.
> 
> I'm afraid setting a hard time limit will make this test flaky. I'm okay with not having a test for this change.

We could at least have a manual test. It's not ideal, but it gives a comparison point that can be used to identify regressions between nightly builds. Eventually, if we have a performance testing framework, we can move this test into it.

[Enrica Casucci]

Created attachment 73111 [details]
Patch2

This is a new patch that addresses Adele's and Ryosuke's comments.
I don't think we need another layout test for this fix, since there are several tests that verify the correct behavior in this case.

[Ryosuke Niwa]

Comment on attachment 73111 [details]
Patch2

View in context: https://bugs.webkit.org/attachment.cgi?id=73111&action=review

> editing/ReplaceSelectionCommand.cpp:804
> +    if (isNodeInTextFormControl(selection.start().node()) || (selection.start().node()->renderer() && selection.start().node()->renderer()->style()->userModify() == READ_WRITE_PLAINTEXT_ONLY))
> +        m_matchStyle = false;

Maybe we should also check the selection end?  Or is there guarantee that this would never happen?

[Adele Peterson]

Comment on attachment 73111 [details]
Patch2

View in context: https://bugs.webkit.org/attachment.cgi?id=73111&action=review

>> editing/ReplaceSelectionCommand.cpp:804
>> +        m_matchStyle = false;
> 
> Maybe we should also check the selection end?  Or is there guarantee that this would never happen?

Is there a need to still do the isNodeInTextFormControl check?

[Enrica Casucci]

Created attachment 73142 [details]
Patch3

All tests pass.

[Adele Peterson]

Comment on attachment 73142 [details]
Patch3

View in context: https://bugs.webkit.org/attachment.cgi?id=73142&action=review

> ChangeLog:19
> +        and force no style matching.

You should update this comment.

[Ryosuke Niwa]

(In reply to comment #17)
> Created an attachment (id=73142) [details]
> Patch3
> 
> All tests pass.

Your patch is made in WebCore/.  Could you remake it in /?

[Ryosuke Niwa]

Comment on attachment 73142 [details]
Patch3

View in context: https://bugs.webkit.org/attachment.cgi?id=73142&action=review

> editing/ReplaceSelectionCommand.cpp:805
> +    if ((selection.start().node()->renderer() && selection.start().node()->renderer()->style()->userModify() == READ_WRITE_PLAINTEXT_ONLY) &&
> +        (selection.end().node()->renderer() && selection.end().node()->renderer()->style()->userModify() == READ_WRITE_PLAINTEXT_ONLY))
> +        m_matchStyle = false;

I'm just curious as to why this is superior to checking that it's in the text form control.  Could you elaborate?  Of course, this patch LGTM nonetheless.

[Adele Peterson]

Its superior because this optimization will apply to other plaintext editable areas besides form controls.
 
(In reply to comment #20)
> (From update of attachment 73142 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73142&action=review
> 
> > editing/ReplaceSelectionCommand.cpp:805
> > +    if ((selection.start().node()->renderer() && selection.start().node()->renderer()->style()->userModify() == READ_WRITE_PLAINTEXT_ONLY) &&
> > +        (selection.end().node()->renderer() && selection.end().node()->renderer()->style()->userModify() == READ_WRITE_PLAINTEXT_ONLY))
> > +        m_matchStyle = false;
> 
> I'm just curious as to why this is superior to checking that it's in the text form control.  Could you elaborate?  Of course, this patch LGTM nonetheless.

[Ryosuke Niwa]

(In reply to comment #21)
> Its superior because this optimization will apply to other plaintext editable areas besides form controls.

Right, but what are they?  What uses editable region other than contenteditable and forms?

[Adele Peterson]

Any element could have this style.  Just like any element could set "-webkit-user-modify: read-write" to get rich text editing.

(In reply to comment #22)
> (In reply to comment #21)
> > Its superior because this optimization will apply to other plaintext editable areas besides form controls.
> 
> Right, but what are they?  What uses editable region other than contenteditable and forms?

[Ryosuke Niwa]

(In reply to comment #23)
> Any element could have this style.  Just like any element could set "-webkit-user-modify: read-write" to get rich text editing.

That makes sense.  Thanks for the information.

[Enrica Casucci]

Committed revision 71459.