1180417 - Significant perf regression, Entering a lot of rows at once into a textarea is horrendously slow on FF compared to Chrome

Status: VERIFIED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Mikel]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20150704030210

Steps to reproduce:

1) Open https://secure.fanboy.co.nz/r/fanboy-ultimate.txt and copy all the text (Ctrl + A)
2) Enter https://adblockplus.org/en/redundancy_check and paste the copied text in the textarea.
3) Wait for the site to become responsive (approximately ~30 seconds on my machine until the textarea got filled and the site got responsive).

*Chrome gets the tab responsive again within a couple of seconds.


Actual results:

The filling of the textarea with a lot of content is horrendously slow and makes the tab unresponsive untill it's all filled.


Expected results:

The insertion should be faster (in couple of seconds, not dozen of seconds).

Comment 1

[Gary [:streetwolf52]]

I can confirm this.

Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:42.0) Gecko/20100101 Firefox/42.0

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ad46ffd4c500&tochange=b232c2a5af6d

Regressed by: Bug 1164693

Comment 3

[Alice0775 White]

Reproduce on Ubuntu14.04

Comment 4

[Alice0775 White]

Attachment: test 76221 rows

Comment 5

[Alice0775 White]

Attachment: test 15000 rows

Comment 6

[Kate Glazko]

Tracking 41, 42, see comment 2.

Comment 7

[Alice0775 White]

Because this is severe performance regression, I think it is time to back out the offending patch from Aurora41.0a2 and Nightly42.0a1.

Comment 8

[Ted Clancy [:tedders1]]

Wait wait wait, give me one more day. I was sick yesterday, and I'm PTO today.

You can back out Bug 1164693 if I don't have a fix for this by end of tomorrow.

Comment 9

[Ted Clancy [:tedders1]]

Attachment: bug-1180417-fix.patch

Okay, this patch just undoes the changes from Part 3 of Bug 1164693.

The thing is, the reason I made the changes in Part 3 of Bug 1164693 is because tests were breaking. But now I don't see any tests breaking. I guess we changed the way preformatted text is handled?

Comment 10

[Ted Clancy [:tedders1]]

Comment on attachment 8634864 [details] [diff] [review]
bug-1180417-fix.patch

Oh wait. Now I remember what was up.

Comment 11

[Ted Clancy [:tedders1]]

Attachment: bug-1180417-fix.patch

Okay, this puts back one of the optimizations that was removed by Part 3 of Bug 1164693, and fixes it so that it works.

Comment 12

[Ted Clancy [:tedders1]]

Green treeherder run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=817f40c525bb

Comment 13

[Simon Montagu :smontagu]

Comment on attachment 8635209 [details] [diff] [review]
bug-1180417-fix.patch

Review of attachment 8635209 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrame.cpp
@@ -3975,5 @@
>    // Remove any NewlineOffsetProperty or InFlowContentLengthProperty since they
>    // might be invalid if the content was modified while there was no frame
>    aContent->DeleteProperty(nsGkAtoms::newline);
> -  if (PresContext()->BidiEnabled()) {
> -    aContent->DeleteProperty(nsGkAtoms::flowlength);

Why is this an optimization? The test for BidiEnabled() was supposed to be an optimization in the first place, see bug 566066 comment 19. That was the theory at least, but was there a reason why it actually made performance worse?

Comment 14

[Ted Clancy [:tedders1]]

No, that's not the bit I'm talking about :-)

