Closed Bug 1180417 Opened 5 years ago Closed 5 years ago

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

Categories

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

41 Branch
x86_64
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla42
Tracking Status
firefox40 --- unaffected
firefox41 + verified
firefox42 + verified

People

(Reporter: nissan4321, Assigned: tedders1)

References

Details

(Keywords: perf, regression, reproducible, Whiteboard: [systemsfe])

Attachments

(4 files, 2 obsolete files)

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).
OS: Unspecified → Windows 8.1
Hardware: Unspecified → x86_64
I can confirm this.

Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:42.0) Gecko/20100101 Firefox/42.0
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: reproducible
[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
Blocks: 1164693
Component: Untriaged → Layout: Text
Flags: needinfo?(tclancy)
Keywords: perf, regression
Product: Firefox → Core
Summary: Entering a lot of rows at once into a textarea is horrendously slow on FF compared to Chrome → Significant perf regression, Entering a lot of rows at once into a textarea is horrendously slow on FF compared to Chrome
Version: Trunk → 41 Branch
Reproduce on Ubuntu14.04
OS: Windows 8.1 → All
Attached file test 76221 rows
Attached file test 15000 rows
Assignee: nobody → tclancy
Whiteboard: [systemsfe]
Target Milestone: --- → FxOS-S2 (10Jul)
Tracking 41, 42, see comment 2.
Because this is severe performance regression, I think it is time to back out the offending patch from Aurora41.0a2 and Nightly42.0a1.
Target Milestone: FxOS-S2 (10Jul) → ---
Flags: needinfo?(smontagu)
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.
Attached patch bug-1180417-fix.patch (obsolete) — Splinter Review
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?
Flags: needinfo?(tclancy)
Attachment #8634864 - Flags: review?(smontagu)
Comment on attachment 8634864 [details] [diff] [review]
bug-1180417-fix.patch

Oh wait. Now I remember what was up.
Attachment #8634864 - Flags: review?(smontagu)
Attached patch bug-1180417-fix.patch (obsolete) — Splinter Review
Okay, this puts back one of the optimizations that was removed by Part 3 of Bug 1164693, and fixes it so that it works.
Attachment #8634864 - Attachment is obsolete: true
Attachment #8635209 - Flags: review?(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?
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.)
> 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.
This reverts Part 3 of Bug 1164693.
Attachment #8635209 - Attachment is obsolete: true
Attachment #8635209 - Flags: review?(smontagu)
Attachment #8635649 - Flags: review?(smontagu)
This is what Part 3 of Bug 1164693 should have been.
Attachment #8635650 - Flags: review?(smontagu)
Attachment #8635649 - Flags: review?(smontagu) → review+
Attachment #8635650 - Flags: review?(smontagu) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0474042a4c55
https://hg.mozilla.org/mozilla-central/rev/7030bc6cba0b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Ted, do you think this is safe to land on Aurora? Or should we just let this ride the trains? Thanks.
Flags: needinfo?(tclancy)
Flags: needinfo?(smontagu)
Yeah, I reckon this is fine for Aurora.
Flags: needinfo?(tclancy)
In that case, can you please request uplift to Aurora on your patches? Thanks.
Flags: needinfo?(tclancy)
Mikel, could you please verify the fix on a Nightly build from 07-21-15 or later? Thanks!
Flags: needinfo?(nissan4321)
(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!
Flags: needinfo?(nissan4321)
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
Flags: needinfo?(tclancy)
Attachment #8635649 - Flags: approval-mozilla-aurora?
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
Attachment #8635650 - Flags: approval-mozilla-aurora?
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.
Attachment #8635650 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Flags: qe-verify+
Comment on attachment 8635649 [details] [diff] [review]
bug-1180417-fix-part-1.patch

Just like with beta, taking it.
Attachment #8635649 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed using Firefox 43 beta 1 under Win 7 64-bit and Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
Depends on: 1246449
No longer depends on: 1246449
You need to log in before you can comment on or make changes to this bug.