CocoaFileUtils::AddOriginMetadataToFile() called at the end of file downloads can do sync IPC on the main thread

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: Ehsan, Assigned: mconley)

Tracking

(Blocks 1 bug, {perf})

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [qf:p1:f64][fxperf:p1])

Attachments

(2 attachments)

STR:

Download a file.

Profile: https://perfht.ml/2o2izEA

The sendServerMessageWithTimeout() call seems to be sending a message to a different process and blocking on it.  In this profile it's pausing for 46ms.  We should do this work off the main thread, if possible.

This is coming from: <http://searchfox.org/mozilla-central/rev/624d25b2980cf5f83452b040e6d664460f9b7ec5/toolkit/components/jsdownloads/src/DownloadPlatform.cpp#176>
Priority: -- → P2
Whiteboard: [qf:p1] → [qf:p2]
Keywords: perf
Whiteboard: [qf:p2] → [qf:p1]
Whiteboard: [qf:p1] → [qf:i60][qf:p1]
Whiteboard: [qf:i60][qf:p1] → [qf:f60][qf:p1]
Whiteboard: [qf:f60][qf:p1] → [qf:f61][qf:p1]
Whiteboard: [qf:f61][qf:p1] → [qf:f64][qf:p1]
Whiteboard: [qf:f64][qf:p1] → [qf:p1:f64]
Whiteboard: [qf:p1:f64] → [qf:p1:f64][fxperf]
Whiteboard: [qf:p1:f64][fxperf] → [qf:p1:f64][fxperf:p2]
Comment on attachment 9002139 [details]
Bug 1355346 - Call nsCocoaFileUtils::AddOriginMetadataToFile off of the main-thread when completing downloads.

Hey spohl,

Here's a strawman that tries to get this stuff off of the main thread.

MAJOR CAVEAT: I'm pretty unfamiliar with these structures that I'm suddenly manipulating off of the main thread. I do not know, for example, if CFStrings are threadsafe (I pass pathCFStr into the lambda). I do not know if AddOriginMetadataToFile or AddQuarantineMetadataToFile are safe to be called off of the main thread.

But on the off-chance that they are, this _might_ suffice. Just be advised that I'm not sure, and I guess that's what I'm asking here with my feedback?.
Attachment #9002139 - Flags: feedback?(spohl.mozilla.bugs)
Comment on attachment 9002139 [details]
Bug 1355346 - Call nsCocoaFileUtils::AddOriginMetadataToFile off of the main-thread when completing downloads.

This should also notify completion, because we only want to consider the download finished after the file is closed and ready to be used. For example, this may be launched automatically after completion.
(In reply to Mike Conley (:mconley) (:⚙️) from comment #2)
> Comment on attachment 9002139 [details]
> Bug 1355346 - [WIP] Call nsCocoaFileUtils::AddOriginMetadataToFile off of
> the main-thread when completing downloads.
>
> MAJOR CAVEAT: I'm pretty unfamiliar with these structures that I'm suddenly
> manipulating off of the main thread. I do not know, for example, if
> CFStrings are threadsafe (I pass pathCFStr into the lambda).

This should be fine. The CFString is an immutable object and it is only accessed by the IO thread after creation.

> I do not know
> if AddOriginMetadataToFile or AddQuarantineMetadataToFile are safe to be
> called off of the main thread.

It appears that all CF objects in question here are only accessed from one thread, so this should be fine as well.

Some background on thread safety, particularly involving Core Foundation (CF) objects:
https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Multithreading/ThreadSafetySummary/ThreadSafetySummary.html#//apple_ref/doc/uid/10000057i-CH12-SW9
Attachment #9002139 - Flags: feedback?(spohl.mozilla.bugs) → feedback+
Assignee: nobody → mconley
Whiteboard: [qf:p1:f64][fxperf:p2] → [qf:p1:f64][fxperf:p1]
Attachment #9002139 - Attachment description: Bug 1355346 - [WIP] Call nsCocoaFileUtils::AddOriginMetadataToFile off of the main-thread when completing downloads. → Bug 1355346 - Call nsCocoaFileUtils::AddOriginMetadataToFile off of the main-thread when completing downloads.
Comment on attachment 9002595 [details]
Bug 1355346 - Make mozIDownloadPlatform::downloadDone return a Promise. r=paolo,spohl

:Paolo Amadini has approved the revision.
Attachment #9002595 - Flags: review+
Comment on attachment 9002139 [details]
Bug 1355346 - Call nsCocoaFileUtils::AddOriginMetadataToFile off of the main-thread when completing downloads.

Markus Stange [:mstange] has approved the revision.
Attachment #9002139 - Flags: review+
Comment on attachment 9002595 [details]
Bug 1355346 - Make mozIDownloadPlatform::downloadDone return a Promise. r=paolo,spohl

Markus Stange [:mstange] has approved the revision.
Attachment #9002595 - Flags: review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/566a5b21073a
Call nsCocoaFileUtils::AddOriginMetadataToFile off of the main-thread when completing downloads. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/acfaacaca4cb
Make mozIDownloadPlatform::downloadDone return a Promise. r=paolo,mstange
https://hg.mozilla.org/mozilla-central/rev/566a5b21073a
https://hg.mozilla.org/mozilla-central/rev/acfaacaca4cb
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.