Closed Bug 1281421 Opened 5 years ago Closed 5 years ago

Merge the find counter and highlighter iterators into a singleton

Categories

(Toolkit :: Find Toolbar, defect, P1)

50 Branch
defect
Points:
3

Tracking

()

RESOLVED FIXED
mozilla50
Iteration:
50.1 - Jun 20
Tracking Status
firefox50 --- fixed

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

After profiling the highlighter with the Gecko Profiler, I noticed that a large deal of time is spent running two iterators for the find counter AND the highlighter.
This is _not_ cheap, neither optimal. Therefore it's a good idea, I think, to merge the two iterators into a FinderIterator module that both can use.
It'd also provide a good place to further investigate performance and behavior tweaks.
Flags: qe-verify-
Flags: firefox-backlog+
Points: 2 → 3
Attachment #8764921 - Flags: review?(jaws)
Comment on attachment 8764922 [details] [diff] [review]
Patch 2: adjust tests a bit to work with the new FindIterator

I'm adding another patch soon with tests dedicated to the new module.
Attachment #8764922 - Flags: review?(jaws)
Attachment #8764921 - Attachment is obsolete: true
Attachment #8764921 - Flags: review?(jaws)
Attachment #8764922 - Attachment is obsolete: true
Attachment #8764922 - Flags: review?(jaws)
Comment on attachment 8768918 [details]
Bug 1281421 - Merge the find counter and highlighter iterators into a FinderIterator singleton.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60520/diff/1-2/
Comment on attachment 8768919 [details]
Bug 1281421 - adjust tests a bit to work with the new FindIterator.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60522/diff/1-2/
Comment on attachment 8768918 [details]
Bug 1281421 - Merge the find counter and highlighter iterators into a FinderIterator singleton.

https://reviewboard.mozilla.org/r/60520/#review60188

::: toolkit/modules/Finder.jsm:189
(Diff revision 2)
>      let searchString = this._fastFind.searchString;
>      this._notify({
>        searchString,
>        result: this._lastFindResult,
>        findBackwards: aFindBackwards,
> -      fidnAgain: true,
> +      findAgain: true,

yikes lol

::: toolkit/modules/FinderIterator.jsm:299
(Diff revision 2)
> +    this._catchingUp.delete(onRange);
> +  }),
> +
> +  /**
> +   * Internal; see the documentation of the start() method above.
> +   * 

nit, trailing whitespace
Attachment #8768918 - Flags: review?(jaws) → review+
Comment on attachment 8768919 [details]
Bug 1281421 - adjust tests a bit to work with the new FindIterator.

https://reviewboard.mozilla.org/r/60522/#review60190
Attachment #8768919 - Flags: review?(jaws) → review+
Review commit: https://reviewboard.mozilla.org/r/67426/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67426/
Attachment #8768919 - Attachment description: Bug 1281421 - adjust tests a bit to work with the new FindIterator and add iterator test coverage. → Bug 1281421 - adjust tests a bit to work with the new FindIterator.
Attachment #8775180 - Flags: review?(jaws)
Comment on attachment 8768918 [details]
Bug 1281421 - Merge the find counter and highlighter iterators into a FinderIterator singleton.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60520/diff/2-3/
Comment on attachment 8768919 [details]
Bug 1281421 - adjust tests a bit to work with the new FindIterator.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60522/diff/2-3/
Comment on attachment 8775180 [details]
Bug 1281421 - add new test to cover the new FinderIterator module code.

https://reviewboard.mozilla.org/r/67426/#review64528

r=me with the following question answered and possibly test adjusted

::: toolkit/modules/tests/xpcshell/test_FinderIterator.js:147
(Diff revision 1)
> +  let rangeCount = 2143;
> +  prepareIterator(findText, rangeCount);
> +
> +  // Start off the iterator.
> +  let count = 0;
> +  let whenDone = FinderIterator.start({
> +    finder: gMockFinder,
> +    onRange: range => ++count,
> +    word: findText
> +  });
> +
> +  // Start again after a few milliseconds.
> +  yield new Promise(resolve => gMockWindow.setTimeout(resolve, 2));
> +  Assert.ok(FinderIterator.running, "We ought to be running here");
> +
> +  let count2 = 0;
> +  let whenDone2 = FinderIterator.start({
> +    finder: gMockFinder,
> +    onRange: range => ++count2,
> +    word: findText
> +  });
> +
> +  // Let the iterator run for a little while longer before we assert the world.
> +  yield new Promise(resolve => gMockWindow.setTimeout(resolve, 10));
> +  FinderIterator.stop();
> +
> +  Assert.ok(!FinderIterator.running, "Stop means stop");
> +
> +  yield whenDone;
> +  yield whenDone2;
> +
> +  Assert.greater(count, 0, "At least one range should've been found");

Just a note here because I had to go back and look. Correct me if I'm wrong: The FinderIterator looks for the first kIterationMax items before pausing by using a setTimeout(..., 0). After kIterationMax items have been found, each subsequent find will trigger another setTimeout(..., 0) call.

If the caller yields on the promise returned by FinderIterator.start, then the full rangeCount will be found. But if the caller uses FinderIterator.stop(); before yielding on the promise, then the rangeCount will be equal to kIterationMax.

If however, in this last test case, the caller also calls setTimeout(..., 10) and pauses for 10 milliseconds before resuming and subsequently calling FinderIterator.stop(), the rangeCount will be equal to kIterationMax plus however many iterations FinderIterator was able to run past kIterationMax.

With all that being said and if my understanding is correct, can you update these last test cases to use Assert.greater(count, kIterationMax, ...) instead of 0?
Attachment #8775180 - Flags: review?(jaws) → review+
Comment on attachment 8768918 [details]
Bug 1281421 - Merge the find counter and highlighter iterators into a FinderIterator singleton.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60520/diff/3-4/
Comment on attachment 8768919 [details]
Bug 1281421 - adjust tests a bit to work with the new FindIterator.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60522/diff/3-4/
Comment on attachment 8775180 [details]
Bug 1281421 - add new test to cover the new FinderIterator module code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67426/diff/1-2/
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e525e1000aa7
Merge the find counter and highlighter iterators into a FinderIterator singleton. r=jaws
https://hg.mozilla.org/integration/autoland/rev/276b214dde0d
adjust tests a bit to work with the new FindIterator. r=jaws
https://hg.mozilla.org/integration/autoland/rev/f962e7e8fb10
add new test to cover the new FinderIterator module code. r=jaws
r=eslint itself.
Attachment #8775538 - Flags: review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/705ce7870772
followup - fix ESLint error, even though the syntax was correct. r=me
Depends on: 1295759
Blocks: 1306320
No longer blocks: 1306320
You need to log in before you can comment on or make changes to this bug.