Closed Bug 1355346 Opened 7 years ago Closed 6 years ago

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

Categories

(Toolkit :: Downloads API, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Performance Impact high
Tracking Status
firefox63 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [fxperf:p1])

Attachments

(2 files)

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: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
See Also: → 1587504
Performance Impact: --- → P1
Whiteboard: [qf:p1:f64][fxperf:p1] → [fxperf:p1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: