Closed Bug 854464 Opened 11 years ago Closed 6 years ago

Incredible lag doing a download with lots of windows open

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bzbarsky, Assigned: florian)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxperf:p3])

Attachments

(1 file, 1 obsolete file)

BUILD: March 24 nightly

STEPS TO REPRODUCE:

1)  Start build with clean profile.
2)  Hit Cmd-N 35 times.
3)  Wait for all of those to finish loading.
4)  Load http://ftp.mozilla.org/pub/mozilla.org/seamonkey/nightly/latest-comm-central-trunk/
5)  Click the "seamonkey-2.16a1.en-US.mac.dmg" link.
6)  In the resulting dialog, select "Open with DiskImageMounter" and click "OK"

EXPECTED RESULTS: Download happens normally, the little download manager button in the toolbar updates as it goes.

ACTUAL RESULTS: UI pretty much freezes up for the duration of the download, browser becomes nearly unresponsive, fans spin up, Activity Monitor shows near-100% CPU usage from Minefield and a bunch of kernel CPU time, sampling in activity monitor blames CGLFlushDrawable called from mozilla::gl::GLContextCGL::SwapBuffers via the normal painting codepath.

NOTES:

1) If I don't open lots of windows, the problem does not seem to manifest?

2) This has got to be a regression, since I've downloaded things before without it being failtastic.  But I just tried a Jan 1 build and got the same result there...
Heh, on Win7 x64 with HWA enabled, just opening 35-40 windows killed my PC. The GPU was at 99% usage, the Video RAM was 90% usage and all the UI operations of the OS  were soooper slow.
It's not exactly a hypothetical situation; I have 24 windows open right now, and I closed some since I filed this bug...
I wonder if we can do something about this in the frontend, like maybe avoiding the download start animations if we don't already, or something.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #3)
> I wonder if we can do something about this in the frontend, like maybe
> avoiding the download start animations if we don't already, or something.

OK, after looking at this for a bit, I'm not sure there's much the frontend can do to help here, so going to clear my needinfo. Hopefully the gfx team can pick this up...

I'll note that the steps from comment #0 perform leagues better on my windows machine than on my mbp, which surprised me. Perhaps there's something in gfx/layers that performs worse on mac than on Windows... The result on the mbp isn't quite as bad as in comment #0 but it's pretty terrible.
Flags: needinfo?(gijskruitbosch+bugs)
Do you happen to have a profile of it?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Florian Quèze [:florian] from comment #5)
> Do you happen to have a profile of it?

