1337869 - modalRepaintScheduler is too slow

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[(no longer active)]

Last night I was trying to figure out why my main browsing profile is nearly unusable and I profiled it.  Here is the profile: <https://clptr.io/2ksb0aU>.  We are spending 35 *seconds* running modalRepaintScheduler, and as you can see the main thread of the content process is basically unable to do any other work.

I had several windows open, with two of them having searchfox tabs open that I had the find toolbar open are.  As soon as I closed them both and removed the find bar highlight from those pages, everything went back to normal.

In order to get a sense of how reproducible this is, I just captured a new profile when I was filing this bug by hitting Cmd+F and searching for "a" and then profiling.  Here is the profile: <https://clptr.io/2ksoVh1>.  Here we only spend 650ms running this function which is quite a lot, but it means that it should be super simple to reproduce this.  I wonder if leaving the page open for a long time will make this worse, since both of the pages I mentioned above were left open for hours.

Comment 1

[(no longer active)]

One thing that I noticed was that this profile shows tons of XRay wrapper stuff (try inverting the callstack).  Bobby, Gabor, are Xray wrappers super slow these days?  Or is this code just using them too much?

Comment 2

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari from comment #1)
> One thing that I noticed was that this profile shows tons of XRay wrapper
> stuff (try inverting the callstack).  Bobby, Gabor, are Xray wrappers super
> slow these days?  Or is this code just using them too much?

They were never optimized. Bill has been looking into XrayWrapper perf.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Xrays are indeed slow, but I don't think that's the problem here. This code is just horribly inefficient. I added instrumentation to count the number of calls to _getRootBounds. It happens multiple times for every match on a page. And it re-runs every time the browser paints anything, regardless of whether the the tab is even in the foreground. As a worst case, I navigated to nsGlobalWindow.cpp in Searchfox and searched for "a" with the findbar. I saw over 350000 calls to _getRootBounds before I hit ^C.

Mike, this bug should have a high priority. It slows down Firefox without users being able to understand what's causing the problem.

Can you fill us in on what this code is doing, Mike? I've seen a similar problem where LastPass uses polling in order to position some overlay elements on a page. Maybe there's something we could add to the platform so that you could use a listener instead?

Comment 4

[Mike de Boer [:mikedeboer]]

Thanks Bill, this is indeed extremely sub-optimal. But, that also makes it a nice optimization to follow-through ;-)

I check the window dimensions this often, because the 'resize' event is not reliable to know that the page dimensions have changed due to other events that cause a page to grow or shrink than the user/ script resizing the actual browser window.
I want to know when the page content has changed in such a way that it might require us to repaint all the found ranges. Things that includes are:

1) DOM Content changed (nodes added/ removed)
2) Window was resized causing a reflow with many text frames' rects invalidated
3) CSS transitions causing elements to reposition

While number 1) may be cause to restart the nsFind iterator to find new ranges or remove them from the current set, the other two require us to re-fetch the rects of the current set of found ranges.

One thing I should really do is pause the scheduler on pagehide and unpause on pageshow.

The reason the scheduler is hit so many times is because I'm listening to the MozAfterPaint event, which is rather noisy unfortunately; not every paint means that the page content changed and some pages do stuff that cause an excessive amount of reflows/ paints, even when in a background tab.
What I'd love to have is an event that fires every time the quantity or position of text on the page may have changed. Does that sound sane?

Comment 5

[(no longer active)]

(In reply to Mike de Boer [:mikedeboer] from comment #4)
> Thanks Bill, this is indeed extremely sub-optimal. But, that also makes it a
> nice optimization to follow-through ;-)
> 
> I check the window dimensions this often, because the 'resize' event is not
> reliable to know that the page dimensions have changed due to other events
> that cause a page to grow or shrink than the user/ script resizing the
> actual browser window.
> I want to know when the page content has changed in such a way that it might
> require us to repaint all the found ranges. Things that includes are:
> 
> 1) DOM Content changed (nodes added/ removed)
> 2) Window was resized causing a reflow with many text frames' rects
> invalidated

These two would be covered by a reflow observer.  (nsIDocShell::AddWeakReflowObserver)

> 3) CSS transitions causing elements to reposition

I'm not sure about this one.. but these transitions happen on the compositor side, why do you need to know about them?

> While number 1) may be cause to restart the nsFind iterator to find new
> ranges or remove them from the current set, the other two require us to
> re-fetch the rects of the current set of found ranges.
> 
> One thing I should really do is pause the scheduler on pagehide and unpause
> on pageshow.

