bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

VERIFIED FIXED in Firefox 41

Status

()

Core
Layout: Text
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Mikel, Assigned: tedders1)

Tracking

({perf, regression, reproducible})

41 Branch
mozilla42
x86_64
All
perf, regression, reproducible
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox40 unaffected, firefox41+ verified, firefox42+ verified)

Details

(Whiteboard: [systemsfe])

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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).
(Reporter)

Updated

3 years ago
OS: Unspecified → Windows 8.1
Hardware: Unspecified → x86_64

Comment 1

3 years ago
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
(Reporter)

Updated

3 years ago
Keywords: reproducible

Comment 2

3 years ago
[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
status-firefox40: --- → unaffected
status-firefox41: --- → affected
status-firefox42: --- → affected
tracking-firefox41: --- → ?
tracking-firefox42: --- → ?
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

Comment 3

3 years ago
Reproduce on Ubuntu14.04
OS: Windows 8.1 → All

Comment 4

3 years ago
Created attachment 8629942 [details]
test 76221 rows

Comment 5

3 years ago
Created attachment 8629945 [details]
test 15000 rows
Assignee: nobody → tclancy
Whiteboard: [systemsfe]
Target Milestone: --- → FxOS-S2 (10Jul)

Comment 6

3 years ago
Tracking 41, 42, see comment 2.
tracking-firefox41: ? → +
tracking-firefox42: ? → +

Comment 7

3 years ago
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) → ---

Updated

3 years ago
Flags: needinfo?(smontagu)
(Assignee)

Comment 8

3 years ago
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.
(Assignee)

Comment 9

3 years ago
Created attachment 8634864 [details] [diff] [review]
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?
Flags: needinfo?(tclancy)
Attachment #8634864 - Flags: review?(smontagu)
(Assignee)

Comment 10

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

Oh wait. Now I remember what was up.
Attachment #8634864 - Flags: review?(smontagu)
(Assignee)

Comment 11

3 years ago
Created attachment 8635209 [details] [diff] [review]
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.
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?
(Assignee)

Comment 14

3 years ago
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.)
(Assignee)

Comment 15

3 years ago
> 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.
(Assignee)

Comment 16

3 years ago
Created attachment 8635649 [details] [diff] [review]
bug-1180417-fix-part-1.patch

This reverts Part 3 of Bug 1164693.
Attachment #8635209 - Attachment is obsolete: true
Attachment #8635209 - Flags: review?(smontagu)
Attachment #8635649 - Flags: review?(smontagu)
(Assignee)

Comment 17

3 years ago
Created attachment 8635650 [details] [diff] [review]
bug-1180417-fix-part-2.patch

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+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0474042a4c55
https://hg.mozilla.org/mozilla-central/rev/7030bc6cba0b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Duplicate of this bug: 1185159
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)
(Assignee)

Comment 23

3 years ago
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)
(Reporter)

Comment 26

3 years ago
(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)
Verified on Nightly42 as per comment 26.
status-firefox42: fixed → verified
(Assignee)

Comment 28

3 years ago
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?
(Assignee)

Comment 29

3 years ago
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
status-firefox41: fixed → verified

Updated

3 years ago
Depends on: 1246449

Updated

3 years ago
No longer depends on: 1246449
You need to log in before you can comment on or make changes to this bug.