1396980 - [FIX] Spending too much time making ranges for spellchecker on Facebook page

Status: RESOLVED FIXED

(Core :: Spelling checker, defect, P3)

Description

[Mike Conley (:mconley) (:⚙️)]

Spinning this out from bug 1367004, so check out the STR in there.

Looking at the profile in comment 9, I noticed:

https://perfht.ml/2eISvPi

mozInlineSpellWordUtils::MakeRange seems to be pretty expensive here - ~50ms or so.

Comment 1

[(no longer active)]

I think we could do a lot better here.

In this loop, we create a new nsRange object for every single word: <https://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/extensions/spellcheck/src/mozInlineSpellChecker.cpp#1507>

then we extract the boundaries of the range: <https://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/extensions/spellcheck/src/mozInlineSpellChecker.cpp#1518>

and if the range was misspelled, we mark it as a misspelled range: <https://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/extensions/spellcheck/src/mozInlineSpellChecker.cpp#1598>

But GetNextWord() could return a much lighter weight object, like a std::pair<NodeOffset> where NodeOffset is <https://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/extensions/spellcheck/src/mozInlineSpellWordUtil.h#47>.  That way we could avoid the heap allocation to create the range object, the refcount manipulations, range boundary checks, setting up mutation observers, etc. etc.  It should be relatively simple to do, I think...

Kan-Ru, is there anyone on your team who would be able to take this on?

Comment 2

[Evelyn Hung]

Attachment: Bug 1396980 - Lazily creating nsRange object when misspell word is found.

We used to create a nsRange for each word when doing spell check. It's time
consuming and waste heap memory. This patch introducing a NodeOffsetRange type
to represent a range with begin and end NodeOffset on call stack. We'll then
create the nsRange object only for a misspell word.

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

Comment 3

[Evelyn Hung]

Comment on attachment 8919995 [details]
Bug 1396980 - Lazily creating nsRange object when misspell word is found.

I did a tiny refactor to pull out NodeOffset from mozInlineSpellWordUtil class, so it can be used in mozInlineSpellChecker. Also introduce a NodeOffsetRange to replace the original use case of nsRange in DoSpellCheck(). Not sure if this is a correct direction of change.

Comment 4

[Evelyn Hung]

Oh, forgot to attach the profile result. I use this etherpad and just measure it at loading time.
Before - https://perfht.ml/2kXdCQc  4ms on MakeReangeforWord
After - https://perfht.ml/2xQajAG 4ms is gone.

Comment 5

[Evelyn Hung]

(In reply to Evelyn Hung [:evelyn] from comment #4)
> Oh, forgot to attach the profile result. I use this etherpad 
https://public.etherpad-mozilla.org/p/spell-check-typing

Comment 6

[Evelyn Hung]

Attachment: Bug 1396980 - Lazily creating nsRange object when misspell word is found.

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

Comment 7

[Evelyn Hung]

Comment on attachment 8921404 [details]
Bug 1396980 - Lazily creating nsRange object when misspell word is found.

https://reviewboard.mozilla.org/r/190946/#review196674

> If MakeRange failed, should we log the failure and give up spellchecking earlier?

We just ignore the failed word and continue the while loop.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8921404 [details]
Bug 1396980 - Lazily creating nsRange object when misspell word is found.

This needs to be heavily rebased.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

er, no. searchfox was just broken.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

RawRangeBoundary doesn't quite fit well here, so use of NodeOffset makes sense.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8921404 [details]
Bug 1396980 - Lazily creating nsRange object when misspell word is found.

I'll upload a patch with some minor nits fixed.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: sp_mod.diff

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/2efa57596038633f4b81cfeef4a1b3f3bcd308e2
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=2efa57596038633f4b81cfeef4a1b3f3bcd308e2

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8940323 [details] [diff] [review]
sp_mod.diff

Adding to my feedback queue so that I'll remember to push this.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bf9c25d174c
Spending too much time making ranges for spellchecker on Facebook page, r=smaug

Comment 15

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/6bf9c25d174c