Closed Bug 1396980 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Spelling checker, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: mconley, Assigned: jj.evelyn)

References

Details

(Keywords: perf, Whiteboard: [qf:p1])

Attachments

(3 files)

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.
Flags: needinfo?(ehsan)
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?
Flags: needinfo?(ehsan) → needinfo?(kchen)
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p2]
Priority: -- → P3
Keywords: perf
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.
Attachment #8919995 - Flags: feedback?(kchen)
Assignee: nobody → ehung
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.
(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
Attachment #8919995 - Attachment is patch: true
Attachment #8919995 - Attachment mime type: text/x-review-board-request → text/plain
Attachment #8919995 - Attachment is patch: false
Flags: needinfo?(kchen)
Attachment #8919995 - Flags: feedback?(kchen) → feedback+
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.
Flags: needinfo?(kchen)
Attachment #8921404 - Flags: review?(kanru) → review?(bugs)
Whiteboard: [qf:p2] → [qf:p1]
Comment on attachment 8921404 [details]
Bug 1396980 - Lazily creating nsRange object when misspell word is found.

This needs to be heavily rebased.
er, no. searchfox was just broken.
RawRangeBoundary doesn't quite fit well here, so use of NodeOffset makes sense.
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.
Attachment #8921404 - Flags: review?(bugs) → review+
Attached patch sp_mod.diffSplinter Review
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 on attachment 8940323 [details] [diff] [review]
sp_mod.diff

Adding to my feedback queue so that I'll remember to push this.
Attachment #8940323 - Flags: feedback?(bugs)
Summary: Spending too much time making ranges for spellchecker on Facebook page → [FIX] Spending too much time making ranges for spellchecker on Facebook page
Flags: needinfo?(kanru)
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bf9c25d174c
Spending too much time making ranges for spellchecker on Facebook page, r=smaug
Attachment #8940323 - Flags: feedback?(bugs)
https://hg.mozilla.org/mozilla-central/rev/6bf9c25d174c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.