[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...

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?

Tracking this performance regression for 50.

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.

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.

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.

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?

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?

> 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.

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/nfroyd@mozilla.com-f27be3c0c1e7d07f5cb47c48f7a2f718d728c4d4/try-macosx64/try-macosx64-bm87-try1-build18724.txt.gz and search for "libffi", then ctrl-g a couple of times.

(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?

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

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.

(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.

`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.

(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.

[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.

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

All dependencies resolved, marking this one fixed.

Mark 51 as fixed as all dependencies are resolved.

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.

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

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

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

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.

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.

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.

(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.

Thanks Mike. That sounds good to me!

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 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.

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 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.