Closed
Bug 1105039
Opened 10 years ago
Closed 10 years ago
Gecko profiler uses intr messages from chrome to content process
Categories
(Core :: Gecko Profiler, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
Tracking | Status | |
---|---|---|
e10s | + | --- |
People
(Reporter: billm, Assigned: billm)
References
Details
Attachments
(1 file)
(deleted),
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PContent.ipdl#518
Mike, this is something we really want to avoid. Is there any reason we need to use intr here?
Flags: needinfo?(mconley)
Comment 1•10 years ago
|
||
I was following the pattern that I saw pre-existing in PPluginModule.ipdl[1].
I believe the Gecko Profiler, when gathering profiles, assumes it will receive those profiles synchronously. BenWa can confirm or refute my read on that. If I'm wrong, we could certainly switch this to an async call.
Otherwise, is there something synchronous other than intr that we should use here?
[1]: http://hg.mozilla.org/mozilla-central/file/8c37c5083952/dom/plugins/ipc/PPluginModule.ipdl#l84
Flags: needinfo?(mconley) → needinfo?(bgirard)
Comment 2•10 years ago
|
||
This used to be RPC but got changed here:
http://hg.mozilla.org/mozilla-central/diff/2466893f18a7/dom/plugins/ipc/PPluginModule.ipdl
TBH I don't know why I made it RPC. I think it should of just been Sync all along. The reason this is async is because nsProfiler must return the profile synchronously.
I vote to change both to 'sync'.
Flags: needinfo?(bgirard)
Assignee | ||
Comment 3•10 years ago
|
||
We simply can't make it sync. Sync messages from the chrome process are forbidden.
Benoit, can you lay out a plan to change nsProfiler so this doesn't have to be synchronous?
Flags: needinfo?(bgirard)
Updated•10 years ago
|
tracking-e10s:
--- → ?
Comment 4•10 years ago
|
||
Shouldn't be too hard it looks like. I think we just have to change the addon (simple) and change devtools.
Victor how big is the change for devtools to get the profile asynchronously? Looks like the profiler actor is already async anyways so it should be trivial right?
Flags: needinfo?(bgirard) → needinfo?(vporof)
Comment 5•10 years ago
|
||
The profiler frontend already expects everything to be async. The backend (actor) expects the profiler module to start synchronously at the moment [0]. Changing this is quite possibly trivial, and IIRC it involves making `onStartProfiler` return a promise (although I can't exactly remember if old-style actors work like this or some other similar way).
[0] http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/profiler.js?from=actors/profiler.js#58
Flags: needinfo?(vporof)
Updated•10 years ago
|
Assignee | ||
Comment 6•10 years ago
|
||
I want to get this out of the way so we can finally be rid of intr messages. The easiest way is to switch to a high priority message, like CPOWs use. It's still sync and it avoids many of the problems with intr messages.
Attachment #8535945 -
Flags: review?(mconley)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → wmccloskey
Comment 7•10 years ago
|
||
Comment on attachment 8535945 [details] [diff] [review]
profiler-cpow
Review of attachment 8535945 [details] [diff] [review]:
-----------------------------------------------------------------
If you're changing this one, can you change the GetProfile call in PPluginModule.ipdl too?
Attachment #8535945 -
Flags: review?(mconley)
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8535945 [details] [diff] [review]
profiler-cpow
The basic problem we have right now is that intr messages and CPOW messages (i.e., prioritized sync messages) don't work together. If there's a protocol that uses both, it's probably broken.
Since PContent has to use CPOWs, we want to get rid of all the intr messages. Since PPluginModule has to use intr messages, we don't want to introduce any CPOW messages.
Attachment #8535945 -
Flags: review?(mconley)
Comment 9•10 years ago
|
||
Comment on attachment 8535945 [details] [diff] [review]
profiler-cpow
Review of attachment 8535945 [details] [diff] [review]:
-----------------------------------------------------------------
Ah OK - I was not aware of that restriction. Thanks for clearing that up.
Attachment #8535945 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 10•10 years ago
|
||
Comment 11•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in
before you can comment on or make changes to this bug.
Description
•