Closed Bug 684638 Opened 13 years ago Closed 13 years ago

Google Docs Spreadsheets Sluggish

Categories

(Core :: DOM: Editor, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla10
Tracking Status
firefox9 + fixed

People

(Reporter: bws42, Assigned: bzbarsky)

References

Details

(4 keywords, Whiteboard: [qa!])

Attachments

(6 files, 4 obsolete files)

1.78 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.79 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
3.15 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.26 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
5.01 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
34.54 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
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.
Keywords: perf, regression
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"
Blocks: 674212
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → kaze
Component: General → Editor
QA Contact: general → editor
Attached patch backout - bug674212 (obsolete) — Splinter Review
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.
Attachment #558847 - Flags: review?(ehsan)
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.
Attachment #558847 - Flags: review?(ehsan) → review-
If the performance problem here is caused by a large number of spellchecking events, fixing bug 599074 might be the answer.
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?
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.
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.
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?
(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.
Attachment #558847 - Attachment is obsolete: true
Attachment #571469 - Flags: review?(ehsan)
Attachment #571469 - Flags: review?(bzbarsky)
Comment on attachment 571469 [details] [diff] [review]
backout - bug674212

r=me; please request aurora approval!
Attachment #571469 - Flags: review?(bzbarsky) → review+
> 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.
Attachment #571539 - Flags: review?(ehsan)
Attachment #571540 - Flags: review? → review?(bugs)
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.
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?
Attachment #571545 - Flags: review?(ehsan)
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!
Assignee: kaze → bzbarsky
Whiteboard: [need review]
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.
Attachment #571563 - Flags: review?(ehsan)
Attachment #571542 - Attachment is obsolete: true
Attachment #571542 - Flags: review?(ehsan)
Hrm.  There is one more failure, on test_bug366682.html, where we somehow miss the second word... looking into why.
Ah, that's my bug; I implemented FindNextTextNode wrong.
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
Attachment #571571 - Flags: review?(ehsan)
Attachment #571563 - Attachment is obsolete: true
Attachment #571563 - Flags: review?(ehsan)
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
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
OS: Linux → All
Hardware: x86_64 → All
Comment on attachment 571469 [details] [diff] [review]
backout - bug674212

This backout solves a perf regression.
Attachment #571469 - Flags: review?(ehsan) → approval-mozilla-aurora?
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.
Attachment #571540 - Flags: review?(bugs) → review+
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.
OS: All → Linux
Hardware: All → x86_64
Blocks: 696618
Blocks: 635618
CCing some of the drivers to make sure that we don't miss the backout here for Aurora.
Attachment #571243 - Attachment is obsolete: true
Comment on attachment 571539 [details] [diff] [review]
part 1.  Speed up IsBreakElement.

Please drop the aDocView argument.  r=me with that.
Attachment #571539 - Flags: review?(ehsan) → review+
Comment on attachment 571540 [details] [diff] [review]
part 2.  Make it simpler to set up a range.

r=me
Comment on attachment 571541 [details] [diff] [review]
part 3.  Speed up mozInlineSpellWordUtil::MakeRange.

r=me
Attachment #571541 - Flags: review?(ehsan) → review+
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!
Attachment #571545 - Flags: review?(ehsan) → review+
(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 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!
Attachment #571571 - Flags: review?(ehsan) → review+
> 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 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?
> 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 on attachment 571469 [details] [diff] [review]
backout - bug674212

[triage comment]

Approved for aurora. Please land today if at all possible.
Attachment #571469 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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 :-)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Version: Trunk → Other Branch
Ehsan, did you really mean to reopen this and remove the "status-firefox9: fixed" flag?
(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.  :-)
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Version: Other Branch → Trunk
Whiteboard: [qa+]
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.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
Depends on: 891904
See Also: → 599074
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: