429732 - Highlighting a huge amount of matches within the find toolbar takes > 1s

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008041704 Minefield/3.0pre ID:2008041704 and Windows XP.

If you search for a word on a website which has a huge amount of matches the highlight feature will completely hang Firefox. Highlighting the matches seems to be a bit faster as reverting the highlight.

Steps to reproduce:
1. Open a website e.g. bug 419731
2. Press Cmd+F or Ctrl+F
3. Search for 'a'
4. Click on "Highlight all" => hang for about 3-5 seconds
5. Click on "Highlight all" again => hang for about 20 seconds

Using the highlight feature shouldn't freeze Firefox in any way.

Comment 1

[Pavlos Touboulidis]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041807 Minefield/3.0pre ID:2008041807

Confirmed on Windows XP.

I also get the "Unresponsive script" dialog some times, Script: chrome://global/content/bindings/findbar.xml:529

Comment 2

[Boris Zbarsky [:bzbarsky]]

Profile data from Shark for turning on the highlight shows that we're spending almost 50% of the total time calling nsCOMPtr_base::assign_with_AddRef.  What we're doing is addrefing and releasing ranges.  The issue here is that each range is a document observer.  Every time the DOM is modified, existing document observers are notified.  The code in <method name="_highlightText"> in findbar.xml creates a new range for every hit, and another range is created by the Find() call.  So we have many many Range objects floating around, and each mutation (which is what the highlights are) notifies all the extant ranges.  That makes the number of notifications sent O(N^2) in the number of search hits, and the addref/release is just the part of the notification with the biggest constant.  The other parts of the notification seem to make up the rest of the time.

For turning off the highlight, the situation is the same from an algorithmic point of view, but with more DOM mutations (normalizing, removing nodes, inserting nodes, etc).

There are 3129 'a's on this page, so anything O(N^2) will bite.

Possible options here include:

1)  Changing something fundamental about ranges (not likely for 1.9)
2)  Reusing some of these Range objects so there aren't thousands of them
    floating around.  Might be hard given the Find API.
3)  Forcing GC after every so many iterations of the loop to keep the number of
    live ranges down.  Might be reasonable.
4)  Not trying to do the whole thing in one shot, possibly combined with
    forcing GC.  Might be even more reasonable.
5)  Finally forcing this highlighting to not modify the non-anonymous DOM.
    This has needed to happen for years now.

Note that 2-5 are mitigation strategies for this particular testcase, but an HTML page could pretty trivially trigger this too by creating lots of ranges and then doing some mutations.  So it might be nice to do something smarter with ranges in general, perhaps... Just not addrefing observers as we notify would help some.  Maybe with XPCOMGC we can do that?

Comment 3

[Ria Klaassen (not reading all bugmail)]

Seems a regression from Bug 358106. The time it needed to display all the highlights changed from 4 to 12 seconds on that day.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think we do want to keep ranges as nsIMutationObservers. While it might slow down this case, it helped for the general case of DOM mutations where we used to do a lot more work in order to deal with ranges.

I have to say though, i'm pretty impressed that the addref/release is the expensive part of the notifications. I was worried that the whole scheme of walking the parent chain and looking for mutationobservers everywhere was going to be costly. Similarly forcing ranges to walk the parent chain of their endpoints to see if they were removed was something I was worried about. Sounds like neither is a real problem.

Btw, how big is the profile. If you could attach it here that'd rock, otherwise i'd be interested to get a copy through other means.

Can we during unhighlighting call detach on each range? That will remove them as nsIMutationObservers, no need to wait for a GC.

And yes, a page could run into this as well, but it seems unlikely that a page would create thousands of ranges, so ideally I'd like to avoid optimizing for that.

And yes, XPCOMGC should remove the need to addref/release.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Another thing we could do is to invent another notification mechanism. If we had a separate notification mechanism for when a node was removed from its parent we could let ranges register as observers for that notification on all nodes in the parent chain. This would create a lot more observers, but we'd only notify on the actual node that was removed from its parent, which would mean that we'd notify on exactly the right set of ranges, so mutations that don't affect a range would not cause any notifications to happen at all.

I bet we could convert other nsIMutationObservers to use such a mechanism instead.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I think for the page case we should really consider just counting mutation observer notifications as part of our "slow script warning" setup.

The idea in comment 5 is something I was thinking about too; might be worth spinning off into a separate bug, since I still think that this particular issue can and should be mitigated in the findbar code.

I'll see what I can do with profiles.  Shark doesn't have a way to export them, but hopefully the .mshark files are not too big.

