The default bug view has changed. See this FAQ.

Spell checking causes performance degradation in Firefox 10+

RESOLVED FIXED in Firefox 13

Status

()

Core
Spelling checker
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: albright, Assigned: ayg)

Tracking

({regression})

Trunk
mozilla15
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox13 verified, firefox14 verified)

Details

(Whiteboard: [qa+])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Created attachment 613391 [details]
spellcheckperfbug.html

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/536.5 (KHTML, like Gecko) Chrome/19.0.1084.15 Safari/536.5

Steps to reproduce:

The attached web page demonstrates the problem. A large table is constructed on the page and two fixed position content editable divs are synchronized via an onkeypress handler. Typing in the left input is copied to the one on the right on each key press.


Actual results:

Typing speed is very laggy - its easy to get ahead of the UI making keyboard access very slow.


Expected results:

This test runs very well in FF 9. Also, there is a button on the right of the page that will turn off spell checking at the document level (disabling it in the content editables). This causes the performance to return to normal. One note, a key difference between FF 9 and 10+ is that content in the destination content editable is spell checked immediately  in FF 10+ whereas it was not in FF 9. This bug also seems to be dependent on the total amount of DOM on the page (the size of the background table).
Component: Untriaged → Spelling checker
Product: Firefox → Core
QA Contact: untriaged → spelling-checker
Keywords: regressionwindow-wanted
Keywords: qawanted
Attachment #613391 - Attachment mime type: text/plain → text/html
Last good nightly: 2011-08-17
First bad nightly: 2011-08-18

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dcb25d71220d&tochan
ge=f69a10f23bf3

bug 674212 ?
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

5 years ago
Regression window(cached m-i)
Cannot reproduce
http://hg.mozilla.org/integration/mozilla-inbound/rev/05268baefef7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110817 Firefox/9.0a1 ID:20110817115336
Can reproduce
http://hg.mozilla.org/integration/mozilla-inbound/rev/800f7541fb20
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110817 Firefox/9.0a1 ID:20110817141601
Pushlog
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=05268baefef7&tochange=800f7541fb20

In local build
Last good ;2c5ef30a89b2
First bad :800f7541fb20

Triggered by:
800f7541fb20	Fabien Cazenave — Bug 674212 - Modifying text of a contenteditable DOM Node removes spellcheck underlinings; r=ehsan
Blocks: 674212
Keywords: regressionwindow-wanted → regression
OS: Linux → All
Version: 10 Branch → Trunk
Aryeh, can you take a look at this, please?
Well, the problem seems to be that bug 674212 calls nsEditor::SyncRealTimeSpell(), which calls nsIInlineSpellChecker::SetEnableRealTimeSpell(), which is meant to be used when spellcheck is first initialized.  That calls nsIInlineSpellChecker::SpellCheckRange(nsnull), which just re-spellchecks everything.  If we knew what range changed, we could call SpellCheckRange() directly with an appropriate range so it only has to recheck that.  But nsHTMLEditRules::DocumentModifiedWorker doesn't seem to know about what nodes changed.  Is there some way for it to find out?  Maybe the re-spellchecking can be moved to some other method?
Created attachment 617836 [details] [diff] [review]
Patch v1

This seems to work.  I take it it doesn't need a test, because it's perf-only.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #617836 - Flags: review?(ehsan)
Hardware: x86_64 → All
Whiteboard: [autoland-try:-u all]

Updated

5 years ago
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]

Comment 6

5 years ago
Autoland Patchset:
	Patches: 617836
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=27ae8cb68de3
Try run started, revision 27ae8cb68de3. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=27ae8cb68de3

Comment 7

5 years ago
Try run for 27ae8cb68de3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=27ae8cb68de3
Results (out of 224 total builds):
    exception: 1
    success: 182
    warnings: 38
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-27ae8cb68de3

Updated

5 years ago
Whiteboard: [autoland-in-queue]
Comment on attachment 617836 [details] [diff] [review]
Patch v1

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

This looks good, but please verify that it doesn't regress bug 674212 before landing.  Thanks!
Attachment #617836 - Flags: review?(ehsan) → review+
Isn't that what the reftest is for?  Manually checking the original test-case seems to show that it still works fine too.

https://hg.mozilla.org/integration/mozilla-inbound/rev/733f13a5cca6
Flags: in-testsuite-
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/733f13a5cca6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Comment on attachment 617836 [details] [diff] [review]
Patch v1

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

This is a safe fix for a perf regression since Firefox 10.  I'd like to take it on Aurora and Beta if possible.
Attachment #617836 - Flags: approval-mozilla-beta?
Attachment #617836 - Flags: approval-mozilla-aurora?
Comment on attachment 617836 [details] [diff] [review]
Patch v1

[Triage comment]
Low risk, please go ahead.
Attachment #617836 - Flags: approval-mozilla-beta?
Attachment #617836 - Flags: approval-mozilla-beta+
Attachment #617836 - Flags: approval-mozilla-aurora?
Attachment #617836 - Flags: approval-mozilla-aurora+
http://hg.mozilla.org/releases/mozilla-aurora/rev/6c55625149dd
http://hg.mozilla.org/releases/mozilla-beta/rev/f9adcedc0411
status-firefox13: --- → fixed
status-firefox14: --- → fixed
Given this is fixed, is qawanted still needed?
Whiteboard: [qa+]
No!
Keywords: qawanted
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0

Verified in F13b3 using attached test case on Mac OS 10.6, Ubuntu 12.04 and Windows 7. Typing speed normal, the same when toggling spell checker on/off.
status-firefox13: fixed → verified
Mozilla/5.0 (X11; Linux i686; rv:14.0) Gecko/20100101 Firefox/14.0

Also verified in F14 on same platforms, using the same test case.
status-firefox14: fixed → verified

Updated

4 years ago
Depends on: 923376
You need to log in before you can comment on or make changes to this bug.