When I said "optimization", I mean caching the end position of the current flow in a node property (nsGkAtoms::flowlength). In Part 3 of Bug 1164693, I removed the optimization for preformatted text, because it was giving the wrong answer. (I shouldn't have done that.) This patch puts it back.

This is the relevant part of the patch:

>    * start of the cached flow.
>    */
> -  if (flowLength && !StyleText()->NewlineIsSignificant(this) &&
> +  if (flowLength &&
>       (flowLength->mStartOffset < mContentOffset ||
>        (flowLength->mStartOffset == mContentOffset && GetContentEnd() > mContentOffset)) &&


So why was the cached value wrong? The problem is that the cached value wasn't being deleted in CharacterDataChanged(). And that's because the guard around the deletion of the node property was wrong.

This is the change that needed to be made:

> -  if (PresContext()->BidiEnabled()) {
> -    aContent->DeleteProperty(nsGkAtoms::flowlength);
> -  }
> +  aContent->DeleteProperty(nsGkAtoms::flowlength);

It was checking for PresContext()->BidiEnabled(). However, the node property isn't just used for bidi text. It's also now used for non-bidi, preformatted text. So I removed the guard.

(You might be wondering why we now cache the end position of the flow for non-bidi, preformatted text. I'll answer that in the next comment, just in case you're wondering.)

Comment 15

[Ted Clancy [:tedders1]]

> The test for BidiEnabled() was supposed to be an optimization in the first place, see bug 566066 comment 19.

Oh. Okay, I'll change the patch.

> You might be wondering why we now cache the end position of the flow for non-bidi, preformatted text. I'll answer that in the next comment, just in case you're wondering.

Actually, no, I started to answer that, but then realized it doesn't matter because I have to change the patch anyway.

Comment 16

[Ted Clancy [:tedders1]]

Attachment: bug-1180417-fix-part-1.patch

This reverts Part 3 of Bug 1164693.

Comment 17

[Ted Clancy [:tedders1]]

Attachment: bug-1180417-fix-part-2.patch

This is what Part 3 of Bug 1164693 should have been.

Comment 18

[Ted Clancy [:tedders1]]

Treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e04561b586f

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0474042a4c55
https://hg.mozilla.org/integration/mozilla-inbound/rev/7030bc6cba0b

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0474042a4c55
https://hg.mozilla.org/mozilla-central/rev/7030bc6cba0b

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Ted, do you think this is safe to land on Aurora? Or should we just let this ride the trains? Thanks.

Comment 23

[Ted Clancy [:tedders1]]

Yeah, I reckon this is fine for Aurora.

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

In that case, can you please request uplift to Aurora on your patches? Thanks.

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Mikel, could you please verify the fix on a Nightly build from 07-21-15 or later? Thanks!

Comment 26

[Mikel]

(In reply to Ritu Kothari (:ritu) from comment #25)
> Mikel, could you please verify the fix on a Nightly build from 07-21-15 or
> later? Thanks!

I can verify that the patch has fixed the perf problem.
Thank you!

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Verified on Nightly42 as per comment 26.

Comment 28

[Ted Clancy [:tedders1]]

Comment on attachment 8635649 [details] [diff] [review]
bug-1180417-fix-part-1.patch

Approval Request Comment
[Feature/regressing bug #]:
Bug 1164693

[User impact if declined]:
Pasting large amounts of text into text boxes will cause the browser to hang for several minutes.

[Describe test coverage new/current, TreeHerder]:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e04561b586f

[Risks and why]: 
None foreseen.

[String/UUID change made/needed]:
None

Comment 29

[Ted Clancy [:tedders1]]

Comment on attachment 8635650 [details] [diff] [review]
bug-1180417-fix-part-2.patch

Approval Request Comment
[Feature/regressing bug #]:
Bug 1164693

[User impact if declined]:
Pasting large amounts of text into text boxes will cause the browser to hang for several minutes.

[Describe test coverage new/current, TreeHerder]:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e04561b586f

[Risks and why]: 
None foreseen.

[String/UUID change made/needed]:
None

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8635650 [details] [diff] [review]
bug-1180417-fix-part-2.patch

[Triage Comment]
Fix was verified on Nightly. Should be safe to uplift to Beta41.

Comment 31

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

https://hg.mozilla.org/releases/mozilla-beta/rev/d9ea08608e05
https://hg.mozilla.org/releases/mozilla-beta/rev/b471fe2fa701

Comment 32

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8635649 [details] [diff] [review]
bug-1180417-fix-part-1.patch

Just like with beta, taking it.

Comment 33

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed using Firefox 43 beta 1 under Win 7 64-bit and Mac OS X 10.9.5.