Closed
Bug 684638
Opened 13 years ago
Closed 13 years ago
Google Docs Spreadsheets Sluggish
Categories
(Core :: DOM: Editor, defect)
Tracking
()
VERIFIED
FIXED
mozilla10
People
(Reporter: bws42, Assigned: bzbarsky)
References
Details
(4 keywords, Whiteboard: [qa!])
Attachments
(6 files, 4 obsolete files)
1.78 KB,
patch
|
bzbarsky
:
review+
christian
:
approval-mozilla-aurora+
|
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.
Reporter | ||
Updated•13 years ago
|
Keywords: perf,
regression
Comment 1•13 years ago
|
||
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"
Updated•13 years ago
|
Assignee: nobody → kaze
tracking-firefox9:
--- → ?
Component: General → Editor
QA Contact: general → editor
Comment 2•13 years ago
|
||
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 4•13 years ago
|
||
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-
Comment 5•13 years ago
|
||
If the performance problem here is caused by a large number of spellchecking events, fixing bug 599074 might be the answer.
Assignee | ||
Comment 6•13 years ago
|
||
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?
Assignee | ||
Comment 7•13 years ago
|
||
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.
Assignee | ||
Comment 8•13 years ago
|
||
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.
Assignee | ||
Comment 9•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
Attachment #558847 -
Attachment is obsolete: true
Attachment #571469 -
Flags: review?(ehsan)
Attachment #571469 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 12•13 years ago
|
||
Comment on attachment 571469 [details] [diff] [review] backout - bug674212 r=me; please request aurora approval!
Attachment #571469 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 13•13 years ago
|
||
> 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.
Assignee | ||
Comment 14•13 years ago
|
||
Attachment #571539 -
Flags: review?(ehsan)
Assignee | ||
Comment 15•13 years ago
|
||
Attachment #571540 -
Flags: review?
Assignee | ||
Comment 16•13 years ago
|
||
Attachment #571541 -
Flags: review?(ehsan)
Assignee | ||
Comment 17•13 years ago
|
||
Attachment #571542 -
Flags: review?(ehsan)
Assignee | ||
Updated•13 years ago
|
Attachment #571540 -
Flags: review? → review?(bugs)
Assignee | ||
Comment 18•13 years ago
|
||
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.
Assignee | ||
Comment 19•13 years ago
|
||
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)
Assignee | ||
Comment 20•13 years ago
|
||
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]
Assignee | ||
Comment 21•13 years ago
|
||
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.
Assignee | ||
Comment 22•13 years ago
|
||
Attachment #571563 -
Flags: review?(ehsan)
Assignee | ||
Updated•13 years ago
|
Attachment #571542 -
Attachment is obsolete: true
Attachment #571542 -
Flags: review?(ehsan)
Assignee | ||
Comment 23•13 years ago
|
||
Hrm. There is one more failure, on test_bug366682.html, where we somehow miss the second word... looking into why.
Assignee | ||
Comment 24•13 years ago
|
||
Ah, that's my bug; I implemented FindNextTextNode wrong.
Comment 25•13 years ago
|
||
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
Assignee | ||
Comment 26•13 years ago
|
||
Attachment #571571 -
Flags: review?(ehsan)
Assignee | ||
Updated•13 years ago
|
Attachment #571563 -
Attachment is obsolete: true
Attachment #571563 -
Flags: review?(ehsan)
Comment 27•13 years ago
|
||
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•13 years ago
|
||
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
Updated•13 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Comment 29•13 years ago
|
||
Comment on attachment 571469 [details] [diff] [review] backout - bug674212 This backout solves a perf regression.
Attachment #571469 -
Flags: review?(ehsan) → approval-mozilla-aurora?
Comment 30•13 years ago
|
||
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+
Assignee | ||
Comment 31•13 years ago
|
||
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
Comment 32•13 years ago
|
||
CCing some of the drivers to make sure that we don't miss the backout here for Aurora.
Updated•13 years ago
|
Attachment #571243 -
Attachment is obsolete: true
Comment 33•13 years ago
|
||
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 34•13 years ago
|
||
Comment on attachment 571540 [details] [diff] [review] part 2. Make it simpler to set up a range. r=me
Comment 35•13 years ago
|
||
Comment on attachment 571541 [details] [diff] [review] part 3. Speed up mozInlineSpellWordUtil::MakeRange. r=me
Attachment #571541 -
Flags: review?(ehsan) → review+
Comment 36•13 years ago
|
||
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+
Comment 37•13 years ago
|
||
(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•13 years ago
|
||
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+
Assignee | ||
Comment 39•13 years ago
|
||
> 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.
Assignee | ||
Comment 40•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a23e1ebfbb1 https://hg.mozilla.org/integration/mozilla-inbound/rev/91d660a94cc2 https://hg.mozilla.org/integration/mozilla-inbound/rev/f0c2904b8edf https://hg.mozilla.org/integration/mozilla-inbound/rev/132b27a4fb02 https://hg.mozilla.org/integration/mozilla-inbound/rev/adcfab34c8ae
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla10
Comment 41•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5a23e1ebfbb1 https://hg.mozilla.org/mozilla-central/rev/91d660a94cc2 https://hg.mozilla.org/mozilla-central/rev/f0c2904b8edf https://hg.mozilla.org/mozilla-central/rev/132b27a4fb02 https://hg.mozilla.org/mozilla-central/rev/adcfab34c8ae
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 42•13 years ago
|
||
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?
Assignee | ||
Comment 43•13 years ago
|
||
> 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•13 years ago
|
||
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+
Comment 45•13 years ago
|
||
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 :-)
Assignee | ||
Comment 46•13 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/e4565d9a60a2
status-firefox9:
--- → fixed
Updated•13 years ago
|
Status: RESOLVED → REOPENED
status-firefox9:
fixed → ---
Resolution: FIXED → ---
Version: Trunk → Other Branch
Assignee | ||
Comment 47•13 years ago
|
||
Ehsan, did you really mean to reopen this and remove the "status-firefox9: fixed" flag?
Comment 48•13 years ago
|
||
(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 ago → 13 years ago
status-firefox9:
--- → fixed
Resolution: --- → FIXED
Version: Other Branch → Trunk
Comment 49•13 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•