I'm not sure what the purpose of this code is, but it seems that we should not do anything for findbar when a tab is hidden, yes.

> The reason the scheduler is hit so many times is because I'm listening to
> the MozAfterPaint event, which is rather noisy unfortunately; not every
> paint means that the page content changed and some pages do stuff that cause
> an excessive amount of reflows/ paints, even when in a background tab.
> What I'd love to have is an event that fires every time the quantity or
> position of text on the page may have changed. Does that sound sane?

Sorry for the possibly dumb question (I haven't really read this code in any detail.)  What is the higher level thing that you are trying to do here?  The old findbar highlight all UI doesn't require any of this, and I suspect there may be much more efficient way to implement what you want by getting Gecko to help you a bit.

Comment 6

[Mike de Boer [:mikedeboer]]

(In reply to :Ehsan Akhgari from comment #5)
> These two would be covered by a reflow observer. 
> (nsIDocShell::AddWeakReflowObserver)

Cool. I'll look into that soon.

> I'm not sure about this one.. but these transitions happen on the compositor
> side, why do you need to know about them?

Well, if a reflow observer catches this too I'm a happy camper.

> Sorry for the possibly dumb question (I haven't really read this code in any
> detail.)  What is the higher level thing that you are trying to do here? 
> The old findbar highlight all UI doesn't require any of this, and I suspect
> there may be much more efficient way to implement what you want by getting
> Gecko to help you a bit.

Not a dumb question at all, don't worry!
OK, here goes: first of all, the old mode was turned OFF by default, thus sparingly used in comparison to current Nightly. Since the old mode was also performing very poorly, I needed to refactor a bunch of code to get things into a good shape. That's all good and well, but not the issue of the perf problem we're seeing here; the way we accomplish the current effect is by using the AnonymousContent API - same as devtools uses for their highlighters - to draw a div fully covering the currently active document. Whilst nsFind yields ranges, they are not added to a special nsSelection type (SELECTION_FIND) - magenta colored rects - like is the case with old UI, but instead the rects are cut out of the div using a special API call that :mstange implemented for me.
The most awesome solution from my perspective would be to be able to style ranges in the SELECTION_FIND layer as block elements (option 1) OR have the ability to make the selection layer have `background-color: rgba(0,0,0,.2)` support, yielding the same effect as the AnonymousContent div node currently, and cut out the ranges out of it using `mix-blend-mode: multiply` (option 2) or something similar (option 3).
The downside of option 1 is that we'll loose the dimmed background, in turn losing an accessibility benefit. 
Does that make sense at all?

Comment 7

[Mike de Boer [:mikedeboer]]

I forgot to mention _why_ the old mode drawing style does exhibit this perf issue: this is because they're ranges, added to this special selection type. This means that they're tightly coupled to the content, meaning that when content changes (DOM/ resize events/ CSS animations, etc), the rectangles happily chug along.
For the new highlighting mode we want the dimmed background and the ranges as cutouts from that background. This was not feasible using the 'range' representation, so I had to collect all the rects manually and throw them into :mstange's API to cut them out. This meant that
1. I had to monitor containers that affect scrolling behavior (position: fixed, position: sticky, iframes) and repaint on scroll
2. Monitor the page for resizes and other events that may have the effects of changing the rect properties of ranges found in the page. This is the modalRepaintScheduler you may have seen in the code. You also noticed that it's not perfect and that's because it's using simple heuristics instead of exact probes and/ or measurements. This approach was born out of pragmatism; this is as far as I got using JS alone and the facilities Gecko provides currently.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Mike, your comments seem to be saying that the old findbar code was slow in the "Highlight All" case. However, the way you describe the implementation, it sounds like it should be fast since it's tied directly to our layout code. Can you clarify?

> The most awesome solution from my perspective would be to be able to style ranges in the
> SELECTION_FIND layer as block elements (option 1) OR have the ability to make the selection layer have
> `background-color: rgba(0,0,0,.2)` support, yielding the same effect as the AnonymousContent div node
> currently, and cut out the ranges out of it using `mix-blend-mode: multiply` (option 2) or something
> similar (option 3).
> The downside of option 1 is that we'll loose the dimmed background, in turn losing an accessibility
> benefit. 

I'm a little confused here too. Isn't the dimmed background the main thing that the new code provides? Are there other goals that it achieves?

Overall it seems like we need to make the old findbar code do the cutout thing that you want.

Comment 9

[(no longer active)]

I'm also trying to parse what's happening here but there are a few missing pieces to this puzzle for me.  What's the "mstange API"?  :-)

The way that the current highlight all feature works is as you said by maintaining ranges of type SELECTION_FIND.  What that does inside Gecko is when we're painting text, we choose different hightlight colors if the text falls into such a range <http://searchfox.org/mozilla-central/rev/ac8a72f2d5204ced2004c93a9c193430c63a6a71/layout/generic/nsTextFrame.cpp#5984>.  This means that painting the highlighted ranges has the same performance as painting any other piece of text.  I think this is a crucial invariant we would like to keep in the new UI.

The approach of overlaying a semi-transparent element on top of the page and "cut these out" from it isn't anything that we can implement efficiently unless *it happens on the compositor.*  There's really no other option.  Remember that with APZ we want to paint these asynchronously from scrolling.

My gfx foo isn't strong enough to suggest a good solution here..  Perhaps we can get to some magic by changing the alpha channel of the highlight colors above and use the CSS blend mode?  Markus should be able to help brainstorm here.

Comment 10

[(no longer active)]

FWIW the AnonymousContent API is generally the wrong solution for the kinds of UI that needs to "attach" to content.  There are other bugs that are caused by this approach, for example try going to http://searchfox.org/, search for a letter and scroll down.  :-)

Comment 11

[Markus Stange [:mstange]]

(In reply to :Ehsan Akhgari from comment #9)
> I'm also trying to parse what's happening here but there are a few missing
> pieces to this puzzle for me.  What's the "mstange API"?  :-)

AnonymousContent::SetCutoutRectsForElement, which causes the creation of a nsDisplaySolidColorRegion.

I'd be happy to brainstorm other solutions again, but preferably in person, and preferably not today.

Comment 12

[Mike de Boer [:mikedeboer]]

(In reply to Bill McCloskey (:billm) from comment #8)
> Mike, your comments seem to be saying that the old findbar code was slow in
> the "Highlight All" case. However, the way you describe the implementation,
> it sounds like it should be fast since it's tied directly to our layout
> code. Can you clarify?

Absolutely! The part that was slow before was in JS: the find occurrence counter and highlight all functionality were invoking nsFind::FindNext in parallel - resulting in duplicate calls and a lot of overhead caused there. For this functionality to be enabled by default, I unified this into one FinderIterator singleton, which has various other features that take care of various async entry points whilst making absolutely sure that only one iterator is running at the same time. This'll make it very easy to add more bells and whistles than 'just' a counter and highlight all feature (for example, see bug 259640).
The part that was and still is fast is the rendering of simple magenta colored-blocks in nsTextFrame, like Ehsan pointed out as well. In fact, when you switch the 'findbar.modalHighlight' pref to `false`, you'll notice that the perf is actually rather great.
So now we fixed the iterator part, but the styling update was the next challenge: have a modal UI - dimmed black background, the ranges' rects as cut-outs and the currently active find occurrence displayed very specifically as a yellow box with inset box-shadow and a slight (yellow) outline. It looks great, but has its own set of challenges in the rendering department when it comes to performance due to its implementation; we're trying every trick in the duffel bag of the frontend developer to make it look like and work as good as possible - squeezing everything out the currently exposed Gecko APIs. This was taking comment 10 into account, but we tried it anyway :-) In fact, we think we've reached a state that's closer to a shippable feature than a successful prototype or experiment.

> I'm a little confused here too. Isn't the dimmed background the main thing
> that the new code provides? Are there other goals that it achieves?
> 
> Overall it seems like we need to make the old findbar code do the cutout
> thing that you want.

I set up my story above to segway into your suggestion here: yes, it achieves important other goals (the iterator) and yes, ideally we would make the 'old findbar code' do what we want.

Comment 13

[(no longer active)]

Thanks for the explanation.  Now that I understand the effect we would like to achieve, let me discuss this Markus later this week and get back to you hopefully with a better solution.  FTR I agree that with the current exposed APIs this can't really be done much better in the front-end.  :-)

Comment 14

[Mike de Boer [:mikedeboer]]

Great! FYI: Brad Werth has been helping me out lately with platform fixes and additions and I think he's been doing great and learning fast.

Comment 15

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1337869 - use a weak reflow observer instead of MozAfterPaint and use the Page Visibility API to pause the repaint scheduler.

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

Comment 16

[Mike de Boer [:mikedeboer]]

Ehsan, in the attached patch I changed the FinderHighlighter code to use a weak reflow observer and pause the repaint loop when the page is hidden.
You can try things with it applied, if you'd like. If you think it's a good improvement for the interim, I'll ask jaws to review it.

Comment 17

[Mike de Boer [:mikedeboer]]

Comment on attachment 8840915 [details]
Bug 1337869 - use a weak reflow observer instead of MozAfterPaint and use the Page Visibility API to pause the repaint scheduler.

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

Comment 18

[Markus Stange [:mstange]]

Hmm, I think a MozAfterPaint listener seems like the better solution in general, as long as you can avoid the repaint loop. I think what's happening is that you're modifying the highlight element even if its position and text doesn't change, which causes another repaint. If you only modify it if something *does* change, you shouldn't get into a loop.

Comment 19

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1337869 - Optimizations for the modal find-in-page highlighting: don't render range cut-outs when scrolling big pages or results in iframes, ignore the paint event when the highlighter caused it to fire and pause the highlighter when the document is h

This patch implements three different performance optimizations:
1. When the page is marked as 'too big' or there are ranges inside dynamic containers,
   don't draw the white cut-outs whilst scrolling the page.
2. After drawing the highlights (yellow _and_ white boxes, doesn't matter which),
   ignore the next paint event that's fired, because it's self-inflicted.
3. Don't continue to update the modal highlights when the document is not visible
   anymore, e.g. after a tab switch. Kick-off a re-render when the document becomes
   visible again.

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

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8949697 [details]
Bug 1337869 - Optimizations for the modal find-in-page highlighting: don't render range cut-outs when scrolling big pages or results in iframes, ignore the paint event when the highlighter caused it to fire and pause the highlighter when the document is h

https://reviewboard.mozilla.org/r/219034/#review224814

::: toolkit/modules/FinderHighlighter.jsm:176
(Diff revision 1)
>          detectedGeometryChange: false,
>          dynamicRangesSet: new Set(),
>          frames: new Map(),
>          lastWindowDimensions: { width: 0, height: 0 },
>          modalHighlightRectsMap: new Map(),
> -        previousRangeRectsAndTexts: { rectList: [], textList: [] }
> +        previousRangeRectsAndTexts: { rectList: [], textList: [] },

Missed a perfectly good opportunity for a better variable name, https://en.wikipedia.org/wiki/Wreckx-n-Effect

::: toolkit/modules/FinderHighlighter.jsm:1263
(Diff revision 1)
>      if (!this._modal)
>        return;
>  
>      window = window.top;
>      let dict = this.getForWindow(window);
> +    // Bail out early if the repaint schedular is paused or when we're supposed

s/schedular/scheduler/

Comment 21

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1337869 - Optimizations for the modal find-in-page highlighting: don't render range cut-outs when scrolling big pages or results in iframes, ignore the paint event when the highlighter caused it to fire and pause the highlighter when the document is h

This patch implements three different performance optimizations:
1. When the page is marked as 'too big' or there are ranges inside dynamic containers,
   don't draw the white cut-outs whilst scrolling the page.
2. After drawing the highlights (yellow _and_ white boxes, doesn't matter which),
   ignore the next paint event that's fired, because it's self-inflicted.
3. Don't continue to update the modal highlights when the document is not visible
   anymore, e.g. after a tab switch. Kick-off a re-render when the document becomes
   visible again.

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

Comment 22

[Mike de Boer [:mikedeboer]]

Jared: I have no idea what ReviewBoard just did to your previous review! It just threw it away entirely :-/

To be clear: I fixed the typo.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> Missed a perfectly good opportunity for a better variable name,
> https://en.wikipedia.org/wiki/Wreckx-n-Effect

I listened to 'Rump Shaker' and it didn't even ring a bell! :-P

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8950067 [details]
Bug 1337869 - Optimizations for the modal find-in-page highlighting: don't render range cut-outs when scrolling big pages or results in iframes, ignore the paint event when the highlighter caused it to fire and pause the highlighter when the document is h

https://reviewboard.mozilla.org/r/219340/#review225050

This usually happens when the mozreview commit ID in the commit message gets lost or changed.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8e0ed7f3b74
Optimizations for the modal find-in-page highlighting: don't render range cut-outs when scrolling big pages or results in iframes, ignore the paint event when the highlighter caused it to fire and pause the highlighter when the document is hidden. r=jaws

Comment 25

[Andreea Pavel [:apavel]]

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