Merge the find counter and highlighter iterators into a singleton

RESOLVED FIXED in Firefox 50

Status

()

P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

(Blocks: 1 bug)

50 Branch
mozilla50
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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+
status-firefox49: unaffected → ---
tracking-firefox50: ? → ---
Keywords: regression
(Assignee)

Updated

3 years ago
Points: 2 → 3
(Assignee)

Comment 6

3 years ago
Created attachment 8764921 [details] [diff] [review]
Patch 1: Merge the find counter and highlighter iterators into a FinderIterator singleton
(Assignee)

Comment 7

3 years ago
Created attachment 8764922 [details] [diff] [review]
Patch 2: adjust tests a bit to work with the new FindIterator
(Assignee)

Updated

3 years ago
Attachment #8764921 - Flags: review?(jaws)
(Assignee)

Comment 8

3 years ago
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)
(Assignee)

Comment 9

3 years ago
Created attachment 8768918 [details]
Bug 1281421 - Merge the find counter and highlighter iterators into a FinderIterator singleton.

Review commit: https://reviewboard.mozilla.org/r/60520/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60520/
Attachment #8768918 - Flags: review?(jaws)
Attachment #8768919 - Flags: review?(jaws)
(Assignee)

Comment 10

3 years ago
Created attachment 8768919 [details]
Bug 1281421 - adjust tests a bit to work with the new FindIterator.

Review commit: https://reviewboard.mozilla.org/r/60522/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60522/
(Assignee)

Updated

3 years ago
Attachment #8764921 - Attachment is obsolete: true
Attachment #8764921 - Flags: review?(jaws)
(Assignee)

Updated

3 years ago
Attachment #8764922 - Attachment is obsolete: true
Attachment #8764922 - Flags: review?(jaws)
(Assignee)

Comment 11

3 years ago
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/
(Assignee)

Comment 12

3 years ago
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+
(Assignee)

Comment 15

2 years ago
Created attachment 8775180 [details]
Bug 1281421 - add new test to cover the new FinderIterator module code.

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)
(Assignee)

Comment 16

2 years ago
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/
(Assignee)

Comment 17

2 years ago
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+
(Assignee)

Comment 19

2 years ago
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/
(Assignee)

Comment 20

2 years ago
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/
(Assignee)

Comment 21

2 years ago
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/

Comment 22

2 years ago
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
(Assignee)

Comment 23

2 years ago
Created attachment 8775538 [details] [diff] [review]
Follow-up: ESLint fix

r=eslint itself.
Attachment #8775538 - Flags: review+

Comment 24

2 years ago
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

Comment 25

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e525e1000aa7
https://hg.mozilla.org/mozilla-central/rev/276b214dde0d
https://hg.mozilla.org/mozilla-central/rev/f962e7e8fb10
https://hg.mozilla.org/mozilla-central/rev/705ce7870772
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

Updated

2 years ago
Depends on: 1295759

Updated

2 years ago
Blocks: 1306320
(Assignee)

Updated

2 years ago
No longer blocks: 1306320
You need to log in before you can comment on or make changes to this bug.