Last Comment Bug 684638 - Google Docs Spreadsheets Sluggish
: Google Docs Spreadsheets Sluggish
Status: VERIFIED FIXED
[qa!]
: perf, regression, verified-aurora, verified-beta
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86_64 Linux
: -- normal with 1 vote (vote)
: mozilla10
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on: 891904
Blocks: 635618 674212 696618
  Show dependency treegraph
 
Reported: 2011-09-04 17:03 PDT by Will Roberts [:bws42]
Modified: 2013-07-10 16:38 PDT (History)
17 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
backout - bug674212 (1.62 KB, patch)
2011-09-07 09:43 PDT, Fabien Cazenave [:kaze]
ehsan: review-
Details | Diff | Splinter Review
This drops my spellcheck times into the 30ms range, for example (2.30 KB, patch)
2011-11-01 22:35 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
backout - bug674212 (1.78 KB, patch)
2011-11-02 15:06 PDT, Fabien Cazenave [:kaze]
bzbarsky: review+
christian: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
part 1. Speed up IsBreakElement. (2.79 KB, patch)
2011-11-02 19:42 PDT, Boris Zbarsky [:bz]
ehsan: review+
Details | Diff | Splinter Review
part 2. Make it simpler to set up a range. (3.15 KB, patch)
2011-11-02 19:42 PDT, Boris Zbarsky [:bz]
bugs: review+
Details | Diff | Splinter Review
part 3. Speed up mozInlineSpellWordUtil::MakeRange. (2.26 KB, patch)
2011-11-02 19:43 PDT, Boris Zbarsky [:bz]
ehsan: review+
Details | Diff | Splinter Review
part 4. Switch from nsIDOMNode to nsINode in mozInlineSpellWordUtil. (16.84 KB, patch)
2011-11-02 19:43 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
part 5. When we have an event posted to spell-check the whole document, suppress other spell-check events. (5.01 KB, patch)
2011-11-02 20:16 PDT, Boris Zbarsky [:bz]
ehsan: review+
Details | Diff | Splinter Review
Part 4 with less crashing, more nsIRange. (34.61 KB, patch)
2011-11-02 23:18 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Part 4 with that fixed (34.54 KB, patch)
2011-11-03 00:01 PDT, Boris Zbarsky [:bz]
ehsan: review+
Details | Diff | Splinter Review

Description Will Roberts [:bws42] 2011-09-04 17:03:50 PDT
Entering text in spreadsheets and even navigating between the cells has been sluggish for a couple weeks for me. I've narrowed it down to the following:

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

STR:

1. Create a new spreadsheet in Google docs
2. Select a cell and quickly enter some text
3. Press and hold the Down arrow to quickly move down the column

On a build before the range the highlight pauses for a split second then continues, after the range the highlight pauses for a noticeably longer period then jumps down a number of cells.
Comment 1 Alice0775 White 2011-09-04 20:34:36 PDT
Confirmed on 
http://hg.mozilla.org/mozilla-central/rev/a351ae35f2c4
Mozilla/5.0 (X11; Linux i686; rv:9.0a1) Gecko/20110904 Firefox/9.0a1 ID:20110904030822

Regression window(cached m-i hourly),
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/de9252e8b8f4
Mozilla/5.0 (X11; Linux i686; rv:9.0a1) Gecko/20110817 Firefox/9.0a1 ID:20110817135845
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/800f7541fb20
Mozilla/5.0 (X11; Linux i686; rv:9.0a1) Gecko/20110817 Firefox/9.0a1 ID:20110817141601
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=de9252e8b8f4&tochange=800f7541fb20
Suspected bug:
800f7541fb20	Fabien Cazenave — Bug 674212 - Modifying text of a contenteditable DOM Node removes spellcheck underlinings; r=ehsan

And It become faster if disabled spell check.
Edit > Preferences > Advanced > Uncheck "Check my spelling as I type"
Comment 2 Fabien Cazenave [:kaze] 2011-09-07 09:43:33 PDT
Created attachment 558847 [details] [diff] [review]
backout - bug674212

Need to find a better approach than this: https://bugzilla.mozilla.org/show_bug.cgi?id=674212#c4

I’ve left the reftests, I’ve just removed the corresponding line in the reftest.list file.
Comment 4 :Ehsan Akhgari 2011-09-07 10:07:50 PDT
Comment on attachment 558847 [details] [diff] [review]
backout - bug674212

