Last Comment Bug 743819 - Spell checking causes performance degradation in Firefox 10+
: Spell checking causes performance degradation in Firefox 10+
Status: RESOLVED FIXED
[qa+]
: regression
Product: Core
Classification: Components
Component: Spelling checker (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on: 923376
Blocks: 674212
  Show dependency treegraph
 
Reported: 2012-04-09 14:16 PDT by albright
Modified: 2013-10-07 10:02 PDT (History)
12 users (show)
ayg: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified


Attachments
spellcheckperfbug.html (1.86 KB, text/html)
2012-04-09 14:16 PDT, albright
no flags Details
Patch v1 (2.82 KB, patch)
2012-04-24 03:55 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review

Description albright 2012-04-09 14:16:58 PDT
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).
Comment 1 Matthias Versen [:Matti] 2012-04-09 16:45:13 PDT
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 ?
Comment 2 Alice0775 White 2012-04-10 20:28:10 PDT
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
Comment 3 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-23 15:43:47 PDT
Aryeh, can you take a look at this, please?
Comment 4 :Aryeh Gregor (away until August 15) 2012-04-23 23:55:06 PDT
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?
Comment 5 :Aryeh Gregor (away until August 15) 2012-04-24 03:55:59 PDT
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.
Comment 6 Mozilla RelEng Bot 2012-04-24 04:00:11 PDT
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 Mozilla RelEng Bot 2012-04-24 09:16:05 PDT
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
Comment 8 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-24 15:24:26 PDT
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!
Comment 9 :Aryeh Gregor (away until August 15) 2012-04-24 22:57:07 PDT
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
Comment 10 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-25 07:45:33 PDT
https://hg.mozilla.org/mozilla-central/rev/733f13a5cca6
Comment 11 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-25 07:47:03 PDT
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.
Comment 12 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-25 11:59:26 PDT
Comment on attachment 617836 [details] [diff] [review]
Patch v1

[Triage comment]
Low risk, please go ahead.
Comment 14 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-26 14:12:53 PDT
Given this is fixed, is qawanted still needed?
Comment 15 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-26 14:24:21 PDT
No!
Comment 16 Virgil Dicu [:virgil] [QA] 2012-05-09 08:22:52 PDT
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.
Comment 17 Virgil Dicu [:virgil] [QA] 2012-06-07 08:53:20 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.