I did, but they weren't 'shared' so I don't anymore... all the associated JS stuff for the download was <50ms, which was considerably less than the amount of hanging I saw. In the profile, the main process was basically waiting almost all the time (mach_msg_trap or whatever that's called on mac). I expect the time might now be spent in the gpu/compositor process(es) but I didn't look into that in detail because I'm not able to really do much with that information... I didn't spot anything obvious that the JS code was doing that would cause terrible things to happen to the gpu side of things.

If it's particularly helpful I can try to rerun the same situation to produce another profile - the steps are pretty trivial. But I suspect that the profile is helpful if/when someone from platform looks at things. This has been in Core::Gfx for 5 years with no action, so I imagine it's not high priority...
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(florian)
I should note that part of the problem is that we seem to be repainting the entire browser window on every update to the progress indicator, at least as far as I could tell.
Whiteboard: [fxperf]
Priority: -- → P3
Whiteboard: [fxperf] → [fxperf:p3]
Priority: P3 → --
I was curious and captured my own profile: https://perfht.ml/2EIwvLi
Flags: needinfo?(florian)
Markus, any idea of why it often takes more than 100ms to composite in the profile of comment 8?
Flags: needinfo?(mstange)
That's very puzzling. I think each composite marker shows the compositing of a single window. If we have 30 windows that all animate at 60fps, we should get 30 markers for a single "frame". I would understand if each of those single-window composites took 16ms, and we'd need 500ms to composite a single frame in all of them. But it looks like it's taking 100ms to composite just *one* window! I don't understand why that would be.

That said, we do have plans to improve partial window composition.

We should also stop compositing occluded windows. I don't know if we have a bug for that.
Flags: needinfo?(mstange)
Attached patch Patch (obsolete) — Splinter Review
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #7)
> I should note that part of the problem is that we seem to be repainting the
> entire browser window on every update to the progress indicator, at least as
> far as I could tell.

This is not exactly what we do, but encouraged me to enable paint flashing, which made the problem clear: for each progress notification, we repaint the progress indicator in every window, even though the indicator is tiny and most repaints result in repainting the same thing over and over again. Inspecting the indicator with the browser toolbox, I saw that each time we do update it, by setting the animationDelay style property with several digits after the decimal point. This is done by the code at https://searchfox.org/mozilla-central/rev/9f3da81290054c5b8955bb67ff98cae66676f745/browser/components/downloads/content/indicator.js#494

Rounding the percentComplete value before we set it ensures we update the indicator at most 100 times (which is more than enough for a progress indicator of 12 pixels). We could reduce the update count even further, but I don't know if we do subpixel anti-aliasing here, so I prefered playing it safe.

Here is a profile with this patch applied, to compare with the profile from comment 8: https://perfht.ml/2EJkaqk
Attachment #8967786 - Flags: review?(paolo.mozmail)
Assignee: nobody → florian
Status: NEW → ASSIGNED
Emilio, is there anything that can be optimized in https://perfht.ml/2EGmq1H ? Isn't there a CSS cache that could be used here to avoid doing the exact same restyle in the 30+ browser windows?
Flags: needinfo?(emilio)
(In reply to Markus Stange [:mstange] from comment #10)

> We should also stop compositing occluded windows. I don't know if we have a
> bug for that.

This would help A LOT in this case!
(In reply to Florian Quèze [:florian] from comment #12)
> Emilio, is there anything that can be optimized in https://perfht.ml/2EGmq1H
> ? Isn't there a CSS cache that could be used here to avoid doing the exact
> same restyle in the 30+ browser windows?

We'd need to share that state across windows, which looks kind of like a pain and I don't think we can do. Even if we didn't need to do that, optimizing for the same dynamic change happening in different parts of the same document doens't look like a trivial problem either.

That being said, that code looks very poorly optimized (::deref() functions not inlined and such).

Is that a local opt build? If so, it has -O1 for rust code which is... more than suboptimal for profiling.
Flags: needinfo?(emilio)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #14)

> Is that a local opt build? If so, it has -O1 for rust code which is... more
> than suboptimal for profiling.

Yes, it's my local mac build with today's inbound.
Comment on attachment 8967786 [details] [diff] [review]
Patch

Thanks for finding this issue!

It's probably better to fix this upstream, it is likely done here:

https://dxr.mozilla.org/mozilla-central/rev/6547c27303bc4d8961b11e656751e839807d65c7/browser/components/downloads/DownloadsCommon.jsm#350-351

Also, better to use "Math.floor" so slow downloads would still be reported as 99% until all bytes have been transferred. Not that it makes such a difference visually at the moment, it's just more correct.
Attachment #8967786 - Flags: review?(paolo.mozmail)
Attached patch Patch v2Splinter Review
Patch updated as suggested in comment 16. I wonder if this is going to affect the smoothness of the progress bar in the downloads panel and the downloads window, where 100 steps is not enough. But even without the patch, these progressbars already don't move smoothly, somehow their updates must already be throttled somewhere else.
Attachment #8968144 - Flags: review?(paolo.mozmail)
Attachment #8967786 - Attachment is obsolete: true
Comment on attachment 8968144 [details] [diff] [review]
Patch v2

Thanks!
Attachment #8968144 - Flags: review?(paolo.mozmail) → review+
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/efac2c840d9b
round the percentComplete value of the download indicator before setting it to set it at most 100 times, r=paolo.
https://hg.mozilla.org/mozilla-central/rev/efac2c840d9b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.