1174461 - [e10s] Return a cached result from SendGetNativePluginPort (avoids sync message from content to browser)

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jim Mathies [:jimm]]

We should cache this value to avoid the ipc traffic.

Here's the stack:

CONTENT
-----------------------------------------------------------------------------------
0 NtWaitForSingleObject
1 WaitForSingleObjectEx
2 WaitForSingleObjectExImplementation
3 WaitForSingleObject
4 PR_WaitCondVar src
5 mozilla::CondVar::Wait(unsigned int) src
6 mozilla::ipc::MessageChannel::WaitForSyncNotify() src
7 mozilla::ipc::MessageChannel::Send(IPC::Message *,IPC::Message *) src
8 mozilla::plugins::PPluginWidgetChild::SendGetNativePluginPort(unsigned int *) src
9 mozilla::widget::PluginWidgetProxy::GetNativeData(unsigned int) src
10 nsPluginFrame::GetWidgetConfiguration(nsTArray<nsIWidget::Configuration> *) src
11 PluginGetGeometryUpdate src
12 nsTHashtable<nsGenericHashKey<CertBlocklistItem> >::s_EnumStub(PLDHashTable *,PLDHashEntryHdr *,unsigned int,void *) src
13 nsRootPresContext::CollectPluginGeometryUpdates(mozilla::layers::LayerManager *) src
14 nsDisplayList::PaintRoot(nsDisplayListBuilder *,nsRenderingContext *,unsigned int) src
15 nsLayoutUtils::PaintFrame(nsRenderingContext *,nsIFrame *,nsRegion const &,unsigned int,unsigned int) src
16 PresShell::Paint(nsView *,nsRegion const &,unsigned int) src
17 nsViewManager::ProcessPendingUpdatesPaint(nsIWidget *) src
18 nsViewManager::ProcessPendingUpdatesForView(nsView *,bool) src
19 nsViewManager::ProcessPendingUpdates() src
20 nsRefreshDriver::Tick(__int64,mozilla::TimeStamp) src
21 mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver *,__int64,mozilla::TimeStamp) src
22 mozilla::RefreshDriverTimer::Tick(__int64,mozilla::TimeStamp) src
23 mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp) src
24 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) src
25 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) src
26 mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const &) src
27 mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const &) src
28 mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const &) src
29 mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const &) src
30 mozilla::ipc::MessageChannel::OnMaybeDequeueOne() src
31 MessageLoop::DoWork() src
32 mozilla::ipc::DoWorkRunnable::Run() src
33 nsThread::ProcessNextEvent(bool,bool *) src
34 NS_ProcessNextEvent(nsIThread *,bool) src
35 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *) src
36 mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate *) src
37 MessageLoop::RunHandler() src
38 MessageLoop::Run() src
39 nsBaseAppShell::Run() src
40 nsAppShell::Run() src
41 XRE_RunAppShell src
42 mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate *) src
43 MessageLoop::RunHandler() src
44 MessageLoop::Run() src
45 XRE_InitChildProcess src
46 content_process_main(int,char * * const) src
47 wmain src
48 __tmainCRTStartup
49 BaseThreadInitThunk
50 __RtlUserThreadStart
51 _RtlUserThreadStart

Comment 1

[Jim Mathies [:jimm]]

Attachment: patch

Comment 2

[Jim Mathies [:jimm]]

Comment on attachment 8631567 [details] [diff] [review]
patch

This adds caching and fixes a bug in SetParent I noticed while working on this.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8631567 [details] [diff] [review]
patch

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

::: widget/PluginWidgetProxy.cpp
@@ +36,5 @@
>                                       mozilla::plugins::PluginWidgetChild* aActor) :
>    PuppetWidget(aTabChild),
> +  mActor(aActor),
> +  mParent(nullptr),
> +  mCachedPluginPort(nullptr)

Nit: initialize mCachedPluginPort to 0

@@ +138,5 @@
>    }
> +  // The parent side window handle or xid never changes so we can
> +  // cache this for our lifetime.
> +  if (mCachedPluginPort) {
> +    return mCachedPluginPort;

This line should have a cast too

Comment 4

[Jim Mathies [:jimm]]

Thanks, actually found both of those when I finally got around to building the patch. Sorry should have done that before posting.

Comment 5

[Jim Mathies [:jimm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4daaca1c3e43

Comment 6

[Jim Mathies [:jimm]]

Attachment: patch

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f612649fdbaf

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f612649fdbaf

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 8631895 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]:
na
[User impact if declined]:
higher content crash rates with e10s, something we're trying to address on aurora and nightly right now.
[Describe test coverage new/current, TreeHerder]:
na
[Risks and why]: 
low
[String/UUID change made/needed]:
none

Comment 10

[Kate Glazko]

Comment on attachment 8631895 [details] [diff] [review]
patch

Low risk, no issues on m-c for this.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c3e5e5213726

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing bug 1182919.

https://hg.mozilla.org/mozilla-central/rev/a905dc2ebe69
https://hg.mozilla.org/releases/mozilla-aurora/rev/28deff24e5f9

Comment 13

[Jim Mathies [:jimm]]

Kicking this out to e10s-perf, tracking plus.

Comment 14

[Jim Mathies [:jimm]]

This isn't showing up in any of our reports. I think it's safe to kick this out.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I see this hurting us a lot in tab switching. When I switch to or from a page with a plugin, the content process has to send a sync message to the parent before it can do any drawing. This is at a time when the parent is doing a lot of work to update the URL bar, so it's pretty busy. In my profiles, it slows down painting by 20-30ms, so it costs us a couple frames.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v2

I think I found the problem in the patch. It was related to the SetParent thing, which isn't really fundamental, but we should fix it anyway. BaseCreate calls aParent->AddChild, but aParent isn't actually set to be the parent of the new widget. Then, later on, when we actually call SetParent, bad stuff happens.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f2e90feea3b

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out along with everything else from this push for build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=24870637&repo=mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/39e3c3e2970e

Comment 19

[Jim Mathies [:jimm]]

It wasn't this patch.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da683f5c0a43

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da683f5c0a43

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/da683f5c0a43