1280978 - [meta] New find in page highlighting is very slow

Status: NEW

(Toolkit :: Find Toolbar, defect)

Description

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Significant user-facing perf regression.

This is a regression from bug 384458.

STEPS TO REPRODUCE (on Mac):

1)  Load http://html.spec.whatwg.org/
2)  Hit Cmd+F.
3)  Type "div".
4)  Hit Cmd+G a few times.

EXPECTED RESULTS: The initial typing of "div" finds the first result pretty much instantaneously.  Each Cmd+G moves to the next search result pretty much instantaneously, like it used to before bug 384458.

ACTUAL RESULTS: The initial search and each Cmd+G takes a quite noticeable amount of time (order of hundreds of milliseconds for me on a newish laptop).  The whole thing feels very laggy.

ADDITIONAL INFORMATION: I heard several people complain about this at the all-hands...

Comment 2

[Andreas Tolfsen ❲:ato❳]

It’s worthwhile to mention that it appears that the second time you cycle through results, it appears a lot faster.  Note that this is based purely on my subjective impression.

Because the jumping to next result can be slow the animation where an occurrence that has a white background gets highlighted yellow, the foreground text changes colour only after the background animation stops.  Perhaps a piece to this is changing the foreground colour first.

Finally, it would be interesting to see if these problems persist when searching the HTML spec with animations turned off completely.  Maybe this is a graphics performance problem?

Comment 3

[Marcia Knous [:marcia]]

Tracking this performance regression for 50.

Comment 4

[Mike de Boer [:mikedeboer]]

Markus, I've discussed this with you before, so I'm clinging to you for a bit of help here...
I've done a fair bit of profiling and call-stack analysis and at this point I'm quite convinced that there's a lot to be won on the platform side.
When I restrict the highlighter code to insert a dimmed rectangle of _only_ the viewport dimensions (thus _not_ the full page dimensions) only once, it takes roughly two seconds to appear on my MBP, debug build of the fx-team branch. Using the WhatWG page, of course. I also insert and position the yellow outline once in the same branch.
I get the feeling that the time to render grows, the larger the page is (large height, in this case). So I'm suspecting that CanvasFrame contents doesn't get the most optimal path to the rendering pipeline?

I'm going to implement a debug mode, which doesn't use CanvasFrame and the Anonymous Content API, but will insert DIVs right into the page itself with a very high z-index, when active. It'll be useful in other areas too, because right now the nodes can not be inspected using the devtools.

Comment 5

[Mike de Boer [:mikedeboer]]

Looking at profiles a bit more, I see that running two nsFind iterators at the same time (matches counter and highlight all) is probably not the smartest way about it.
I think it's worthwhile to investigate merging the two iterator into one.

Comment 6

[Mike de Boer [:mikedeboer]]

What I'm also seeing is that _getWindowDimensions() is causing a layout flush, which is triggered by window.scrollMaxX, window.scrollMaxY, window.scrollMinX and window.scrollMinY.
I can envision a method on nsDOMWindowUtils that implements a version that does not flush, but does `EnsureSizeUpToDate()` instead.

Comment 7

[Boris Zbarsky [:bzbarsky]]

scrollMax* needs to know the size of the content to be scrolled, no?  How would just ensuring the size of the _window_ being up to date help with that at all?

Comment 8

[Mike de Boer [:mikedeboer]]

Well, if you say so; I was copying the train of thought as present in nsGlobalWindow::GetScrollXY, which flushes conditionally.
Since the highlight does not mutate the content document but only observes it, I don't want it force a flush when inspecting its properties. Does that make sense?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> I was copying the train of thought as present in nsGlobalWindow::GetScrollXY,

GetScrollXY basically avoids the flush if and only if the scroll position is "at the top or left".  If it is, it doesn't matter how big the content is, obviously.  If it's not, it proceeds to flush and try again.

> I don't want it force a flush when inspecting its properties.

Do you have a way to update if the document then changes under you?  Because we could certainly add completely non-flushing things here if they would be good enough for what you care about.

Comment 10

[Nathan Froyd [:froydnj]]

I have noticed this while searching through build logs on treeherder.  The animations (?) of the highlight moving between matches are also quite visible, e.g.

http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-f27be3c0c1e7d07f5cb47c48f7a2f718d728c4d4/try-macosx64/try-macosx64-bm87-try1-build18724.txt.gz

and search for "libffi", then ctrl-g a couple of times.

Comment 11

[Markus Stange [:mstange]]