Comment 7

[Boris Zbarsky [:bzbarsky]]

The files are 7.5MB and 11MB for the highlight and unhighlight respectively.  I've put them at http://web.mit.edu/bzbarsky/www/profiles/bug429732/

Those files don't have the source files attached, since I haven't found a sane way to do that for the whole tree yet (plus, it might have made them that much larger).  So you'll be able to get the assembly, but not the corresponding source lines for the line-by-line blame.  The nsCOMPtr_base::assign_with_addref blame is on the NSCAP_ADDREF line, which is basically the "call" instruction.  Presumably because it's a virtual call?  But the call into the observer itself is also virtual; not sure why that's not popping up just as big.

If you do want to get the source-line information, and have a Mac, I can send you my diff to start/stop the profiler and step-by-step instructions on what to do with it to get the profiles.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hrm, bummer that shark can export anything more useful. I don't think I'll have time to do anything for the backend for 1.9, nor do I think we'd really want to take any patches there anyway. Filed bug 430002.

So I think this one should be fixed in the frontend for now.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

This will not block the final release of Firefox 3. Any patch will need unit tests in order to be approved.

Comment 10

[Boris Zbarsky [:bzbarsky]]

The shark output is pretty useful if you have a Mac... you're right that there is no sane flat-file output, sadly.

For what it's worth, I tried making the pointer in NS_OBSERVER_ARRAY_NOTIFY_OBSERVERS weak and the time as measured by Date.now() to perform highlighting did in fact drop by a factor of 2....

Comment 11

[Graeme McCutcheon [:graememcc]]

> 5)  Finally forcing this highlighting to not modify the non-anonymous DOM.
>     This has needed to happen for years now.

I've got a reviewed patch for this sitting in bug 263683, which will hopefully get checked-in soon, which should remove the main pain point. 

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Graeme, I checked with the latest trunk version and it looks much better. Highlighting takes about 1s and un-highlighting around 2,5s. Would there any more improvements possible?

Comment 13

[Graeme McCutcheon [:graememcc]]

> Would there any more improvements possible?

If bug 339400 gets fixed, then I could tighten up the unhighlight case - what we really want to do is get the "find" selection from the selection controller, and call nsISelection::RemoveAllRanges() on it. However, currently editable elements have their own independent selection controllers, making this insufficient - it would fail to remove the highlighting in any editable elements. Thus when highlighting, the code has to do a full search of the document again, just to find the matches in editable content.

Beyond that, I doubt there's much more that can be done in the front-end code. Any further improvements are likely to come from Boris' suggestions 1 - 4 in comment 2.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Improvements 1-4 won't help now that you're not munging the DOM with ranges anymore...

Profiling things as of today, it looks like we spend a lot of time executing JS and doing xpconnect silliness.  Of the 50% of total time we spend in Gecko, it looks like we're mostly doing nsTextFrame::SetSelected, which we hit in two ways: nsTypedSelection::AddRange and nsTypedSelection::Repaint.  Is there a reason we end up doing both?  The two together take about 2.5x the amount of time nsFind::Find does.

That's for turning the highlight on.  For turning it off, the issue is nsTypedSelection::RemoveAllRanges, or rather the SetSelected calls it makes.  Since each SetSelected call checks all the ranges that are still in the nsTypedSelection to see whether the textframe is selected, we have an O(N^2) algorithm again.

Comment 15

[Graeme McCutcheon [:graememcc]]

> Is there a reason we end up doing both?

That would be me, and my patch explicitly calling RepaintSelection. In my explorations trying to fathom layout code, I never looked far enough to see that AddRange would invalidate the relevant frames and cause repainting without me having to explicitly do it in the findbar. I'll patch that pronto.

As for highlight off, not sure what can be done there? Fixing bug 348681 may speed up the lookup code in nsTypedSelection slightly, but I doubt the improvement would be significant.
  

Comment 16

[Boris Zbarsky [:bzbarsky]]

> As for highlight off, not sure what can be done there?

See patch in bug 449167.  That makes highlight off be just as fast as highlight on, maybe a little faster, over here, instead of being 2-3 times slower.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Not blocking, though it looks like good progress on some of the underlying problems here might lead to this being resolved fixed by other bugs?

Comment 18

[Siddhartha Dugar [:sdrocking]]

Duplicate of Bug 251862 ?

Comment 19

[Wayne Mery (:wsmwk)]

