1354641 - Adjust the heuristic in mozInlineSpellChecker to break the work to smaller chunks

Status: RESOLVED FIXED

(Core :: Spelling checker, enhancement, P1)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

+++ This bug was initially created as a clone of Bug #1303749 +++

The heuristics in mozInlineSpellChecker.cpp does not make sense anymore in e10s world where each CheckWord involves a sync IPC. It also doesn't make sense to allow a single spellcheck run to take 50ms on main thread.

The original goal seems to check as many words as possible in a single run but for responsiveness we should optimize for spending less time in a single run.

http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/extensions/spellcheck/src/mozInlineSpellChecker.cpp#80-95

Comment 1

[Till Schneidereit [:till]]

(In reply to Kan-Ru Chen [:kanru] (UTC-4, EDT) from comment #0)
> The original goal seems to check as many words as possible in a single run
> but for responsiveness we should optimize for spending less time in a single
> run.

I'm not sure I understand. The profiles in bug 1343864 and bug 1303749 seem to indicate the exact opposite: there are too many individual calls. This seems to include the "after" profile from bug 1303749 comment 31. If there are any ipc calls that took longer than a millisecond in that one, I wasn't able to find them.

Comment 2

[Markus Stange [:mstange]]

There are too many individual IPC calls, but the code that makes these calls spends too much time on it continuously before it returns to the event loop. The goal in this bug is to do shorter amounts of work per individual event loop runnable.

Comment 3

[Kan-Ru Chen [:kanru] (UTC+9)]

Yes, thanks Markus's clarification.

Comment 4

[Till Schneidereit [:till]]

Ah, got it - that makes sense.

Comment 5

[Evelyn Hung]

I commented out `INLINESPELL_TIMEOUT_CHECK_FREQUENCY` check and set `INLINESPELL_CHECK_TIMEOUT` to a smaller number, 10ms and 1ms respectively, so after every word spell checking, we evaluate the passed time to see if we want to check more words at this round. The profile results in [1][2] show the main thread is busy on repainting and eats the spare cycles. In profile [1], DidComposite and Notify messages come in and then Paint and Vsync tasks occupied around 50% times in total. So maybe we need to buffer misspelled results to not call selectFrames (which schedules frame update tasks) frequently. 


[1]with 1ms - https://perfht.ml/2nVzvMh
[2]with 10ms - https://perfht.ml/2orMbhV
[3]with 50ms (original value but INLINESPELL_TIMEOUT_CHECK_FREQUENCY is removed) - https://perfht.ml/2orOjWD

Comment 6

[Evelyn Hung]

Update our offline discussion (with Ehsan) here.

We revisited the profile in comment 5, and found out a few places to make small improvements, like bug 1355595. These small gains and this heuristic polish may actually being "perceived".

On the other hand, the test in comment 5 was just logging performance in "loading time", however, we should test interactions like "typing" as it's also the case that triggers spell checking.

Questions we want to answer now:

1. How much extra time will the spellchecker take if we change the INLINESPELL_CHECK_TIMEOUT from 50ms to 10ms, or to 1ms? We need to evaluate if it's worthy to extend the checking cycles for gaining (how much) responsiveness. And we should avoid falling into a trap that we can't finish the checking so the feature is broken although we gain responsiveness.

2. How does the heuristic polish actually help on "typing" scenario? Will it looks more responsive because the typing characters shows up immediately? We need to test on some real cases.

Comment 7

[Evelyn Hung]

(In reply to Evelyn Hung [:evelyn] from comment #6) 
> 1. How much extra time will the spellchecker take if we change the
> INLINESPELL_CHECK_TIMEOUT from 50ms to 10ms, or to 1ms? We need to evaluate
> if it's worthy to extend the checking cycles for gaining (how much)
> responsiveness. And we should avoid falling into a trap that we can't finish
> the checking so the feature is broken although we gain responsiveness.
> 

Here is the profile data of loading https://public.etherpad-mozilla.org/p/wr-plan. When the INLINESPELL_CHECK_TIMEOUT decreasing, the total time to finish the whole content's spell check doesn't increase a lot (3 times from 50ms to 1ms). It might be also worth mentioning that the accumulated time spending in |ResumeCheck()| is doubled (which means, not surprisingly, overall the spell check occupies main thread longer to finish all checks).

50ms: 1.2 sec; ResumeCheck - 920ms (https://perfht.ml/2q0aR28)
25ms: 1.2 sec; ResumeCheck - 798ms (https://perfht.ml/2pd5wBP)
10ms: 2.2 sec; ResumeCheck - 1099ms (https://perfht.ml/2pcFJtl)
 1ms: 3.5 sec; ResumeCheck - 1747ms (https://perfht.ml/2pcOwve)

From users' point of view, after bug 1330912 we can see the misspelled words being marked underline almost immediately after the content loaded. The heuristic polish doesn't make it better.

> 2. How does the heuristic polish actually help on "typing" scenario? Will it
> looks more responsive because the typing characters shows up immediately? We
> need to test on some real cases.

However, short the timeout to 1ms did help to make the UI more responsive when the user is typing, especially typing when the content is still loading and doing spell check. I can see my typing words show up immediately without delays.

To sum up (1) and (2), IMO it's not a bad trade-off to increase the overall spell check time a bit to gain responsiveness.

Comment 8

[Evelyn Hung]

Attachment: Bug 1354641 - adjust the value of spell check heuristics.

This patch is mainly for adjusting the value of INLINESPELL_CHECK_TIMEOUT
from 50ms to 1ms. The value means how long the main thread is blocked
for spelling check, and 50ms is too long. It causes significant delays
when a rich content document is loading, and the user tries to
type immediately before spell checking is done.

After modifying INLINESPELL_CHECK_TIMEOUT to 1ms, it seems we don't need
INLINESPELL_TIMEOUT_CHECK_FREQUENCY anymore because we intend to check
often.

Review commit: https://reviewboard.mozilla.org/r/133828/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133828/

Comment 9

[Evelyn Hung]

Hi :masayuki, I saw your reviewed bug 1330912 so I was wondering if you can take a look of this patch too. Thanks!

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 8861824 [details]
Bug 1354641 - adjust the value of spell check heuristics.

https://reviewboard.mozilla.org/r/133828/#review136752

::: commit-message-73752:3
(Diff revision 1)
> +This patch is mainly for adjusting the value of INLINESPELL_CHECK_TIMEOUT
> +from 50ms to 1ms. The value means how long the main thread is blocked
> +for spelling check, and 50ms is too long. It causes significant delays
> +when a rich content document is loading, and the user tries to
> +type immediately before spell checking is done.
> +
> +After modifying INLINESPELL_CHECK_TIMEOUT to 1ms, it seems we don't need
> +INLINESPELL_TIMEOUT_CHECK_FREQUENCY anymore because we intend to check
> +often.

If it doesn't cause making users feel the total performance slower, looks this fine to me.

However, there are some concerns. I'd like to know your idea for them before giving r+.

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp:1302
(Diff revision 1)
>  mozInlineSpellChecker::ScheduleSpellCheck(const mozInlineSpellStatus& aStatus)
>  {
>    if (mFullSpellCheckScheduled) {
>      // Just ignore this; we're going to spell-check everything anyway
>      return NS_OK;
>    }
>  
>    RefPtr<mozInlineSpellResume> resume =
>      new mozInlineSpellResume(aStatus, mDisabledAsyncToken);
>    NS_ENSURE_TRUE(resume, NS_ERROR_OUT_OF_MEMORY);

After applying your patch, the number of creating mozInlineSpellResume must be increased a lot. However, AFAIK, memory allocation is slow. Shouldn't mozInlineSpellChecker store the instance and reuse it if it's possible?

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp:1480
(Diff revision 1)
>    while (NS_SUCCEEDED(aWordUtil.GetNextWord(wordText,
>                                              getter_AddRefs(wordRange),
>                                              &dontCheckWord)) &&
>           wordRange) {
> -    wordsSinceTimeCheck++;
>  
>      // get the range for the current word.
>      nsINode *beginNode;
>      nsINode *endNode;
>      int32_t beginOffset, endOffset;
>  
>      ErrorResult erv;
>      beginNode = wordRange->GetStartContainer(erv);
>      endNode = wordRange->GetEndContainer(erv);
>      beginOffset = wordRange->GetStartOffset(erv);
>      endOffset = wordRange->GetEndOffset(erv);
>  
>  #ifdef DEBUG_INLINESPELL
>      printf("->Got word \"%s\"", NS_ConvertUTF16toUTF8(wordText).get());
>      if (dontCheckWord)
>        printf(" (not checking)");
>      printf("\n");
>  #endif
>  
>      // see if there is a spellcheck range that already intersects the word
>      // and remove it. We only need to remove old ranges, so don't bother if
>      // there were no ranges when we started out.
>      if (originalRangeCount > 0) {
>        // likewise, if this word is inside new text, we won't bother testing
>        if (!aStatus->mCreatedRange ||
>            !aStatus->mCreatedRange->IsPointInRange(*beginNode, beginOffset, erv)) {
>          nsTArray<RefPtr<nsRange>> ranges;
>          aSpellCheckSelection->GetRangesForInterval(*beginNode, beginOffset,
>                                                     *endNode, endOffset,
>                                                     true, ranges, erv);
>          ENSURE_SUCCESS(erv, erv.StealNSResult());
>          for (uint32_t i = 0; i < ranges.Length(); i++)
>            RemoveRange(aSpellCheckSelection, ranges[i]);
>        }
>      }
>  
>      // some words are special and don't need checking
>      if (dontCheckWord)
>        continue;
>  
>      // some nodes we don't spellcheck
>      if (!ShouldSpellCheckNode(editor, beginNode))
>        continue;
>  
>      // Don't check spelling if we're inside the noCheckRange. This needs to
>      // be done after we clear any old selection because the excluded word
>      // might have been previously marked.
>      //
>      // We do a simple check to see if the beginning of our word is in the
>      // exclusion range. Because the exclusion range is a multiple of a word,
>      // this is sufficient.
>      if (aStatus->mNoCheckRange &&
>          aStatus->mNoCheckRange->IsPointInRange(*beginNode, beginOffset, erv)) {
>        continue;
>      }
>  
>      // check spelling and add to selection if misspelled
>      bool isMisspelled;
>      aWordUtil.NormalizeWord(wordText);
>      nsresult rv = mSpellCheck->CheckCurrentWordNoSuggest(wordText.get(), &isMisspelled);
>      if (NS_FAILED(rv))
>        continue;
>  
>      if (isMisspelled) {
>        // misspelled words count extra toward the max
> -      wordsSinceTimeCheck += MISSPELLED_WORD_COUNT_PENALTY;
>        AddRange(aSpellCheckSelection, wordRange);
>  
>        aStatus->mWordCount ++;
>        if (aStatus->mWordCount >= mMaxMisspellingsPerCheck ||
>            SpellCheckSelectionIsFull())
>          break;
>      }
>  
> -    // see if we've run out of time, only check every N words for perf
> +    // see if we've run out of time
> -    if (wordsSinceTimeCheck >= INLINESPELL_TIMEOUT_CHECK_FREQUENCY) {
> -      wordsSinceTimeCheck = 0;
> -      if (PR_Now() > PRTime(beginTime + INLINESPELL_CHECK_TIMEOUT * PR_USEC_PER_MSEC)) {
> +    if (PR_Now() > PRTime(beginTime + INLINESPELL_CHECK_TIMEOUT * PR_USEC_PER_MSEC)) {

There are a lot of "continue" before here, therefore, DoSpellCheck() still can take long time. How often we hit above "continue"s?  If it's too many, I think that timeout check needs earlier.

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp:1558
(Diff revision 1)
> -      if (PR_Now() > PRTime(beginTime + INLINESPELL_CHECK_TIMEOUT * PR_USEC_PER_MSEC)) {
> +    if (PR_Now() > PRTime(beginTime + INLINESPELL_CHECK_TIMEOUT * PR_USEC_PER_MSEC)) {
> -        // stop checking, our time limit has been exceeded
> +      // stop checking, our time limit has been exceeded
>  
> -        // move the range to encompass the stuff that needs checking
> +      // move the range to encompass the stuff that needs checking
> -        rv = aStatus->mRange->SetStart(endNode, endOffset);
> +      rv = aStatus->mRange->SetStart(endNode, endOffset);
> -        if (NS_FAILED(rv)) {
> +      if (NS_FAILED(rv)) {
> -          // The range might be unhappy because the beginning is after the
> +        // The range might be unhappy because the beginning is after the
> -          // end. This is possible when the requested end was in the middle
> +        // end. This is possible when the requested end was in the middle
> -          // of a word, just ignore this situation and assume we're done.
> +        // of a word, just ignore this situation and assume we're done.
> -          return NS_OK;
> +        return NS_OK;
> -        }
> +      }
> -        *aDoneChecking = false;
> +      *aDoneChecking = false;
> -        return NS_OK;
> +      return NS_OK;

So, if the environment is slow, DoSpellCheck() may check only one word at a call. If there are too many words to be checked, it means that this is called a lot. If so, should this guarantee that some words are checked at least?

Comment 11

[Evelyn Hung]

Thanks for the review, Masayuki. I will address your comments in my next patch, but while I'm working on the patch, I found another issue. It seems the 1ms change leads a result that spell check will early break because the number of misspell words hits the upper bound.
http://searchfox.org/mozilla-central/source/extensions/spellcheck/src/mozInlineSpellChecker.cpp#1567-1570

It's weird because I was using the same test case and the detected misspell words should be the same. I now suspect this is because every time when we reschedule a check there is a overlapped words with the last round. So the more reschedule, the more overlap words. Therefore the misspell words are double counted. 

I will look into how we set the checking area for the next round, and if you have any hit, please let me know. Thanks!

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Hmm, I don't have idea which exactly causes such behavior, I feel odd to check/add misspelled word/range before checking SpellCheckSelectionIsFull().

Comment 13

[Evelyn Hung]

I think I have figured out the overlapping words. I firstly observed that there were always two words overlapped, and then realized they were caused by different reasons. Both derive from the logic of how we build the to-be-checked text in |mozInlineSpellWordUtil::BuildSoftText()|:

1. When we schedule the next spell check round, we set the next start position to the endOffset of the current checked word. (http://searchfox.org/mozilla-central/source/extensions/spellcheck/src/mozInlineSpellChecker.cpp#1580) So in the next round of |BuildSoftText()|, we will find the first separator *before* this offset, which means the last checked word will be included in this round.

I was thinking to pre-fetch the next word when we do scheduling, and if there is no next word then we don't bother to schedule the next round. This can be achieved by moving the timeout check and the scheduling code earlier.

2. According to the logic of |BuildSoftText()|, we will always look for one more separator before the separator we have found. Bug 596333 comment 5 described the problem we wanted to resolve. However, in our case, it introduces another overlapping word:

    the first round
|-------------------| 
This is a spell check example             
         X     X    ^<-- endOffset
         2nd   1st found separators
          |-----------------|
            the second round

I don't have an idea to properly fix this issue, and feel this is out of this bug's scope. I will file a follow-up bug.

Comment 14

[Evelyn Hung]

Comment on attachment 8861824 [details]
Bug 1354641 - adjust the value of spell check heuristics.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133828/diff/1-2/

Comment 15

[Evelyn Hung]

Comment on attachment 8861824 [details]
Bug 1354641 - adjust the value of spell check heuristics.

https://reviewboard.mozilla.org/r/133828/#review136752

> After applying your patch, the number of creating mozInlineSpellResume must be increased a lot. However, AFAIK, memory allocation is slow. Shouldn't mozInlineSpellChecker store the instance and reuse it if it's possible?

I found mStatus in mozInlineSpellResume object occupies the most memory and could be reused. Therefore I made it just one single mozInlineSpellStatus instance everywhere.

> So, if the environment is slow, DoSpellCheck() may check only one word at a call. If there are too many words to be checked, it means that this is called a lot. If so, should this guarantee that some words are checked at least?

INLINESPELL_MINIMUM_WORDS_BEFORE_TIMEOUT is added for the guarantee.

Comment 16

[Evelyn Hung]

Hi Masayuki, the changes in this patch address what you said in comment 10 and what I found in comment 13 (not including point #2). Thanks for your review suggestions!

Comment 17

[Evelyn Hung]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=944fe067d7a15f73deea8a02d4180c975021bfff

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 8861824 [details]
Bug 1354641 - adjust the value of spell check heuristics.

https://reviewboard.mozilla.org/r/133828/#review138282

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp:473
(Diff revision 2)
>  class mozInlineSpellResume : public Runnable
>  {
>  public:
> -  mozInlineSpellResume(const mozInlineSpellStatus& aStatus,
> +  mozInlineSpellResume(UniquePtr<mozInlineSpellStatus>&& aStatus,
>                         uint32_t aDisabledAsyncToken)
> -    : mDisabledAsyncToken(aDisabledAsyncToken), mStatus(aStatus) {}
> +    : mDisabledAsyncToken(aDisabledAsyncToken), mStatus(Move(aStatus)) {}

nit: could you wrap this line for initializing a member per line. So, "," should be below ":". Then, could you move {} to the next line? Then, when somebody adds new member, we'll see only one new line for initializing the member in the diff.

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp:885
(Diff revision 2)
> -  rv = status.InitForEditorChange((EditAction)aAction,
> +  rv = status->InitForEditorChange((EditAction)aAction,
>                                    anchorNode, anchorOffset,
>                                    aPreviousSelectedNode, aPreviousSelectedOffset,
>                                    aStartNode, aStartOffset,
>                                    aEndNode, aEndOffset);

nit: Please add intent between #886 and #889. Start of each method should be below the second "(" of #885.

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp:887
(Diff revision 2)
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> -  mozInlineSpellStatus status(this);
> -  rv = status.InitForEditorChange((EditAction)aAction,
> +  auto status = MakeUnique<mozInlineSpellStatus>(this);
> +  rv = status->InitForEditorChange((EditAction)aAction,
>                                    anchorNode, anchorOffset,
>                                    aPreviousSelectedNode, aPreviousSelectedOffset,

nit: This line is over 80 characters, so, could you wrap this line with this patch?

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp:1315
(Diff revision 2)
>    if (mFullSpellCheckScheduled) {
>      // Just ignore this; we're going to spell-check everything anyway
>      return NS_OK;
>    }
>  
> +  bool isFullSpellCheck = aStatus->IsFullSpellCheck();

Could you explain why ScheducleSpellCheck needs to cache the result here? (as comment in this method.)

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp:1317
(Diff revision 2)
>    RefPtr<mozInlineSpellResume> resume =
> -    new mozInlineSpellResume(aStatus, mDisabledAsyncToken);
> +    new mozInlineSpellResume(Move(aStatus), mDisabledAsyncToken);

So, I'm still worry about the creating cost of mozInlineSpellResume, but I am not sure the actual cost and now, INLINESPELL_MINIMUM_WORDS_BEFORE_TIMEOUT, reduces the number of recreation drastically than the previous patch. So, I guess that my concern is wrong now.

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp:1507
(Diff revision 2)
>      endOffset = wordRange->GetEndOffset(erv);
>  
> +    nsresult rv;
> +    // see if we've done enough words in this round and run out of time.
> +    if (wordsChecked >= INLINESPELL_MINIMUM_WORDS_BEFORE_TIMEOUT &&
> +        PR_Now() > PRTime(beginTime + INLINESPELL_CHECK_TIMEOUT * PR_USEC_PER_MSEC)) {

Too long line. I think that INLINESPELL_CHECK_TIMEOUT * PR_USEC_PER_MSEC can be |static const| variable. Then, you can reduce the length.

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp:1511
(Diff revision 2)
> +    if (wordsChecked >= INLINESPELL_MINIMUM_WORDS_BEFORE_TIMEOUT &&
> +        PR_Now() > PRTime(beginTime + INLINESPELL_CHECK_TIMEOUT * PR_USEC_PER_MSEC)) {
> +      // stop checking, our time limit has been exceeded.
> +      wordsChecked = 0;
> +      // move the range to encompass the stuff that needs checking.
> +      rv = aStatus->mRange->SetStart(endNode, endOffset);

I think that you should declare |nsresult rv| here then, other developers can understand that the result won't be referred by out of the block easier.

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp:1571
(Diff revision 2)
>      }
>  
>      // check spelling and add to selection if misspelled
>      bool isMisspelled;
>      aWordUtil.NormalizeWord(wordText);
> -    nsresult rv = mSpellCheck->CheckCurrentWordNoSuggest(wordText.get(), &isMisspelled);
> +    rv = mSpellCheck->CheckCurrentWordNoSuggest(wordText.get(), &isMisspelled);

So, this change shouldn't be necessary.

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp:1875
(Diff revision 2)
> -  rv = status.InitForNavigation(aForceWordSpellCheck, aNewPositionOffset,
> +  rv = status->InitForNavigation(aForceWordSpellCheck, aNewPositionOffset,
>                                  currentAnchorNode, currentAnchorOffset,
>                                  mCurrentSelectionAnchorNode, mCurrentSelectionOffset,
>                                  &shouldPost);

nit: fix the indent and wrap long line.

Comment 19

[Evelyn Hung]

Comment on attachment 8861824 [details]
Bug 1354641 - adjust the value of spell check heuristics.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133828/diff/2-3/

Comment 20

[Evelyn Hung]

Comment on attachment 8861824 [details]
Bug 1354641 - adjust the value of spell check heuristics.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133828/diff/3-4/

Comment 21

[(no longer active)]

Comment on attachment 8861824 [details]
Bug 1354641 - adjust the value of spell check heuristics.

https://reviewboard.mozilla.org/r/133828/#review138592

Evelyn asked me on IRC to take a look at her changes here.  I skimmed this and it looks really awesome.  I'd like to ask for some more testing below, but I don't think this needs to block landing the work done here, so I suggest filing follow-ups for the below if needed.  Thanks a lot for your great work here Evelyn!

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp:87
(Diff revision 4)
>  
>  // The number of words to check before we look at the time to see if
> -// INLINESPELL_CHECK_TIMEOUT ms have elapsed. This prevents us from spending
> -// too much time checking the clock. Note that misspelled words count for
> -// more than one word in this calculation.
> -#define INLINESPELL_TIMEOUT_CHECK_FREQUENCY 50
> +// INLINESPELL_CHECK_TIMEOUT ms have elapsed. This prevents us from getting
> +// stuck and not moving forward because the INLINESPELL_CHECK_TIMEOUT might
> +// be too short to a low-end machine.
> +#define INLINESPELL_MINIMUM_WORDS_BEFORE_TIMEOUT 5

I'm a bit worried that 5 here may be too much...  But I think the changes here are probably good enough to land at this point.  That being said, I suggest trying to measure how often we actually hit the case where we start checking 5 words in a run of the main loop below while we're over out 1ms budget on a slower machine.

I suggest in order to do this testing use the Acer Quantum reference hardware with maybe a real site like Facebook.  You can try typing in a news feed comment, try writing something on a user's wall and things like that, and see if we ever fall into a case where we run over the budget because INLINESPELL_MINIMUM_WORDS_BEFORE_TIMEOUT is too high on that configuration.

Comment 22

[Evelyn Hung]

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #21)
 > I'm a bit worried that 5 here may be too much...  But I think the changes
> here are probably good enough to land at this point.  That being said, I
> suggest trying to measure how often we actually hit the case where we start
> checking 5 words in a run of the main loop below while we're over out 1ms
> budget on a slower machine.
> 
> I suggest in order to do this testing use the Acer Quantum reference
> hardware with maybe a real site like Facebook.  You can try typing in a news
> feed comment, try writing something on a user's wall and things like that,
> and see if we ever fall into a case where we run over the budget because
> INLINESPELL_MINIMUM_WORDS_BEFORE_TIMEOUT is too high on that configuration.

Yes, I am not very sure of the magic number 5, and plan to test on our reference hardware when I get it next week. Thanks for the suggestion, and will file follow-ups if my test result doesn't go well.

Comment 23

[Evelyn Hung]

Comment on attachment 8861824 [details]
Bug 1354641 - adjust the value of spell check heuristics.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133828/diff/4-5/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20a9d741cdf4
adjust the value of spell check heuristics. r=masayuki

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/20a9d741cdf4

Comment 26

[Carsten Book [:Tomcat]]

seems the backout does not apply cleanly, so might need a backout patch

checking for uncommitted changes
backing out 20a9d741cdf4
applying patch from stdin
patching file extensions/spellcheck/src/mozInlineSpellChecker.cpp
Hunk #3 FAILED at 474
1 out of 24 hunks FAILED -- saving rejects to file extensions/spellcheck/src/mozInlineSpellChecker.cpp.rej
abort: patch failed to apply

Comment 27

[Evelyn Hung]

Attachment: revert_bug1354641.patch

Comment 28

[Evelyn Hung]

(In reply to Evelyn Hung [:evelyn] from comment #27)
> Created attachment 8866400 [details] [diff] [review]
> revert_bug1354641.patch

Hi :Tomcat, could you try this revert patch? Thanks!

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8a43b69fbae255a2af98f11507862437d6303033

Comment 30

[Evelyn Hung]

Attachment: Bug 1354641 - part 2: fix reftest 672709-ref.html intermittent failure.

Review commit: https://reviewboard.mozilla.org/r/142376/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142376/

Comment 31

[Evelyn Hung]

Comment on attachment 8861824 [details]
Bug 1354641 - adjust the value of spell check heuristics.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133828/diff/5-6/

Comment 32

[Evelyn Hung]

Comment on attachment 8861824 [details]
Bug 1354641 - adjust the value of spell check heuristics.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133828/diff/6-7/

Comment 33

[Evelyn Hung]

Comment on attachment 8870811 [details]
Bug 1354641 - part 2: fix reftest 672709-ref.html intermittent failure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142376/diff/1-2/

Comment 34

[Evelyn Hung]

Hi Ehsan,

I would like to have your review on both part 1 and part 2.

For part 1, compare to the last version, I simply move the timeout check back, right before the sync IPC, like what you suggested in bug 1362858.

For part 2, I'm fixing the intermittent error in bug 1361820. The root cause is different from the infinite loop bug, it's just because we take longer time to finish the checking of every word in <body> in several rounds. I intend to move both <script> and <style> to <head> element, but I failed to move <style>. The test will failed (not timeout) in image comparison stage, the caret showed up in the test case image. I don't know the exact reason, but it sounds a bug. I decide to work around by just moving scripts to the <head>.

I want to land both at the same time so I put two patches in this bug. I am fine to move part 2 to bug 1361820 if you think it makes more sense. Thanks!

Comment 35

[Evelyn Hung]

try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0bd8912ec6a66067681221a4693bd9e33370939&selectedJob=101513249

I triggered the failed case a few times and they are all green now.

Comment 36

[(no longer active)]

Comment on attachment 8861824 [details]
Bug 1354641 - adjust the value of spell check heuristics.

Looks good!  I'm r+ing on bugzilla since MozReview was thinking that my review was cleared and I don't have the right to reset my own flag there for some reason.

Comment 37

[(no longer active)]

Comment on attachment 8870811 [details]
Bug 1354641 - part 2: fix reftest 672709-ref.html intermittent failure.

https://reviewboard.mozilla.org/r/142376/#review148448

Looks good, but we should really fix the spellchecker itself here.  Evelyn and I discussed some plans for this on IRC.

Comment 38

[(no longer active)]

Speaking on IRC with Evelyn about why part 2 is necessary, we arrived at a theory by debugging this on searchfox instead of on gdb.  My theory was that we get to this code:

<http://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/extensions/spellcheck/src/mozInlineSpellChecker.cpp#1307> and then since we're in a designMode document and all of the nodes in the document are editable, that code returns true from <http://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/dom/html/nsGenericHTMLElement.cpp#397> and we incorrectly decide to spellcheck the style node.

If that theory is true, then it may be possible to special case node types such as script and style in ShouldSpellCheckNode() as an alternative fix to part 2.

Comment 39

[Evelyn Hung]

Sorry for not updating for a while. I checked Ehsan's theory above and it's correct for designMode case. We could fix it in ShouldSpellCheckNode() by special case script and style node types. 

But if we want to completely ignore script and style nodes, i.e. never _consider_ to check <script> and <style>, we should special case them earlier at the stage of building soft text. Ehsan proposed to modify IsTextNode(nsINode* aNode) by adding checks to its parent node and see if it's a script or style type. I feel it's worthy trying, and I will do it in bug 1362858.

So the patches on this bug should be landed. We are closing to branch Fx55 to beta, I feel it's not a good time to squeeze them in now. Let's wait 56 and I hope bug 1362858 could also be fixed in the same timeframe.

Hi Ryan, is there a flag that I can set to indicate this bug is *Fx56* checked-in needed? ;)

Comment 40

[Ryan VanderMeulen [:RyanVM]]

(In reply to Evelyn Hung [:evelyn] from comment #39)
> Hi Ryan, is there a flag that I can set to indicate this bug is *Fx56*
> checked-in needed? ;)

Best to just wait on requesting checkin until after the uplift next week.

Comment 41

[Evelyn Hung]

Comment on attachment 8861824 [details]
Bug 1354641 - adjust the value of spell check heuristics.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133828/diff/7-8/

Comment 42

[Evelyn Hung]

Hi Ehsan, sorry for asking review again, but I would like to get your attention before landing this patch.

Since bug 1362858 fixed some known issues, I made the following change to the patches here:

1. remove part 2 which modified reftest 672709-ref.html to workaround <script> and <style> tag issue.
2. for the original part 1, I moved the timeout check prior to ShouldSpellCheckNode() and other checks. Refer to the discussion on bug 1362858 comment 9 and 10, "these checks become expensive when there are non-editable elements with a lot of text in there." Therefore, if there is a <div> with tons of words, it might block main thread for more than 1ms to loop all its words. I feel this reorder makes more sense to the goal of this performance optimization.[1]

I also tested Facebook and typed on etherpad with the patches on bug 1362858 and this one, all look good on the reference hardware. Try result is good too, no intermittent failure on the reftest (re-triggered them a couple of times). [2]

[1] I did a rebase to latest m-c, so you see some changes in the diff which aren't made by me but other patches.
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0e5a77fab8bed76e12e1451e3fbdafbdd0728a2&selectedJob=106614015

Comment 43

[(no longer active)]

Comment on attachment 8861824 [details]
Bug 1354641 - adjust the value of spell check heuristics.

https://reviewboard.mozilla.org/r/133828/#review152928

I still think moving the timeout check is risky, but it's probably fine since we will have time to deal with any other regressions that we may discover after landing this, so let's try it again!  Things may be much better now after the fix to bug 1362858.  Thanks!

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/daa3afd9efdc
adjust the value of spell check heuristics. r=Ehsan,masayuki

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/daa3afd9efdc