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.
Created attachment 73006 [details] Test
Created attachment 73007 [details] Patch
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?
See also: bug 34237 (a case of slow pasting that Enrica fixed a few months ago).
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?
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.
(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.
(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.
(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?
(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.
(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?
(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.
(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.
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.
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?
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?
Created attachment 73142 [details] Patch3 All tests pass.
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.
(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 /?
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.
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.
(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?
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?
(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.
Committed revision 71459.