(In reply to sdrocking from comment #18)
> Duplicate of Bug 251862 ?

I see no hint either of these current trunk. results are instant.  but I'm on a fast machine.

gone for everyone else also?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Using the steps to reproduce from comment 0 of this bug, the unhighlighting is pretty instant, but the highlighting takes a second or two (on a pretty fast machine, btw).

It may be worth resolving this and filing a new bug on that just to keep the noise down, though.

Comment 23

[Steffen Wilberg]

Changing the summary per comment 20.
I'm not cloning the bug to not spam the dependent bugs.

Comment 24

[Tomasz Kołodziejski [:tomasz]]

Attachment: find-playground.patch

I was playing around with the findbar during my second week in Mozilla. It looks like we can do the highlight all functionality incrementally in batches.

Please have a look at the above patch but just _very_high_level_ overview. It's not ready and does not work perfectly but does not freeze the browser when doing find on huge pages.

Some questions:
1. How do you like the idea itself? (do a batch and wait, do a batch and wait, ...)
2. With this patch we can get rid of Match Limit, right?

I selected you Marco for feedback as the last active person. I hope that's ok.

Comment 25

[Marco Mucci [:MarcoM]]

Hi Tomasz, sorry I can't provide feedback on your patch.  It's not my field of expertise.

(In reply to Tomasz Kołodziejski [:tomasz] from comment #24)
> Created attachment 8475625 [details] [diff] [review]
> find-playground.patch
> 
> I was playing around with the findbar during my second week in Mozilla. It
> looks like we can do the highlight all functionality incrementally in
> batches.
> 
> Please have a look at the above patch but just _very_high_level_ overview.
> It's not ready and does not work perfectly but does not freeze the browser
> when doing find on huge pages.
> 
> Some questions:
> 1. How do you like the idea itself? (do a batch and wait, do a batch and
> wait, ...)
> 2. With this patch we can get rid of Match Limit, right?
> 
> I selected you Marco for feedback as the last active person. I hope that's
> ok.

Comment 26

[Tomasz Kołodziejski [:tomasz]]

So I did a little general profiling and it looks like this:

6s total.

2s: _highlightRange. In that almost 1s is _getEditableNode. I was trying to understand why we treat editable guys differently. First, the reason it takes so long is that for every match we look all the way up the dom tree to check whether it is part of something editable to get the selectionController from that editable (see 339400). Second, the reason we use the individual selection controller for every single editable is that we listen for their update events (see nsIEditActionListener) in functions like WillDeleteText and we don't want to go through all the ranges in the entire document to find the right one to remove.
By the way, it looks like it is quite broken: try do highlight all and edit input -- removing part of the matched word does the right thing but adding new match does nothing. It also does not seem to work for contenteditable elements.

~1s: nsDefaultComparator<nsIMutationObserver*, nsIMutationObserver*>::Equals. I'm not really sure how to find out more about this guy.

Also I'm still interested in getting some feedback about the attached patch. I needinfo ehsan as he was interested on IRC.

Comment 27

[(no longer active)]

(In reply to Tomasz Kołodziejski [:tomasz] from comment #26)
> So I did a little general profiling and it looks like this:
> 
> 6s total.
> 
> 2s: _highlightRange. In that almost 1s is _getEditableNode. I was trying to
> understand why we treat editable guys differently. First, the reason it
> takes so long is that for every match we look all the way up the dom tree to
> check whether it is part of something editable to get the
> selectionController from that editable (see 339400). Second, the reason we
> use the individual selection controller for every single editable is that we
> listen for their update events (see nsIEditActionListener) in functions like
> WillDeleteText and we don't want to go through all the ranges in the entire
> document to find the right one to remove.

I think _getEditableNode can be heavily simplified.  I chatted with Tomasz about this on IRC, and explained what needs to happen, but in summary, that function just needs to check to see if its argument is a text node, and if so see if the parent's parent QIs to nsIDOMNSEditableElement succeeds.  It doesn't need to walk up the entire parent chain.

Once you've made that change, you need to reprofile.

> By the way, it looks like it is quite broken: try do highlight all and edit
> input -- removing part of the matched word does the right thing but adding
> new match does nothing. It also does not seem to work for contenteditable
> elements.

Hmm, these are probably bugs that need to be filed separately.

> ~1s: nsDefaultComparator<nsIMutationObserver*,
> nsIMutationObserver*>::Equals. I'm not really sure how to find out more
> about this guy.

It's hard to figure out what this means without having a call stack.  All I can say is this probably an nsTArray of mutation observers...  I asked Tomasz to give me a profile URL so that I can try to make more sense out of it.

> Also I'm still interested in getting some feedback about the attached patch.
> I needinfo ehsan as he was interested on IRC.

I think the overall approach of doing things incrementally makes sense, but I think we should first see if there is any low hanging fruits for better performance before we make the code here more complicated by making it do its work in chunks.  (But I didn't look at the patch very closely.)

Comment 28

[Tomasz Kołodziejski [:tomasz]]

Here is the example http://people.mozilla.org/~bgirard/cleopatra/#report=01388293ab17270617bac204e58bd4fe13164f2d.

For page with deep dom structure the numbers in _getEditableNode are even higher. So I guess that we can first try to improve it as ehsan suggests.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> I'm not really sure how to find out more about this guy.

The linked profile doesn't so much have useful stacks, but it's pretty clear that there's an nsTArray<nsIMutationObserver*> that is getting IndexOf called on it.

That would presumably be the mMutationObservers of a node (in the slots).  And the indexof is likely from AddMutationObserverUnlessExists?  But the only callers of that are HTMLEditor and form fill, which should not be relevant here.

Tomasz, were you perhaps profiling a debug build?  In a debug build, AddMutationObserver will assert that the observer is not already in the list, which makes adding lots of ranges O(N^2) in debug builds but O(N) in opt builds...

Comment 30

[Tomasz Kołodziejski [:tomasz]]

Yes, I was profiling debug build. I thought I should do that to get symbols but now I realize that's not too clever to profile not optimized version :-D I'll try to play with optimized one.

Comment 31

[Boris Zbarsky [:bzbarsky]]

You should just build an optimized build with symbols and profile that.

Comment 32

[(no longer active)]

Clearing the needinfo request until we have a profile of an optimized build.

Comment 33

[Tomasz Kołodziejski [:tomasz]]

Attachment: find-geteditablenode.patch

Before:
http://people.mozilla.org/~bgirard/cleopatra/#report=6bfe456c0ff22d766ef839bd38a6ac5aaae0ec4f&search=getEditableNode

After:
http://people.mozilla.org/~bgirard/cleopatra/#report=4d4043629579897daf6149beee146d116f6a7fbd&search=getEditableNode

That is: 3282 ms down to 414 ms.

In the attachment you'll find the patch that implements this improvement.


After this change it looks like the big player here is nsINode::AddMutationObserver being called from _findIterator. I'd be happy to hear some clues as why and from where it may be called.

Comment 34

[Tomasz Kołodziejski [:tomasz]]

I did some lldb research and I've found that nsRange::DoSetRange (nsRange.cpp) is calling AddMutationObserver that is defined in nsINode.h. However, I'm not sure why it does take so significant part of the search time. Any suggestions?

Comment 35

[Boris Zbarsky [:bzbarsky]]

You should use Node.TEXT_NODE instead of hardcoding the value.

I don't see AddMutationObserver much in the "After" profile in comment 33.  Just tons of JS/XPConnect stuff.  Am I just missing something?

Comment 36

[(no longer active)]

Comment on attachment 8477068 [details] [diff] [review]
find-geteditablenode.patch

Review of attachment 8477068 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below addressed.  But I think you may want to move this patch to its own bug, as there might be more that we need to do in order to fix this bug.

::: toolkit/modules/Finder.jsm
@@ +620,5 @@
>     * @returns the first node in the parent chain that is editable,
>     *          null if there is no such node
>     */
>    _getEditableNode: function (aNode) {
> +    // For explanaition see: Bug 429732, comment 27.

It would be better to actually explain what this check does here instead of referring to the bug.

@@ +622,5 @@
>     */
>    _getEditableNode: function (aNode) {
> +    // For explanaition see: Bug 429732, comment 27.
> +    const TEXT_NODE = 3;
> +    if (aNode.nodeType === TEXT_NODE && aNode.parentNode && aNode.parentNode.parentNode &&

Please use aNode.TEXT_NODE instead of this const.

Comment 37

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #35)
> I don't see AddMutationObserver much in the "After" profile in comment 33. 
> Just tons of JS/XPConnect stuff.  Am I just missing something?

Please delete the getEditableNode filter from the profile and invert the callstacks.

Tomasz, your profile is missing the C++ callstacks.  Can you please capture one with them, so that we can know more about what's going on here outside of the JS code?

Comment 38

[Boris Zbarsky [:bzbarsky]]

> Please delete the getEditableNode filter from the profile 

Oh.  Yeah, that's not helpful.  ;)

So AddMutationObserver is almost certainly happening from the creation and addition of new ranges.  Why it's taking so long is not clear from this profile; perhaps it has to realloc a bunch?  The ranges are likely the ones that _findIterator creates; it's super-range-creation-happy afaict.

I also can't reproduce that profile, fwiw; I'm seeing quite different hotspots, while highlighting all matches for "e" on <http://www.whatwg.org/specs/web-apps/current-work/>.  Mostly under nsRange::Release, under Find(), fwiw, calling _RemoveMutationObserver_.  Which would have to do a bunch of memmoving, I expect...

Comment 39

[Tomasz Kołodziejski [:tomasz]]

Attachment: find-geteditablenode.patch

I addressed the comments and fixed a bug. I returned the wrong node from that function.

Is that really the case that we have almost no tests for this module? browser_Finder.js does not look like testing much.

Comment 40

[Tomasz Kołodziejski [:tomasz]]

I did another profile on http://www.w3.org/html/wg/drafts/html/master/single-page.html (I searched for a) with flags:

ac_add_options --disable-debug
ac_add_options --enable-optimize
ac_add_options --enable-profiling

It is available here: http://people.mozilla.org/~bgirard/cleopatra/#report=ecfb37c4d02ae539f96bd1eb45b0f6f9e9f97034. But I don't get it a little: nsRange::DoSetRange is reported to take forever but it has no calls inside. But it looks for me that that function does not do anything on its own. Maybe that's because of compiler optimizations?

Comment 41

[Boris Zbarsky [:bzbarsky]]

DoSetRange calls things, but those things are inlined.  I fully expect that what it's doing is described in comment 38.

Comment 42

[(no longer active)]

Comment on attachment 8477551 [details] [diff] [review]
find-geteditablenode.patch

Review of attachment 8477551 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/Finder.jsm
@@ +622,5 @@
>     *          null if there is no such node
>     */
>    _getEditableNode: function (aNode) {
> +    if (aNode.nodeType === aNode.TEXT_NODE && aNode.parentNode && aNode.parentNode.parentNode &&
> +        aNode.parentNode.parentNode instanceof Ci.nsIDOMNSEditableElement) {

Nit: please wrap this instanceof check in a pair of parenthesis.  I'm always uncomfortable reading code like this because I can never remember the operator precedence rules!

Comment 43

[(no longer active)]

(In reply to Tomasz Kołodziejski [:tomasz] from comment #39)
> Is that really the case that we have almost no tests for this module?
> browser_Finder.js does not look like testing much.

Yes, this is super old code.  It would be great if you could add some tests here.

Comment 44

[(no longer active)]

Tomasz, can you please add some debugging printfs to nsINode::Add/RemoveMutationObserver to see how many mutation observers we typically have here?

I have definitely seen perf problems similar to this in the past which are caused by allocating and deallocating and memmoving things inside an array too many times.  It would be nice to see if we can do something along the lines of bumping up the 0 here <http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTObserverArray.h#427> to reserve some space inside this array to eliminate some of that overhead.  (But it won't be as simple as that since we can't just increase the size of nsINode as that can affect the memory usage of all pages negatively.)

Comment 45

[Boris Zbarsky [:bzbarsky]]

> how many mutation observers we typically have here?

For the case described in comment 40, I would expect "millions".

Comment 46

[Tomasz Kołodziejski [:tomasz]]

Attachment: find-playground.patch

I updated my original patch. It works very decent with huge websites like html single-page draft.

I got rid of matches limit entirely as well as matches related timeouts. I'm using Task.js to waterfall the async code and do the sleep.

Also the usage of Finder changed a little bit so I should update all the code using it. This is not included in this patch.

Function doing count and function doing highlight got merged. I should probably rename requestMatchesCount to something more adequate.

The bad news is that it crashes e10s. I'll dig into that on Monday.

Comment 47

[(no longer active)]

Comment on attachment 8477754 [details] [diff] [review]
find-playground.patch

Review of attachment 8477754 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry this patch is using too many fancy JS stuff (yield/Task.jsm/etc) for me to be able to understand it.  Please ask someone like Mike de Boer or Gavin to look at the patch.  And do let me know if you have any other questions.  Thanks!

Comment 48

[(no longer active)]

Also, make sure you fix the e10s crash.  Ping mconley if you need help with e10s stuff.  And please remember to add tests.

Comment 49

[Tomasz Kołodziejski [:tomasz]]

Try run for find-geteditablenode.patch https://tbpl.mozilla.org/?tree=Try&rev=8de84e77b8dd. This patch is ready. Should I mark it as checkin-needed, ehsan? (Will it be clear for people checking the code to pick up only that patch?)

Comment 50

[(no longer active)]

(In reply to Tomasz Kołodziejski [:tomasz] from comment #49)
> Try run for find-geteditablenode.patch
> https://tbpl.mozilla.org/?tree=Try&rev=8de84e77b8dd. This patch is ready.
> Should I mark it as checkin-needed, ehsan? (Will it be clear for people
> checking the code to pick up only that patch?)

Awesome!

I suggest filing a new bug, attaching the patch there, marking it as checkin-needed and saying that the patch has r=ehsan.  Once you have filed that bug, please make this one depend on it.  Thanks!

Comment 51

[Tomasz Kołodziejski [:tomasz]]

Mostly note to myself but if someone has further insight please comment.

There is another "backend" (pdfjs) for findbar.xml which is the reason we are calling dispatchFindEvent. It is mostly behaving as I tried to make the Finder.jsm behave: the backend is holding the state and updating it on changes (for example: when case sensitivity is changed the backend automatically does the search and calls updateControlState. But Finder.jsm uses onFindResult and onMatchesCountResult). It also looks like in pdf case we are using both the pdf finder (see below) to do the search and highlight and standard finder to count matches which looks very strange.

There is also RemoteFinder.jsm which I didn't touch yet.

This implies that before trying to do something complicated we should simplify what we have now. Especially, we should define interface of findbar.xml and probably move the logic to Finder.jsm.

Event emitting from internal UI:
http://hg.mozilla.org/mozilla-central/file/eed9fe35a00d/browser/extensions/pdfjs/content/web/viewer.js#l738

Event handling:
http://hg.mozilla.org/mozilla-central/file/d728b2f31c56/browser/extensions/pdfjs/content/web/viewer.js#l990

Comment 52

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/ed1973bacb15

Comment 53

[Tomasz Kołodziejski [:tomasz]]

I'm removing fixed-in-fx-team flag as only the bug 1060510 is fixed now which should improve performance a little bit.

Comment 54

[Tomasz Kołodziejski [:tomasz]]

Attachment: find-yield.patch

Because of the code being a little bit more complicated than I thought (especially the pdf dependency) I created a solution with the least amount of general refactoring that should be easy to follow.

Comment 55

[Tomasz Kołodziejski [:tomasz]]

If anyone's interested I created a related bug 1062025 that follows the investigation done in comment 51.

Comment 56

[Ekanan Ketunuti]

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

Comment 57

[Tomasz Kołodziejski [:tomasz]]

I'm reopening. It's only a small improvement that got in right now (bug 1060510) and I wouldn't call it fixing this bug.

Comment 58

[Tomasz Kołodziejski [:tomasz]]

I did try run for this patch which strangely fails:

https://tbpl.mozilla.org/?tree=Try&rev=5c1aad691c12

After increasing test timeouts it passes:

https://tbpl.mozilla.org/?tree=Try&rev=c85b5fb180ad

I tried to find the reason but I couldn't without windows or linux machine. If anyone is interested in helping me I'd be very happy :-)

Comment 59

[Mike de Boer [:mikedeboer]]

Comment on attachment 8482852 [details] [diff] [review]
find-yield.patch

Review of attachment 8482852 [details] [diff] [review]:
-----------------------------------------------------------------

\o/ using iterators for the win!

Thanks so much for working on this.

I found one issue, but it's serious enough to me to r- this specific patch. Don't see this as a rejection of the entire patch.

In fact, I think the next patch you'll upload will be ready to land, so can you push it to try as well?

::: toolkit/modules/Finder.jsm
@@ +357,5 @@
>     *        the Function to invoke when a word is found. if Boolean `false` is
>     *        returned, the find operation will be stopped and the Function will
>     *        not be invoked again.
>     */
> +  _findIterator: function*(aWord, aWindow) {

nit: please follow a common style when using asterisk functions:

```js-ish
function*<space>(aWord,<space>aWindow)<space>{
```

@@ +401,5 @@
> +  }),
> +
> +  _abortHighlight: null,
> +  _highlightSleep: function(delay) {
> +    return new Promise(function (resolve, reject) {

Please make this an arrow-function so you don't need the bind().

@@ +402,5 @@
> +
> +  _abortHighlight: null,
> +  _highlightSleep: function(delay) {
> +    return new Promise(function (resolve, reject) {
> +      this._abortHighlight = reject;

Please make this something like:

```js
this._abortHighlight = () => {
  this._abortHighlight = null;
  reject();
};
```

Otherwise the _abortHighlight property never get reset to not being a function!

Comment 60

[Tomasz Kołodziejski [:tomasz]]

Attachment: find-yield.patch

I did some changes:

1. Addressed your comments
2. Modified tests. After my changes (as I mentioned in comment 58) tests waiting for highlight started to fail on Linux. I changed them so that they wait for event instead of for some specific timeout. I have no Linux machine with me so I couldn't really reproduce that problem.
3. I changed timeout to 0. It works still smoothly but faster.

I pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=d20b405d5f8f

What's left in this bug (assuming we'll push sooner or later my changes) is that removeAllRanges can take a little bit too long. However, I'm not sure if I'm going to address this here or just post a follow-up bug.

Comment 61

[Mike de Boer [:mikedeboer]]

Comment on attachment 8486552 [details] [diff] [review]
find-yield.patch

Review of attachment 8486552 [details] [diff] [review]:
-----------------------------------------------------------------

Getting close! I have a couple of questions about the highlighting code.

::: toolkit/modules/Finder.jsm
@@ +391,5 @@
>  
> +  _highlightIterator: Task.async(function* (aWord, aWindow, aOnFind) {
> +    let count = 0;
> +    for (let range of this._findIterator(aWord, aWindow)) {
> +      aOnFind(range);

since you don't do anything anymore with the `aOnFind` return value, why don't you just yield the range?

Then this'd become a nested iterator... how wild!

@@ +392,5 @@
> +  _highlightIterator: Task.async(function* (aWord, aWindow, aOnFind) {
> +    let count = 0;
> +    for (let range of this._findIterator(aWord, aWindow)) {
> +      aOnFind(range);
> +      if (++count >= 100) {

Please don't use constants like this, but define them on top like:

```js
const kHighlightIterationSizeMax = 100;
```

A bit verbose, I know, but it's important to keep things legible for our future generations!

@@ +394,5 @@
> +    for (let range of this._findIterator(aWord, aWindow)) {
> +      aOnFind(range);
> +      if (++count >= 100) {
> +          count = 0;
> +          yield this._highlightSleep(0);

what I mentioned above also applies here.

Comment 62

[Tomasz Kołodziejski [:tomasz]]

Attachment: find-yield.patch

I updated the comments and addressed the issues. However, I'm not sure about the last thing you said. It's probably about 0, but 0 means immediate. Should I make it a constant as well?

The reason that there is aOnFind is that that one iterator is async. I can't both yield a range and yield a timeout. Well, technically I think I can - I can pass the last range to the highlightSleep so it resolves with it but I think it will make it more difficult to follow.

Comment 63

[Mike de Boer [:mikedeboer]]

(In reply to Tomasz Kołodziejski [:tomasz] from comment #62)
> Created attachment 8486731 [details] [diff] [review]
> find-yield.patch
> 
> I updated the comments and addressed the issues. However, I'm not sure about
> the last thing you said. It's probably about 0, but 0 means immediate.
> Should I make it a constant as well?

You're right, not necessary.

> The reason that there is aOnFind is that that one iterator is async. I can't
> both yield a range and yield a timeout. Well, technically I think I can - I
> can pass the last range to the highlightSleep so it resolves with it but I
> think it will make it more difficult to follow.

Alright, I agree. Thanks for the explanation! Could you add that in a comment atop the new functions?

Also, I'm missing the changes in findbar.xml and findbar_window.xul... is that intentional? And when you push a patch to try, could you mention the push url in a comment? I'm always interested in the results! Showing a green run will assure sheriffs (and us) that the landed patch will have less chance to get backed out right away.

Comment 64

[Mike de Boer [:mikedeboer]]

Comment on attachment 8486731 [details] [diff] [review]
find-yield.patch

Postponing review, see my question in comment 63.

Comment 65

[Tomasz Kołodziejski [:tomasz]]

I undid the tests changes on purpose since I had problems finishing the test. The reason is that findbar.xml does not call requestMatchesCount if there are no matches which results in no event fired and so I couldn't check the case where there were 0 matches by adding my custom listener to finder.

I did a try push but it fails:
https://tbpl.mozilla.org/?tree=Try&rev=d3b057381a5a

The problem is that it fails on linux and I cannot reproduce it on my ubuntu. Any ideas as how to approach this?

Comment 66

[Tomasz Kołodziejski [:tomasz]]

Ok, it looks like I know how to reproduce the error:
1. start compilation on vm host
2. run tests on ubuntu in vm
3. test is failing

Other choice is to put an execution cap on a processor for that vm.

I made highlight more asynchronous and on a slow machine the timeout waiting for results may fire before the results are there. I think I will get back to the tests modification that I did in a previous patch and I will change the logic of findbar itself so that it asks for matches always (even if it knows that there are 0 matches). This way Finder.jsm can do that logic ("I know that the last result was not find so I'll emit that there are 0 matches") and I could add another observer of Finder.jsm which validates the behavior of findbar.

Does this small change seem ok?

Comment 67

[Tomasz Kołodziejski [:tomasz]]

Attachment: find-yield.patch

So I did what I described in my previous comment. I like how the async test looks with generator. It now passes on try: https://tbpl.mozilla.org/?tree=Try&rev=6f103101732c.

But I'm still confused as why there is updateMatchesCount called in toggleHighlight. Changing highlight does not affect matches count. It looks like it is there for a reason because even you (?) left a comment there but not very descriptive. The only thing I can imagine is that we want to trigger it because page might have changed.

For me it would make the most sense for Finder.jsm to automatically call requestMatchesCount as it knows whether something has changed or not and findbar.xml should just show the result without having to maintain that logic. However, this bug is not a good place for a refactor like that.

Comment 68

[Mike de Boer [:mikedeboer]]

Comment on attachment 8489036 [details] [diff] [review]
find-yield.patch

Review of attachment 8489036 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed. Thanks Tomasz, this looks quite nice!

::: toolkit/content/tests/chrome/findbar_window.xul
@@ +444,3 @@
>            } else {
> +            assertMatches(test, matches);
> +            for (let i = 1; i < 2 * test.total; i++) {

Please move the iteration length to a variable declaration of its own. Also provide a comment of why the doubling the total amount matters.

@@ +447,4 @@
>                gFindBar.onFindAgainCommand();
> +              yield;
> +              assertMatches({
> +                current: (test.current + i - 1) % test.total + 1,

Please add a comment above this line that explains what this does; it might seem straightforward to you atm, but I'm sure someone unfamiliar with the code will trip here ;)

Comment 69

[Tomasz Kołodziejski [:tomasz]]

Attachment: find-yield.patch

I addressed those nits. There is no good reason for 2* there. I was just making sure locally that it works as expected but it does not add to the tests so much. And I added the comment about modulo trickery (modulo by design works well with 0, ..., n-1 and so making it work with 1, ..., n requires some extra).

Final push to try, hopefully will turn green:
https://tbpl.mozilla.org/?tree=Try&rev=c9e291613f4b

Comment 70

[Tomasz Kołodziejski [:tomasz]]

Attachment: find-yield.patch

I'm sorry about the last change. I looped one element too much and it changed the first item found in the next test. Sounds like that test is a little fragile. This time it is green and ready.

https://tbpl.mozilla.org/?tree=Try&rev=4b76f13cbc5e

Should I already checkin-needed this patch?

Comment 71

[Mike de Boer [:mikedeboer]]

Comment on attachment 8491096 [details] [diff] [review]
find-yield.patch

Review of attachment 8491096 [details] [diff] [review]:
-----------------------------------------------------------------

You can upload a new patch with the two nits addressed. I'd also like to see the commit message updated to be more descriptive - someone should be able to understand what this commit was all about when hunting down a regression a year from now.

After that you can set the checkin-needed keyword on the bug.

::: toolkit/content/tests/chrome/findbar_window.xul
@@ +447,4 @@
>                gFindBar.onFindAgainCommand();
> +              yield;
> +              // test.current + 1, test.current + 2, ..., test.total, 1, ..., test.current
> +              let current = (test.current + i - 1) % test.total + 1; 

nit: trailing whitespace

::: toolkit/modules/Finder.jsm
@@ +282,3 @@
>    requestMatchesCount: function(aWord, aMatchLimit, aLinksOnly) {
> +    if (this._lastFindResult == Ci.nsITypeAheadFind.FIND_NOTFOUND
> +        || this.searchString == "") {

nit: usually we put the operator before the linebreak.

Comment 72

[Tomasz Kołodziejski [:tomasz]]

Attachment: find-yield.patch

It should be ready to go now! :-)

Comment 73

[Tomasz Kołodziejski [:tomasz]]

The checkin-needed is for find-yield.patch. The other one got checked in in bug 1060510.

Comment 74

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d22a8a36638

Comment 75

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2d22a8a36638

Comment 76

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:
FF 35
Build Id:20140921030208
OS: Win 7 x64, Ubuntu 12.10 x64, Mac Os X 10.9.5