Closed Bug 1587504 Opened 6 months ago Closed 1 month ago

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

Categories

(Toolkit :: Downloads API, task, P3)

Desktop
macOS
task

Tracking

()

RESOLVED FIXED
mozilla75
Tracking Status
firefox75 --- fixed

People

(Reporter: mconley, Assigned: emalysz)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [fxperf:p3])

Attachments

(1 file)

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?

Flags: needinfo?(kwright)

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

Flags: needinfo?(kwright)
Priority: -- → P3
Whiteboard: [fxperf]
Whiteboard: [fxperf] → [fxperf:p3]

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

Blocks: 1606879
Assignee: plawless → emalysz
Pushed by mconley@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
You need to log in before you can comment on or make changes to this bug.