I'd like to first understand why this regression happens.  Have you profiled Firefox to see why it's behaving sluggish in this case?  We should fix the performance problem IMO.
Comment 5 :Ehsan Akhgari 2011-09-07 13:08:18 PDT
If the performance problem here is caused by a large number of spellchecking events, fixing bug 599074 might be the answer.
Comment 6 Boris Zbarsky [:bz] 2011-11-01 20:22:52 PDT
OK.  So I don't know whether I'm seeing the bug or not, but I have a fast machine.  I did try profiling arrowing up and down on a spreadsheet in a current build, and the vast majority of the time (72%) is spent under mozInlineSpellResume::Run.  The rest is painting.

Under spellcheck, there's 12% GetNextWord (creating ranges and setting their endpoints) and 55% mozInlineSpellWordUtil::SetPosition; this last largely calls BuildSoftText which spends a huge amount of time in IsBreakElement (45% of total time in this case is under IsBreakElement!).  IsBreakElement seems to be getting computed style on nodes that don't have frames or something: it's recomputing styles from scratch.  Is that expected?  If so, why are we doing it?

So we could well have lots of spellcheck events here.  Or each one could be slow to process (how much text are we checking, anyway?).  Or both.

In any case, this has been sitting in the tracking nom queue for almost 2 months.  If we're going to fix this in 9 one way or another, we need to seriously work on that.  Fabien, Ehsan, anything I can do to help this along?
Comment 7 Boris Zbarsky [:bz] 2011-11-01 22:17:41 PDT
So I added some code to increment and print a counter in mozInlineSpellResume::Post and to decrement and print it in mozInlineSpellResume::Run.

Scrolling with arrow keys through a spreadsheet in gdocs I can easily get to 150 events posted but not yet run.

