Closed
Bug 1180417
Opened 9 years ago
Closed 9 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)
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)
2.46 MB,
text/html
|
Details | |
282.90 KB,
text/html
|
Details | |
1.73 KB,
patch
|
smontagu
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.09 KB,
patch
|
smontagu
:
review+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
I can confirm this. Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:42.0) Gecko/20100101 Firefox/42.0
Updated•9 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: reproducible
Comment 2•9 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 4•9 years ago
|
||
Comment 5•9 years ago
|
||
Updated•9 years ago
|
Assignee: nobody → tclancy
Whiteboard: [systemsfe]
Target Milestone: --- → FxOS-S2 (10Jul)
Comment 7•9 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•9 years ago
|
Flags: needinfo?(smontagu)
Assignee | ||
Comment 8•9 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•9 years ago
|
||
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•9 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•9 years ago
|
||
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)
Assignee | ||
Comment 12•9 years ago
|
||
Green treeherder run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=817f40c525bb
Comment 13•9 years ago
|
||
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•9 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•9 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•9 years ago
|
||
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•9 years ago
|
||
This is what Part 3 of Bug 1164693 should have been.
Attachment #8635650 -
Flags: review?(smontagu)
Assignee | ||
Comment 18•9 years ago
|
||
Treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e04561b586f
Updated•9 years ago
|
Attachment #8635649 -
Flags: review?(smontagu) → review+
Updated•9 years ago
|
Attachment #8635650 -
Flags: review?(smontagu) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 19•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0474042a4c55 https://hg.mozilla.org/integration/mozilla-inbound/rev/7030bc6cba0b
Keywords: checkin-needed
Comment 20•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0474042a4c55 https://hg.mozilla.org/mozilla-central/rev/7030bc6cba0b
Status: NEW → RESOLVED
Closed: 9 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)
Updated•9 years ago
|
Flags: needinfo?(smontagu)
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•9 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.
Assignee | ||
Comment 28•9 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•9 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+
https://hg.mozilla.org/releases/mozilla-beta/rev/d9ea08608e05 https://hg.mozilla.org/releases/mozilla-beta/rev/b471fe2fa701
Updated•9 years ago
|
Flags: qe-verify+
Comment 32•9 years ago
|
||
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+
Comment 33•9 years ago
|
||
Verified as fixed using Firefox 43 beta 1 under Win 7 64-bit and Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•