1587504 - Make DownloadPlatform use the dedicated low-priority background thread for macOS metafile work rather than a LazyIdleThread

Status: RESOLVED FIXED

(Toolkit :: Downloads API, task, P3)

Description

[Mike Conley (:mconley) (:⚙️)]

In bug 1355346, I landed a patch that added a new LazyIdleThread (a thread that spins up on demand, and then goes away after a timeout) to DownloadPlatform.cpp, to do some work after downloads complete on macOS.

With bug 1450059 fixed, I suspect we can tear out that one-off thread, and use the dedicated low-priority background thread instead - presuming that it's alright that we do IO on that thread.

Assigning to plawless for now, since she was interested in taking on something like this.

Hey KrisWright, do you know if it's okay if we put disk IO on the low-priority background thread?

Comment 1

[Kris Wright :KrisWright]

(In reply to Mike Conley (:mconley) (:⚙️) (Wayyyy behind on needinfos) from comment #0)

In bug 1355346, I landed a patch that added a new LazyIdleThread (a thread that spins up on demand, and then goes away after a timeout) to DownloadPlatform.cpp, to do some work after downloads complete on macOS.

With bug 1450059 fixed, I suspect we can tear out that one-off thread, and use the dedicated low-priority background thread instead - presuming that it's alright that we do IO on that thread.

Assigning to plawless for now, since she was interested in taking on something like this.

Hey KrisWright, do you know if it's okay if we put disk IO on the low-priority background thread?

This is actually a thread I've looked at in the past as a candidate to throw onto some lazy/shared/background thread instance before we had the background thread. Since there's not many places that the background thread is being used right now and the job in bug 1355346 looks fairly self contained, it looks like it should be fine. Note that the background thread has a fairly small stack size, but as I recall LazyIdleThreads do as well so it probably won't affect anything. Maybe in the future we'll have some kind of API for a dedicated background IO thread, in which case this is an excellent candidate for one. For now I don't see a problem with using the background thread, unless any unexpected issues come up with it.

Comment 2

[Kris Wright :KrisWright]

I wanted to note for this bug that there's now a flag for dispatching blocking I/O, NS_DISPATCH_EVENT_MAY_BLOCK[1], so that jobs like this can go to a dedicated I/O pool.

[1] https://searchfox.org/mozilla-central/rev/be7d1f2d52dd9474ca2df145190a817614c924e4/xpcom/threads/nsIEventTarget.idl#56-62

Comment 3

[Emma Malysz]

Attachment: Bug 1587504, remove the usage of the LazyIdleThread from DownloadPlatform and instead use the low-priority background thread

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/294aa763e2a4
remove the usage of the LazyIdleThread from DownloadPlatform and instead use the low-priority background thread r=mconley,KrisWright

Comment 5

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/294aa763e2a4