Closed
Bug 1297446
Opened 8 years ago
Closed 8 years ago
simple addons trigger large amounts of sync IPC in e10s
Categories
(Core :: DOM: Content Processes, defect)
Core
DOM: Content Processes
Tracking
()
RESOLVED
FIXED
mozilla51
People
(Reporter: bkelly, Assigned: gkrizsanits)
Details
Attachments
(2 files)
315.37 KB,
image/png
|
Details | |
2.18 KB,
patch
|
billm
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Please see sample range [122250, 122599] on the content process in this profile: https://cleopatra.io/#report=03ca7a5d631e37ea3d638d62f6c06a0b32a52727 There is a ton if sync IPC happening. This is coming from RemoteAddonChild.jsm here: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/addoncompat/RemoteAddonsChild.jsm#417 Which is being triggered from "document-element-inserted" being observed in the addon: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/addoncompat/RemoteAddonsParent.jsm#384 The addon in question here is the GeckoProfiler itself. AFAICT its triggering this simply by doing: request('sdk/panel') The panel module pulls in panel/events: https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/panel.js#30 Which then observes "document-element-inserted": https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/panel/events.js#25 Basically anything using sdk/panel is going to trigger this sync IPC. Blake, what do you think?
Flags: needinfo?(mrbkap)
This is basically bug 1167802.
Reporter | ||
Comment 2•8 years ago
|
||
Ok. I think its a problem if our main profiler addon interferes with perf to this degree, though.
Yes, I agree. We can mark the profiler as multiprocessCompatible. That should eliminate the shims. Benoit, are you still the maintainer? You just need to add a key to install.rdf and make sure everything still works.
Flags: needinfo?(bgirard)
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #1) > This is basically bug 1167802. This is odd. This should have been fixed by bug 1196975. system/events.js is already using shim waivers.
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Ben Kelly [:bkelly] from comment #0) > Please see sample range [122250, 122599] on the content process in this > profile: > > https://cleopatra.io/#report=03ca7a5d631e37ea3d638d62f6c06a0b32a52727 For some reason I only see necko/predictor activity on that section... > > There is a ton if sync IPC happening. > > This is coming from RemoteAddonChild.jsm here: > > https://dxr.mozilla.org/mozilla-central/source/toolkit/components/ > addoncompat/RemoteAddonsChild.jsm#417 > > Which is being triggered from "document-element-inserted" being observed in > the addon: > > https://dxr.mozilla.org/mozilla-central/source/toolkit/components/ > addoncompat/RemoteAddonsParent.jsm#384 > > The addon in question here is the GeckoProfiler itself. AFAICT its > triggering this simply by doing: > > request('sdk/panel') > > The panel module pulls in panel/events: > > https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/ > panel.js#30 > > Which then observes "document-element-inserted": > > https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/ > panel/events.js#25 > > Basically anything using sdk/panel is going to trigger this sync IPC. > > Blake, what do you think? Are you sure about this diagnostic? Panel's should use system/events.js which is working hard to avoid the shim layer. There is a really handy flag I tend to use to hunt down these kind of issues: dom.ipc.shims.enabledWarnings (it should add some warnings to the console when the shim layer is hit) I only see the shim being used from here: http://searchfox.org/mozilla-central/source/addon-sdk/source/lib/sdk/content/l10n-html.js#118 (which should be investigated...) Is there something I'm missing here?
Flags: needinfo?(bkelly)
Reporter | ||
Comment 6•8 years ago
|
||
Pretty sure. This is what I see in cleopatra. There is network predictor stuff causing problems, but thats in the parent process and its later in the trace. I filed bug 1295565 for that.
Flags: needinfo?(bkelly)
Comment 7•8 years ago
|
||
Does JPM not support it? Do I have to repack the addon and add it manually?
Flags: needinfo?(bgirard) → needinfo?(wmccloskey)
Comment 8•8 years ago
|
||
Maybe we just need to repack the addon with a more recent version of the addon-sdk.
Comment 9•8 years ago
|
||
I updated the addon to 1.16.18 which should include the most recent addon-sdk.
I think it's the multiprocess permission: https://developer.mozilla.org/en-US/Add-ons/SDK/Tools/package_json#permissions Might be good to check the install.rdf file that gets generated though.
Flags: needinfo?(wmccloskey)
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #9) > I updated the addon to 1.16.18 which should include the most recent > addon-sdk. Yeah the old version seem to include the old SDK in the xpi (which did not use shim waiving), so that might have been the reason for the problem. But it's a bit odd that I don't see the warnings on the console only for l10n-html so maybe the included SDK is ignored entirely. Do you have any reason not to strip the SDK from your xpi btw? (I thought we do that automatically so I don't quite get why is it included at all) (In reply to Ben Kelly [:bkelly] from comment #6) > Pretty sure. This is what I see in cleopatra. Thanks I see it too now. I get that we hit the RemoteAddonsChild part, I was just wondering if it's the result of the panel code. Can you explain to me where do you see it from that this is the result of the panel code and not the l10n code? (given that the old SDK is included into the xpi I can imagine that it is the case I just want to be able to reproduce it so I can fix it)
Reporter | ||
Comment 12•8 years ago
|
||
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #11) > Thanks I see it too now. I get that we hit the RemoteAddonsChild part, I was > just wondering if it's the result of the panel code. Can you explain to me > where do you see it from that this is the result of the panel code and not > the l10n code? (given that the old SDK is included into the xpi I can > imagine that it is the case I just want to be able to reproduce it so I can > fix it) See: http://logs.glob.uno/?c=mozilla%23gfx&s=15+Aug+2016&e=15+Aug+2016&h=GeckoProfiler#c278617 We don't know for sure its sdk/panel, but it is coming from something in GeckoProfiler. GeckoProfiler does not directly observe document-element-inserted itself AFAICT. The sdk/panel seemed the most likely after looking at the modules in use. But again, not 100% certain.
Assignee | ||
Comment 13•8 years ago
|
||
(In reply to Ben Kelly [:bkelly] from comment #12) > We don't know for sure its sdk/panel, but it is coming from something in > GeckoProfiler. GeckoProfiler does not directly observe > document-element-inserted itself AFAICT. The sdk/panel seemed the most > likely after looking at the modules in use. But again, not 100% certain. If it's from the panel then repacking should fix it but I don't think so, I believe that the SDK that is shipped with Firefox is being used and the one in the xpi is ignored. Which means it should waive the shim layer entirely for panels. If it's from l10n (what I've seen triggering the shim layer for the profiler and does attach a listener to document-element-inserted) this patch should fix it. This is affecting a lot of SDK add-ons if not all of them, so we should land this either way. Let's see if it fixes the profiler or not.
Attachment #8785230 -
Flags: review?(wmccloskey)
Comment on attachment 8785230 [details] [diff] [review] Shimwaiver for l10n. v1 Review of attachment 8785230 [details] [diff] [review]: ----------------------------------------------------------------- OK. It looks like Jetpack's localization is supposed to work for content scripts, so this should be fine. Thanks Gabor. Hopefully this fixes it.
Attachment #8785230 -
Flags: review?(wmccloskey) → review+
Reporter | ||
Comment 15•8 years ago
|
||
Is this something we can or should uplift to other branches?
Flags: needinfo?(mrbkap)
Assignee | ||
Comment 16•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=21aac83ffd23
I don't see why not. It's a very straightforward patch. Would you mind testing to see if it works, Ben? It looks like Gabor has some try builds there.
Flags: needinfo?(bkelly)
Assignee | ||
Comment 18•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a32c1e67f222930df2fc7f0b1806c365af3187b Bug 1297446 - Shimwaiver for l10n. r=billm
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8785230 [details] [diff] [review] Shimwaiver for l10n. v1 I think we want this for aurora but I'm afraid we're too late for beta... but let's try. For aurora it is I think more important because there might be some add-on testing on the beta channel in the next cycle for add-on white listing and this patch affects SDK add-ons. For the release it's less important since e10s is turned off for add-on users there, however since it affects the profiler add-on it would be really nice for internal work... Approval Request Comment [Feature/regressing bug #]: [User impact if declined]: SDK based add-ons trigger the shim layer and can cause performance issues unnecessary (janks in the content process) [Describe test coverage new/current, TreeHerder]: looks green on try, should be OK on inbound too soon [Risks and why]: it's a very simple patch I don't see any risk [String/UUID change made/needed]: none
Attachment #8785230 -
Flags: approval-mozilla-beta?
Attachment #8785230 -
Flags: approval-mozilla-aurora?
Reporter | ||
Comment 20•8 years ago
|
||
Looks like the large blocks of sync IPC for the addon are gone. The sync IPC I see left are: * A large block of sync IPC during initial tab load for PContent::SendReadPermissions() * A large block of sync IPC from content-sessionStore.js * A single blip for PScreenManager's GetPrimaryScreen() Those two large blocks happened when clicking on a placeholder tab that caused session store to reload it. Anyway, the issue originally reported here seems to be fixed. Thanks!
Flags: needinfo?(bkelly)
Reporter | ||
Comment 21•8 years ago
|
||
For reference, the profile I was looking at in comment 20 is: https://cleopatra.io/#report=c1bd0f092a1b8c00eda13426faa4413412df7ded This is an optimized DEBUG build with this bug's patch applied. I had to do a local build since I needed symbols.
Comment 22•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6a32c1e67f22
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
![]() |
||
Updated•8 years ago
|
Assignee: nobody → gkrizsanits
Comment 23•8 years ago
|
||
This is very late in beta to uplift but let's try it. We are planning on enabling e10s with some whitelisted addons during 49 and sounds like some of them would be affected by this.
Updated•8 years ago
|
status-firefox49:
--- → affected
status-firefox50:
--- → affected
Comment 24•8 years ago
|
||
Comment on attachment 8785230 [details] [diff] [review] Shimwaiver for l10n. v1 Fix for a sync issue with e10s enabled addons.
Attachment #8785230 -
Flags: approval-mozilla-beta?
Attachment #8785230 -
Flags: approval-mozilla-beta+
Attachment #8785230 -
Flags: approval-mozilla-aurora?
Attachment #8785230 -
Flags: approval-mozilla-aurora+
Comment 25•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/8fd9e2518fc2
Comment 26•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/1d47b6f9e8dc
You need to log in
before you can comment on or make changes to this bug.
Description
•