(In reply to Mike de Boer [:mikedeboer] from comment #4)
> I get the feeling that the time to render grows, the larger the page is
> (large height, in this case).

Does this also happen if you disable APZ? (layers.async-pan-zoom.enabled)
If not, then the slowdown comes from the display port: We paint more than what's visible so that scrolling doesn't insta-checkerboard.

The most important thing we need to do to get better performance is to stop using a mask for the dimmer. If the proposed mix-blend-mode approach isn't fast enough, then we can do something more involved. For example, the highlighter JS could compute the dimmer region using emscripten'd pixman regions code and assemble the dimmer from individual elements that just have a partially transparent background color and no other effects. And then on the platform side we could add support for ColorLayers that are not simple rectangles but regions, and add support for creating such ColorLayers to FrameLayerBuilder.

> I'm going to implement a debug mode, which doesn't use CanvasFrame and the
> Anonymous Content API, but will insert DIVs right into the page itself with
> a very high z-index, when active. It'll be useful in other areas too,
> because right now the nodes can not be inspected using the devtools.

I agree that such a mode should be very helpful for investigating the performance and playing with solutions.

(In reply to Mike de Boer [:mikedeboer] from comment #6)
> What I'm also seeing is that _getWindowDimensions() is causing a layout
> flush, which is triggered by window.scrollMaxX, window.scrollMaxY,
> window.scrollMinX and window.scrollMinY.
> I can envision a method on nsDOMWindowUtils that implements a version that
> does not flush, but does `EnsureSizeUpToDate()` instead.

Why do you need these values? Is it just to make the dimmer cover the whole page? If so, is there a way to express this in CSS without setting values in pixels?

Comment 12

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1280978 - don't show the modal highlighting ui on very large pages, because it degrades performance too much.

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

Comment 13

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbbc554490af

Comment 14

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

Comment on attachment 8775940 [details]
Bug 1280978 - don't show the modal highlighting ui on very large pages, because it degrades performance too much.

https://reviewboard.mozilla.org/r/67952/#review65142

This is fine, but it doesn't fix this bug for me. Modal highlight find-in-page is still very slow. This bug should become a meta bug, and this patch moved to a new bug that blocks this one.

We are creating one very large <div> that covers the full scroll length of the content then using `mixed-blend-mode:mulitply` to cut out parts of the <div>. It seems this is inherently expensive for long pages suchs as the HTML spec. Can you separate out the dimming and highlighting? If the highlighting alone has acceptable performance we may want to see if it is viable to ship by itself.

::: toolkit/modules/FinderHighlighter.jsm:104
(Diff revision 1)
>  }
>  
>  .findbar-modalHighlight-outlineMask[brighttext] > .findbar-modalHighlight-rect {
>    background: #000;
>  }`;
>  const kXULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";

kXULNS is unused.

Comment 15

[Mike de Boer [:mikedeboer]]

(In reply to Markus Stange [:mstange] from comment #11)
> Why do you need these values? Is it just to make the dimmer cover the whole
> page? If so, is there a way to express this in CSS without setting values in
> pixels?

I need these values to make it cover the whole page, indeed. Unfortunately, there is no CSS equivalent for this.

(In reply to Boris Zbarsky [:bz] from comment #9)
> > I don't want it force a flush when inspecting its properties.
> 
> Do you have a way to update if the document then changes under you?  Because
> we could certainly add completely non-flushing things here if they would be
> good enough for what you care about.

I have a way of updating when the document changes, so that's taken care of. I'm currently looking into adding non-flushing counterparts.

Comment 16

[Mike de Boer [:mikedeboer]]

`nsIDOMWindowUtils::GetBoundsWithoutFlushing(aNode)` does what I need when passing in document.body, except it may return `0` for 'width' or 'height' when a flush is pending/ they couldn't be calculated for some reason.
So what I'm going to do here is use it when I can and resort back to the will-flush method when I get invalid numbers back.

Comment 17

[Mike de Boer [:mikedeboer]]

(In reply to Markus Stange [:mstange] from comment #11)
> Does this also happen if you disable APZ? (layers.async-pan-zoom.enabled)
> If not, then the slowdown comes from the display port: We paint more than
> what's visible so that scrolling doesn't insta-checkerboard.

This also happens when I disable APZ.

Comment 18

[Andrew Overholt [:overholt]]

[Tracking Requested - why for this release]: we should track this for 51.

I was told the new find bar isn't riding the trains with 50 and I can't see it in today's dev edition build so marking not affected.

Comment 19

[Marcia Knous [:marcia]]

Tracking 51+ based on Comment 18. Removing tracking + flag for 50.

Comment 20

[Mike de Boer [:mikedeboer]]

All dependencies resolved, marking this one fixed.

Comment 21

[Gerry Chang [:gchang]]

Mark 51 as fixed as all dependencies are resolved.

Comment 22

[Boris Zbarsky [:bzbarsky]]

The dependencies might be fixed, but the bug as filed is not fixed.  It still reproduces just fine with the steps from comment 0.

The performance is slightly better, but still really laggy.  At least the first three times I hit Cmd+G.  It becomes a bit faster for a few times after that, as long as the page doesn't have to scroll to show the new hit.  Once it has to scroll again, it's dog-slow again.

Testing on the September 19 nightly here.

Comment 23

[Mike de Boer [:mikedeboer]]

True! I still need to use Markus' new API.

Comment 24

[Mike de Boer [:mikedeboer]]

Boris, when you look at the latest Nightly, what are your thoughts on the performance of the find toolbar?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Still looks clearly slower than Firefox 49 to me...  That's using this morning's nightly.

Comment 26

[Tanvi Vyas[:tanvi]]

This seems to have gotten worse in the last couple weeks.  I use the find bar in Nightly all the time and haven't had a big issue until now.  When searching large files, the find bar searches the entire file for the first letter I type instead of the whole string.  The browser hangs before I can type the more characters, while it searches through the large file for the letter "i" (for example).  I have to wait about 2 minutes before the browser responds again.

I'd rather disable the find bar entirely to prevent from getting into this situation.  Since I use it so frequently, its second nature and I forget that using it will hang my browser.

Comment 27

[Mike de Boer [:mikedeboer]]

Yeah, this is one of those 'first we make it slower, then we make it faster than before' kind of things.
We will obviously not ship something that hangs your browser.

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

Mike, are you still planning to ship this to Beta 51? We do have another week and a half till merge/release on Nov. 14th/15. But if don't think you will get to it, I'd rather disable the feature for beta 51 before the merge.

Comment 29

[Mike de Boer [:mikedeboer]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #28)
> Mike, are you still planning to ship this to Beta 51? We do have another
> week and a half till merge/release on Nov. 14th/15. But if don't think you
> will get to it, I'd rather disable the feature for beta 51 before the merge.

Hi Liz, if all is well, this shouldn't be enabled on Fx 51. In fact, it should be enabled on Nightly only for now. We'll see if we can shape this up to be good enough to ride the trains further in Fx 53.

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

Thanks Mike. That sounds good to me!

Comment 31

[Mike de Boer [:mikedeboer]]

Comment on attachment 8775940 [details]
Bug 1280978 - don't show the modal highlighting ui on very large pages, because it degrades performance too much.

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

Comment 32

[Boris Zbarsky [:bzbarsky]]

That last patch uses an arbitrary 500k pixel limit, right?  

And even for pages near that limit, this looks pretty slow....  Not to mention the UX confusion of having your search UI look different on some pages.  :(

Comment 33

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

Comment on attachment 8775940 [details]
Bug 1280978 - don't show the modal highlighting ui on very large pages, because it degrades performance too much.

https://reviewboard.mozilla.org/r/67952/#review103940

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #32)
> That last patch uses an arbitrary 500k pixel limit, right?  
> 
> And even for pages near that limit, this looks pretty slow....  Not to
> mention the UX confusion of having your search UI look different on some
> pages.  :(

Yes, I am in agreement. As I mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1324143#c5 and https://bugzilla.mozilla.org/show_bug.cgi?id=1324143#c6 this route is unmaintainable and we need to face the real issue that the approach for this implementation is not scaling well. We will most likely need to drop the dimming part and focus on how we can make our highlights look better.

Comment 34

[Mike de Boer [:mikedeboer]]

Well, that limit is indeed arbitrary. I'm not sure what else I can do here - even though I don't agree that the route of where the patch is going is unmaintainable.
Why? Because the other route Jared mentions is to request help from platform developers that will allow us to style ranges better which I briefly exchanged messages with :heycam about a long time ago. Take-away of that was that it'd be doable to programmatically specify inline styles like font(-style, -size, -color, etc) and color of the selection, other things like border(-width, -color, -radius, etc), outline(-width, -color, -style) we might want are much more complex to implement.

Biggest contributor that's making Firefox a slowpoke here whilst modal highlighting is visible is LAYOUT. This may be due to various complex things, but I do not understand nor am I able to analyze them. There appears to be a part of reflow that is O(n) to O(n^2) to the dimensions of the page. That's what (the updated) Gecko Profiler is telling me.
But the question is whether putting resources on solving this issue vs. working on Quantum is viable today.

Meanwhile I'll continue spending time on issues I _can_ fix that block this feature from riding the trains. The patch I put up here is what I'm able to do and capable of at this time.

Comment 35

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

Comment on attachment 8775940 [details]
Bug 1280978 - don't show the modal highlighting ui on very large pages, because it degrades performance too much.

https://reviewboard.mozilla.org/r/67952/#review104196

Okay, I don't want to stand in the way of making what we have better here. But maybe there is a conversation that we can have between layout devs and UX as to what is possible here.

::: toolkit/modules/FinderHighlighter.jsm:212
(Diff revision 2)
> +   * scale, we have to turn the feature off based on the `kPageIsTooBigPx`
> +   * threshold as hardcoded above.
> +   *
> +   * @param  {Object}       dict     Dictionary of properties belonging to the
> +   *                                 currently active window
> +   * @param  {nsIDOMWindow} [window] The dimmed background is overlaying this window.

I think the description of this parameter is confusing. It is written as though it is a boolean.

Comment 36

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.