On the particular spreadsheet I was looking at, each invocation of the runnable (so each call to ResumeCheck() takes about 80-90ms.

It looks like a runnable is posted for every nonempty cell I enter and for every nonempty cell I leave, by the way.  So going down about 50 rows will post about 100 runnables, which will take 8-9 seconds to all run...

But even if we fix bug 599074 each runnable taking 80-90 ms is a bit weird.  I need to look into why we're spending so much time under IsBreakElement.
Comment 8 Boris Zbarsky [:bz] 2011-11-01 22:26:16 PDT
So yeah, IsBreakElement is using computed display style to check whether something is a break element, and this is expensive for display:none objects.  And apparently we're spellchecking content that includes a bunch of them or something.  I still don't know whether we're just spellchecking the whole spreadsheet or not....

Rewriting IsBreakElement in terms of nsIContent and nsIFrame would, if ok, make it a _lot_ faster.
Comment 9 Boris Zbarsky [:bz] 2011-11-01 22:35:43 PDT
Created attachment 571243 [details] [diff] [review]
This drops my spellcheck times into the 30ms range, for example

What I don't know is whether that last test is correct.  The old code was clearly confused (e.g. checking for non-static position after it had determined that display is "inline", which can never happen), so I'm not sure what semantics it was really after.  Is this looking for things that cause a break _before_ them, or for things that separate content?  Specifically, what should this function return for an inline-block?
Comment 10 Bill Gianopoulos [:WG9s] 2011-11-02 04:07:35 PDT
(In reply to Boris Zbarsky (:bz) from comment #9)
> Created attachment 571243 [details] [diff] [review] [diff] [details] [review]
> This drops my spellcheck times into the 30ms range, for example
> 
> What I don't know is whether that last test is correct.  The old code was
> clearly confused (e.g. checking for non-static position after it had
> determined that display is "inline", which can never happen), so I'm not
> sure what semantics it was really after.  Is this looking for things that
> cause a break _before_ them, or for things that separate content? 
> Specifically, what should this function return for an inline-block?

That code was added by Bug 339066.
Comment 11 Fabien Cazenave [:kaze] 2011-11-02 15:06:45 PDT
Created attachment 571469 [details] [diff] [review]
backout - bug674212
Comment 12 Boris Zbarsky [:bz] 2011-11-02 15:30:07 PDT
Comment on attachment 571469 [details] [diff] [review]
backout - bug674212

r=me; please request aurora approval!
Comment 13 Boris Zbarsky [:bz] 2011-11-02 19:41:58 PDT
> That code was added by Bug 339066.

Thanks.  Looks like for display:none stuff we should indeed return false from IsBreakElement.

Patches coming up to make each of the spellcheck events on my testcase run in about 20ms instead of 80.
Comment 14 Boris Zbarsky [:bz] 2011-11-02 19:42:07 PDT
Created attachment 571539 [details] [diff] [review]
part 1.  Speed up IsBreakElement.
Comment 15 Boris Zbarsky [:bz] 2011-11-02 19:42:49 PDT
Created attachment 571540 [details] [diff] [review]
part 2.  Make it simpler to set up a range.
Comment 16 Boris Zbarsky [:bz] 2011-11-02 19:43:11 PDT
Created attachment 571541 [details] [diff] [review]
part 3.  Speed up mozInlineSpellWordUtil::MakeRange.
Comment 17 Boris Zbarsky [:bz] 2011-11-02 19:43:31 PDT
Created attachment 571542 [details] [diff] [review]
part 4.  Switch from nsIDOMNode to nsINode in mozInlineSpellWordUtil.
Comment 18 Boris Zbarsky [:bz] 2011-11-02 19:58:03 PDT
OK, so the other fundamental issue is that the patch in bug 674212 runs spellcheck on the whole editor on every DocumentModifiedWorker() call.  In this case, any time part of the spreadsheet is mutated via script (e.g. the typein area for values as we move across cells) we spellcheck the whole spreadsheet!

I'm not sure how to limit the spellcheck to just the affected part, but fixing bug 599074 in the special case of scheduling a full-document spellcheck should be easy.  Patch coming up for that.
Comment 19 Boris Zbarsky [:bz] 2011-11-02 20:16:44 PDT
Created attachment 571545 [details] [diff] [review]
part 5.  When we have an event posted to spell-check the whole document, suppress other spell-check events.

Without this patch, but with the other ones in this bug, I could get up to about 7 events all being posted at once if I held down the arrow key.  With this patch, there is only one event posted at a time.

Fabien, it would still be good to only spell-check the part of the document that was mutated.  In this case, that's just the typein field at the top left, as you move down the spreadsheet.  Maybe a followup bug for that?
Comment 20 Boris Zbarsky [:bz] 2011-11-02 20:21:18 PDT
Stealing bug.  I pushed all of the above to try server too; I'd really appreciate it if people could try those builds once they're ready!
Comment 21 Boris Zbarsky [:bz] 2011-11-02 22:46:02 PDT
Ok, I get some crashes on try because of testcases that scream "Jesse" to me that do the following:

1)  Append a <span contenteditable="true"> to the document.
2)  Remove that span

When this happens we set up a spellcheck for a range which has the <span> as both start and end container.  Then on removal we don't change anything.  But the mozInlineSpellWordUtil has the <body> as mRootNode.  So when we go to actually spell-check, mozInlineSpellWordUtil::SetEnd calls FindNextTextNode but mRoot is not actually an ancestor of aNode and then GetNextNode asserts and crashes.

I could restore the old code that could "handle" this situation, but I think it makes more sense to just not spell-check if the range is not under mRootNode.  Going to do that.
Comment 22 Boris Zbarsky [:bz] 2011-11-02 23:18:31 PDT
Created attachment 571563 [details] [diff] [review]
Part 4 with less crashing, more nsIRange.
Comment 23 Boris Zbarsky [:bz] 2011-11-02 23:38:27 PDT
Hrm.  There is one more failure, on test_bug366682.html, where we somehow miss the second word... looking into why.
Comment 24 Boris Zbarsky [:bz] 2011-11-02 23:57:18 PDT
Ah, that's my bug; I implemented FindNextTextNode wrong.
Comment 25 Mozilla RelEng Bot 2011-11-03 00:00:48 PDT
Try run for 281419465d12 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=281419465d12
Results (out of 192 total builds):
    success: 161
    warnings: 30
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bzbarsky@mozilla.com-281419465d12
Comment 26 Boris Zbarsky [:bz] 2011-11-03 00:01:42 PDT
Created attachment 571571 [details] [diff] [review]
Part 4 with that fixed
Comment 27 Mozilla RelEng Bot 2011-11-03 01:20:24 PDT
Try run for d1ed0fcfb261 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d1ed0fcfb261
Results (out of 18 total builds):
    exception: 18
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bzbarsky@mozilla.com-d1ed0fcfb261
Comment 28 Mozilla RelEng Bot 2011-11-03 04:00:32 PDT
Try run for 5a1c9caa43d8 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5a1c9caa43d8
Results (out of 192 total builds):
    exception: 4
    success: 182
    warnings: 5
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bzbarsky@mozilla.com-5a1c9caa43d8
Comment 29 Fabien Cazenave [:kaze] 2011-11-03 04:40:39 PDT
Comment on attachment 571469 [details] [diff] [review]
backout - bug674212

