Closed Bug 1174461 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox41 --- affected
firefox42 --- affected
firefox48 --- fixed

People

(Reporter: jimm, Assigned: jimm)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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
Assignee: nobody → jmathies
Blocks: e10s-perf
tracking-e10s: --- → +
Attached patch patch (obsolete) — Splinter Review
Comment on attachment 8631567 [details] [diff] [review]
patch

This adds caching and fixes a bug in SetParent I noticed while working on this.
Attachment #8631567 - Flags: review?(aklotz)
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
Attachment #8631567 - Flags: review?(aklotz) → review+
Thanks, actually found both of those when I finally got around to building the patch. Sorry should have done that before posting.
Attached patch patch (obsolete) — Splinter Review
Attachment #8631567 - Attachment is obsolete: true
Attachment #8631895 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f612649fdbaf
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
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
Attachment #8631895 - Flags: approval-mozilla-aurora?
Comment on attachment 8631895 [details] [diff] [review]
patch

Low risk, no issues on m-c for this.
Attachment #8631895 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Backed out for causing bug 1182919.

https://hg.mozilla.org/mozilla-central/rev/a905dc2ebe69
https://hg.mozilla.org/releases/mozilla-aurora/rev/28deff24e5f9
Status: RESOLVED → REOPENED
Depends on: 1182919
Resolution: FIXED → ---
Target Milestone: mozilla42 → ---
Attachment #8631895 - Flags: approval-mozilla-aurora+
Summary: [e10s] Return a cached result from SendGetNativePluginPort whenever possible → [e10s] Return a cached result from SendGetNativePluginPort (avoids sync message from content to browser)
Kicking this out to e10s-perf, tracking plus.
This isn't showing up in any of our reports. I think it's safe to kick this out.
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → WONTFIX
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Attached patch patch v2Splinter Review
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.
Attachment #8631895 - Attachment is obsolete: true
Attachment #8729779 - Flags: review?(jmathies)
Attachment #8729779 - Flags: review?(jmathies) → review+
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
Flags: needinfo?(jmathies)
It wasn't this patch.
Flags: needinfo?(jmathies)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/da683f5c0a43
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1295596
You need to log in before you can comment on or make changes to this bug.