The mainthreadio feature doesn't work with startup profiling.

RESOLVED FIXED in Firefox 66

Status

()

enhancement
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: florian, Assigned: florian)

Tracking

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

4 months ago

When starting Firefox with these environment variables:
MOZ_PROFILER_STARTUP_FEATURES="mainthreadio"
MOZ_PROFILER_STARTUP=1
the resulting profile doesn't contain any IO marker.

If after the end of startup I restart the profiler (with the "Main Thread IO" checkbox checked in the add-on), then the next profile I capture contains IO markers.

Debugging a bit, I saw that we return early at https://searchfox.org/mozilla-central/rev/39265dd58298c40bed029617ad408bf806cce761/xpcom/build/IOInterposer.cpp#475

Calling IOInterposer::Init() in the profiler code before calling IOInterposer::Register fixes it.

Assignee

Comment 1

4 months ago
This worked for me locally. I don't know if it's correct, especially as I don't understand why the profiler's mainthreadio feature only cares about IO happening on the main thread, but apparently the code can be triggered off main-thread.

Try builds at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea350fceccc2cc4169fa7ad6316b37877972f25b
Assignee

Comment 2

4 months ago
Comment on attachment 9038849 [details] [diff] [review]
Initialize the IOInterposer before registering the profiler's IO interposer observer

I used this successfully locally on Mac, and on Windows using the try build from comment 1, so I'm requesting review. Hopefully Markus knows what to do when this is called off main thread (and/or why that would ever happen).
Attachment #9038849 - Flags: review?(mstange)
Comment on attachment 9038849 [details] [diff] [review]
Initialize the IOInterposer before registering the profiler's IO interposer observer

Review of attachment 9038849 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah the non-main-thread thing seems like a problem: In content processes, this function can be called off the main thread. However, in that case, that call to IOInterposer::Init() is probably not the *first* call to IOInterposer::Init()... but we shouldn't rely on that. I think you need to follow what we do for IOInterposer::Register just below: call directly if on main thread, otherwise go through a runnable.

Oh, and IOInterposer::Init() contains the code "bool isMainThread = true; RegisterCurrentThread(isMainThread);", so it definitely expects to only be called on the main thread.
Attachment #9038849 - Flags: review?(mstange) → review-
Assignee

Comment 4

4 months ago
Posted patch Patch v2Splinter Review
Attachment #9038911 - Flags: review?(mstange)
Assignee

Updated

4 months ago
Attachment #9038849 - Attachment is obsolete: true
Assignee

Updated

4 months ago
Assignee: nobody → florian
Status: NEW → ASSIGNED
Comment on attachment 9038911 [details] [diff] [review]
Patch v2

Review of attachment 9038911 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me! Thanks!
Attachment #9038911 - Flags: review?(mstange) → review+

Comment 6

4 months ago
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/249e88d11b9e
Initialize the IOInterposer before registering the profiler's IO interposer observer, r=mstange.

Comment 7

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Depends on: 1522873
You need to log in before you can comment on or make changes to this bug.