743819 - Spell checking causes performance degradation in Firefox 10+

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[albright]

Attachment: 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]]

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]

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

[(no longer active)]

Aryeh, can you take a look at this, please?

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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 (:ayg) (no longer with Mozilla)]

Attachment: 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]

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]

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/[e-mail]-27ae8cb68de3

Comment 8

[(no longer active)]

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 (:ayg) (no longer with Mozilla)]

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

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/733f13a5cca6

Comment 11

[(no longer active)]

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]

Comment on attachment 617836 [details] [diff] [review]
Patch v1

[Triage comment]
Low risk, please go ahead.

Comment 13

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/6c55625149dd
http://hg.mozilla.org/releases/mozilla-beta/rev/f9adcedc0411

Comment 14

[u279076]

Given this is fixed, is qawanted still needed?

Comment 15

[(no longer active)]

No!

Comment 16

[Virgil Dicu [:virgil] [QA]]

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]]

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.