Closed
Bug 1174461
Opened 9 years ago
Closed 8 years ago
[e10s] Return a cached result from SendGetNativePluginPort (avoids sync message from content to browser)
Categories
(Core Graveyard :: Plug-ins, defect)
Core Graveyard
Plug-ins
Tracking
(e10s+, firefox41 affected, firefox42 affected, firefox48 fixed)
RESOLVED
FIXED
mozilla48
People
(Reporter: jimm, Assigned: jimm)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
5.27 KB,
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
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
Updated•9 years ago
|
Updated•9 years ago
|
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
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 3•9 years ago
|
||
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+
Assignee | ||
Comment 4•9 years ago
|
||
Thanks, actually found both of those when I finally got around to building the patch. Sorry should have done that before posting.
Assignee | ||
Comment 5•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4daaca1c3e43
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8631567 -
Attachment is obsolete: true
Attachment #8631895 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f612649fdbaf
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Assignee | ||
Comment 9•9 years ago
|
||
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 10•9 years ago
|
||
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+
Comment 12•9 years ago
|
||
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 → ---
Updated•9 years ago
|
Attachment #8631895 -
Flags: approval-mozilla-aurora+
Assignee | ||
Updated•9 years ago
|
Summary: [e10s] Return a cached result from SendGetNativePluginPort whenever possible → [e10s] Return a cached result from SendGetNativePluginPort (avoids sync message from content to browser)
Assignee | ||
Comment 14•9 years ago
|
||
This isn't showing up in any of our reports. I think it's safe to kick this out.
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 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 → ---
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8729779 -
Flags: review?(jmathies) → review+
Comment 18•8 years ago
|
||
backout bugherder landing |
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)
Assignee | ||
Comment 19•8 years ago
|
||
It wasn't this patch.
Flags: needinfo?(jmathies)
Keywords: checkin-needed
Comment 20•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/da683f5c0a43
Keywords: checkin-needed
Comment 21•8 years ago
|
||
bugherder landing |
https://hg.mozilla.org/integration/mozilla-inbound/rev/da683f5c0a43
Comment 22•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/da683f5c0a43
Status: REOPENED → RESOLVED
Closed: 9 years ago → 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•