This backout solves a perf regression.
Comment 30 Olli Pettay [:smaug] 2011-11-03 04:42:06 PDT
Comment on attachment 571540 [details] [diff] [review]
part 2.  Make it simpler to set up a range.


>+  virtual nsresult Set(nsINode* aStartParent, PRInt32 aStartOffset,
>+                       nsINode* aEndParent, PRInt32 aEndOffset);
So, virtual because we want this method to be usable outside libxul or what?
This is not terrible hot code, so that is ok.
Comment 31 Boris Zbarsky [:bz] 2011-11-03 04:56:20 PDT
Hmm.  I made it virtual because the other methods there were.  But maybe it doesn't need to be.  We can see later, I guess.
Comment 32 :Ehsan Akhgari 2011-11-03 17:52:18 PDT
CCing some of the drivers to make sure that we don't miss the backout here for Aurora.
Comment 33 :Ehsan Akhgari 2011-11-03 18:03:06 PDT
Comment on attachment 571539 [details] [diff] [review]
part 1.  Speed up IsBreakElement.

Please drop the aDocView argument.  r=me with that.
Comment 34 :Ehsan Akhgari 2011-11-03 18:05:06 PDT
Comment on attachment 571540 [details] [diff] [review]
part 2.  Make it simpler to set up a range.

r=me
Comment 35 :Ehsan Akhgari 2011-11-03 18:06:57 PDT
Comment on attachment 571541 [details] [diff] [review]
part 3.  Speed up mozInlineSpellWordUtil::MakeRange.

r=me
Comment 36 :Ehsan Akhgari 2011-11-03 18:10:11 PDT
Comment on attachment 571545 [details] [diff] [review]
part 5.  When we have an event posted to spell-check the whole document, suppress other spell-check events.

Looks great!
Comment 37 :Ehsan Akhgari 2011-11-03 18:12:43 PDT
(In reply to Olli Pettay [:smaug] from comment #30)
> Comment on attachment 571540 [details] [diff] [review] [diff] [details] [review]
> part 2.  Make it simpler to set up a range.
> 
> 
> >+  virtual nsresult Set(nsINode* aStartParent, PRInt32 aStartOffset,
> >+                       nsINode* aEndParent, PRInt32 aEndOffset);
> So, virtual because we want this method to be usable outside libxul or what?
> This is not terrible hot code, so that is ok.

spellchecker is outside of libxul.  But I'd make it inline instead of virtual.  ;-)
Comment 38 :Ehsan Akhgari 2011-11-03 18:30:29 PDT
Comment on attachment 571571 [details] [diff] [review]
Part 4 with that fixed

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

::: content/base/public/nsINode.h
@@ +49,5 @@
>  #include "nsDOMError.h"
>  #include "nsDOMString.h"
>  #include "jspubtd.h"
>  #include "nsDOMMemoryReporter.h"
> +#include "nsIVariant.h"

Hrm, why is this necessary?

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp
@@ +1343,5 @@
>      wordsSinceTimeCheck ++;
>  
> +    // get the range for the current word.
> +    // Not using nsINode here for now because we have to call into
> +    // selection APIs that use nsIDOMNode.

I would append a ":(" here, but whatever!

::: extensions/spellcheck/src/mozInlineSpellChecker.h
@@ +103,4 @@
>  
>    // If we happen to know something was inserted, this is that range.
>    // Can be NULL (this only allows an optimization, so not setting doesn't hurt)
>    nsCOMPtr<nsIDOMRange> mCreatedRange;

Planning to change this too?  :-)

