Closed Bug 1297446 Opened 3 years ago Closed 3 years ago

simple addons trigger large amounts of sync IPC in e10s

Categories

(Core :: DOM: Content Processes, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox49 --- fixed
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: bkelly, Assigned: gkrizsanits)

Details

Attachments

(2 files)

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)
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)
(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.
(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)
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)
Does JPM not support it? Do I have to repack the addon and add it manually?
Flags: needinfo?(bgirard) → needinfo?(wmccloskey)
Maybe we just need to repack the addon with a more recent version of the addon-sdk.
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)
(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)
(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.
(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+
Is this something we can or should uplift to other branches?
Flags: needinfo?(mrbkap)
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)
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?
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)
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.
https://hg.mozilla.org/mozilla-central/rev/6a32c1e67f22
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Assignee: nobody → gkrizsanits
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.
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+
You need to log in before you can comment on or make changes to this bug.