Closed Bug 1658202 Opened 4 years ago Closed 4 years ago

Massive slowdowns when downloading to samba share

Categories

(Core :: DOM: Core & HTML, defect)

79 Branch
defect

Tracking

()

RESOLVED FIXED
82 Branch
Performance Impact medium
Tracking Status
firefox82 --- fixed

People

(Reporter: lightlike, Assigned: Gijs)

References

(Depends on 1 open bug)

Details

(Keywords: main-thread-io, perf:responsiveness)

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:79.0) Gecko/20100101 Firefox/79.0

Steps to reproduce:

Download file to smb-share
(my case Win10 to Debian/OpenMediaVault)

Actual results:

Firefox became very unresponsive. (about 10 seconds to switch tabs)
Websides still worked fine (except for bandwidth problems. But that is on me)

Expected results:

Firefox should continue working fine while the download loads in the background.

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Component: Untriaged → File Handling

I don't have access to a samba share. Can you create a performance profile using https://profiler.firefox.com/ and share it with us? That'll help pinpoint the problem.

Flags: needinfo?(lightlike)

Here you go: https://share.firefox.dev/3aJCd17

I did a profile using the Front-End Settings.

Slowdowns only started after a short while when I started the second download.

Flags: needinfo?(lightlike)

Thanks for the super quick turnaround, that already helps. Can you elaborate on what steps you're taking to "download file"? Do you use ctrl-s or similar to save a file that's displayed in the browser? Or click a link and "save link as..."? Or something else?

(edit: oops, submitted too early, ctd...)

Flags: needinfo?(lightlike)
Keywords: main-thread-io

I am using ctrl+s or right-mouse -> save on a direct video page.

Flags: needinfo?(lightlike)

The root cause here seems to be that https://searchfox.org/mozilla-central/rev/6cc48251bb97600fdf11a5b4c5f621bfc8606d55/dom/webbrowserpersist/nsWebBrowserPersist.cpp#915 does main thread IO, and in 8kb chunks to boot, using NSPR APIs... so if you save a big file that's going to hurt. This code is ancient (2002) and hasn't been very well-optimized... :-(

This is worse on network drives like samba, of course, but it's going to be bad everywhere tbh, and we shouldn't be doing it. By now the codebase has primitives to make this a lot easier.

Nathan, do you have suggestions on what the most modern way to do this is? I'm guessing we should be using a background IO queue task thingy, or do it on the socket thread pool or something (but we'll need to make sure tasks get executed in order...).

(Moving to Core :: DOM & HTML because that's where the webbrowserpersist code lives)

Component: File Handling → DOM: Core & HTML
Flags: needinfo?(nfroyd)
Product: Firefox → Core
Whiteboard: [qf]

I think this gets into questions about how our networking stack works, which I am relatively ignorant about. We'd need to receive nsIRequestListener callbacks off the main thread, and we'd need to make sure whatever code is calling that is async...but looking at the stacks in the profile, it looks like this is getting called from UI, so it...needs to be sync? Maybe we could do a better job of continuing to process events while the I/O is being done by doing the I/O somewhere else.

Sending ni? to Valentin to inquire about receiving OnDataAvailable off the main thread somehow.

Flags: needinfo?(nfroyd) → needinfo?(valentin.gosu)

(In reply to lightlike from comment #3)

Here you go: https://share.firefox.dev/3aJCd17

In addition to the issue discussed in comment 6, this profile also shows us spending 4s checking if a file exists. This is caued by this code, and that part is already covered by bug 827010.

(In reply to Nathan Froyd [:froydnj] from comment #7)

I think this gets into questions about how our networking stack works, which I am relatively ignorant about. We'd need to receive nsIRequestListener callbacks off the main thread, and we'd need to make sure whatever code is calling that is async...but looking at the stacks in the profile, it looks like this is getting called from UI, so it...needs to be sync? Maybe we could do a better job of continuing to process events while the I/O is being done by doing the I/O somewhere else.

Sending ni? to Valentin to inquire about receiving OnDataAvailable off the main thread somehow.

Yes, that's possible using nsIThreadRetargetableRequest

I think the JS issues are separate, and the bulk of the time is spent writing things to disk in nsWebBrowserPersist::OnDataAvailable
Making nsWebBrowserPersist implement nsIThreadRetargetableRequest and retarget data delivery to a background thread should fix the bulk of the delays.

Flags: needinfo?(valentin.gosu)
Assignee: nobody → gijskruitbosch+bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Depends on: 1661941
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bc244bc15836
move writes from onDataAvailable away from the main thread, r=valentin
https://hg.mozilla.org/integration/autoland/rev/577001cf12ad
move flushes caused by closing the stream away from the main thread, r=valentin

Hrmpf, mach try auto failed me. I'll have to take a look tomorrow.

Whiteboard: [qf] → [qf:p2:responsiveness]
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1b44712c42dc
move writes from onDataAvailable away from the main thread, r=valentin
https://hg.mozilla.org/integration/autoland/rev/a8b3c33aee21
move flushes caused by closing the stream away from the main thread, r=valentin
Flags: needinfo?(gijskruitbosch+bugs)
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch
Regressions: 1663499
Performance Impact: --- → P2
Whiteboard: [qf:p2:responsiveness]
You need to log in before you can comment on or make changes to this bug.