::: extensions/spellcheck/src/mozInlineSpellWordUtil.cpp
@@ +193,4 @@
>    }
>    
>    while (checkNode && !IsTextNode(checkNode)) {
>      checkNode = FindNextNode(checkNode, aRoot);

This callsite doesn't use the callback function.  Would it make sense for us to just call GetNextNode directly here?

@@ +411,3 @@
>  {
> +  return aNode->IsNodeOfType(nsINode::eCONTENT) &&
> +    static_cast<nsIContent*>(aNode)->IsHTML(nsGkAtoms::br);

Nit:

  return aNode->IsElement() &&
         aNode->AsElement()->IsHTML(nsGkAtoms::br);

@@ -433,5 @@
> -
> -// Find the previous node in the DOM tree in preorder. This isn't fast because
> -// one call to GetPrevSibling can be O(N) in the number of siblings...
> -static nsIDOMNode*
> -FindPrevNode(nsIDOMNode* aNode, nsIDOMNode* aRoot)

/me loves patches which remove stuff like this!
Comment 39 Boris Zbarsky [:bz] 2011-11-03 19:19:49 PDT
> But I'd make it inline instead of virtual.

OK.  Will do.

> Hrm, why is this necessary?

Because nsINode::GetUserData (the overload with an out param) is inline and calls NS_IF_ADDREF on an nsIVariant*.  So including nsINode in a file that does not include nsIVariant or otherwise indicate that nsIVariant inherits from nsISupports fails.  I suppose I could have put that include in the including file, but this seems more robust.

> I would append a ":(" here, but whatever!

Will do.

> Planning to change this too?  :-)

Followup, if so.  This patch was already getting too big.  There are interactions with other editor bits there, iirc...

> Would it make sense for us to just call GetNextNode directly here?

Yes.  Will do.

> Nit:

Ah, cute.  I should have thought of that!  Will do.
Comment 42 :Ms2ger 2011-11-04 07:30:29 PDT
Comment on attachment 571571 [details] [diff] [review]
Part 4 with that fixed

>--- a/extensions/spellcheck/src/mozInlineSpellWordUtil.cpp
>+++ b/extensions/spellcheck/src/mozInlineSpellWordUtil.cpp
>+static inline bool
>+IsTextNode(nsINode* aNode)
> {
>-  PRUint16 type = 0;
>-  aNode->GetNodeType(&type);
>-  return type == nsIDOMNode::TEXT_NODE;
>+  return aNode->IsNodeOfType(nsINode::eTEXT);
> }

Not aNode->NodeType() == nsIDOMNode::TEXT_NODE?
Comment 43 Boris Zbarsky [:bz] 2011-11-04 07:44:17 PDT
> Not aNode->NodeType() == nsIDOMNode::TEXT_NODE?

I forgot we had this now.  That would be better, yeah...  We should consider getting rid of nsINode::eTEXT globally in favor of that.  Want to do it?
Comment 44 christian 2011-11-07 13:33:17 PST
Comment on attachment 571469 [details] [diff] [review]
backout - bug674212

[triage comment]

Approved for aurora. Please land today if at all possible.
Comment 45 Ed Morley [:emorley] 2011-11-07 13:41:47 PST
Thanks bz, this appears to have solved the several-times-a-day hangs of up to 15 seconds a time, I was experiencing with Google Docs :-)
Comment 47 Boris Zbarsky [:bz] 2011-11-12 00:19:59 PST
Ehsan, did you really mean to reopen this and remove the "status-firefox9: fixed" flag?
Comment 48 :Ehsan Akhgari 2011-11-14 10:43:31 PST
(In reply to Boris Zbarsky (:bz) from comment #47)
> Ehsan, did you really mean to reopen this and remove the "status-firefox9:
> fixed" flag?

Nope.  I just meant to make it tracking+ as it seems like that the triage team was unwilling to do so, and Bugzilla managed to help me embarrass myself while doing so.  :-)
Comment 49 Ioana (away) 2011-11-22 05:58:55 PST
Verfied as fixed on:
Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20100101 Firefox/9.0 (20111116091359)
Mozilla/5.0 (X11; Linux x86_64; rv:10.0a2) Gecko/20111121 Firefox/10.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:11.0a1) Gecko/20111122 Firefox/11.0a1

The test case from comment 0 was used. When the user presses the down key, the focus moves on the cell below and the highlight works fine. No more moving down the column takes place.

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