Closed Bug 1067455 Opened 5 years ago Closed 5 years ago

Reduce Fence::merge() call

Categories

(Core :: Graphics: Layers, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
blocking-b2g 2.1+
Tracking Status
firefox33 --- wontfix
firefox34 --- wontfix
firefox35 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(2 files, 12 obsolete files)

37.06 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
61.21 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
Fence::merge() consumes relatively longer time. It is better to reduce to call Reduce Fence::merge().

In the past, the Fence::merge() was removed by Bug 1028532. But it caused a regression of Bug 1055934. To improve the composition performance, we need to reduce to call Fence::merge() without regression.
Assignee: nobody → sotaro.ikeda.g
Status: NEW → ASSIGNED
Depends on: 1010966
This is wip patch. By the patch, somehow gecko ipc notified incorrect fd num, when multiple async message is sent :-(
(In reply to Sotaro Ikeda [:sotaro] from comment #2)
> This is wip patch. By the patch, somehow gecko ipc notified incorrect fd
> num, when multiple async message is sent :-(

I understand why it happens. If multiple AsyncParentMessageDatas are delivered as one AsyncMessages, all file descriptors are delivered by using one Message. Therefore multiple Fences are delivered by using one Message. ParamTraits<FenceHandle>::Read() checks number of fds by using Message::num_fds(). But it includes all fds of AsyncParentMessageDatas.
The problem in Comment 3 is fixed by partially reverting a fix of Bug 1042387.
Fixed ipc fence delivery problem.
Attachment #8497772 - Attachment is obsolete: true
Attachment #8497797 - Attachment is obsolete: true
No longer depends on: 1010966
Attachment #8498219 - Attachment is obsolete: true
Attachment #8498219 - Flags: review?(nical.bugzilla)
Add limitation to a number of async messages per SendParentAsyncMessages() call.
Attachment #8498381 - Attachment is obsolete: true
Update a comment.
Attachment #8498592 - Attachment is obsolete: true
Attachment #8498596 - Flags: review?(nical.bugzilla)
Comment on attachment 8498596 [details] [diff] [review]
patch - Reduce Fence::merge() call on compositor thread

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

Before r+ing this patch I need some clarification about the SendParentAsyncMessage calls in my comment below. Please also address the other two remarks (but the Send stuff is the most important part)

::: gfx/layers/ipc/ImageBridgeParent.cpp
@@ +465,5 @@
> +  for (size_t i = 0; i < mPendingAsyncMessage.size(); i++) {
> +    messages.AppendElement(mPendingAsyncMessage[i]);
> +    // Some type of message could have one file descriptor (e.g. OpDeliverFence).
> +    // A number of file descriptors per message have a limitation.
> +    // for linux, it is defined by FileDescriptorSet::MAX_DESCRIPTORS_PER_MESSAGE

This comment says that the limit is defined by a special constant on linux but we don't use this constant here. I think we should. maybe something like:

#ifdef THE_PLATFORM_WHERE_THE_CONSTANT_EXISTS
  static const uint32_t kMaxMessageNumber = FileDescriptorSet::MAX_DESCRIPTORS_PER_MESSAGE;
# else
  // default number that works everywhere else
  static const uint32_t kMaxMessageNumber = 250;
#endif

@@ +477,5 @@
> +  if (messages.Length() > 0) {
> +    mozilla::unused << SendParentAsyncMessages(messages);
> +  }
> +  mPendingAsyncMessage.clear();
> +  mozilla::unused << SendParentAsyncMessages(messages);

Do we need to Send again here? Looks like we just did in the branch 3 lines above and I don't see how we could have other messages to send after this. It looks like this would make us send the same fences twice in some cases.

::: gfx/layers/ipc/LayerTransactionParent.cpp
@@ +982,5 @@
> +  if (messages.Length() > 0) {
> +    mozilla::unused << SendParentAsyncMessages(messages);
> +  }
> +  mPendingAsyncMessage.clear();
> +  mozilla::unused << SendParentAsyncMessages(messages);

Same comment as for the ImageBridge equivalent

By the way, the implementation looks identical, perhaps you should move it into CompositableTransactionParent to avoid the duplication?
Comment on attachment 8498219 [details] [diff] [review]
patch - Reduce Fence::merge() call on compositor thread

patch marked obsolete, so removing the review flag.
Attachment #8498219 - Flags: review?(nical.bugzilla)
Attachment #8498596 - Flags: review?(nical.bugzilla)
(In reply to Nicolas Silva [:nical] from comment #11)
> Comment on attachment 8498596 [details] [diff] [review]
> patch - Reduce Fence::merge() call on compositor thread
> 
> Review of attachment 8498596 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Before r+ing this patch I need some clarification about the
> SendParentAsyncMessage calls in my comment below. Please also address the
> other two remarks (but the Send stuff is the most important part)
> 
> ::: gfx/layers/ipc/ImageBridgeParent.cpp
> @@ +465,5 @@
> > +  for (size_t i = 0; i < mPendingAsyncMessage.size(); i++) {
> > +    messages.AppendElement(mPendingAsyncMessage[i]);
> > +    // Some type of message could have one file descriptor (e.g. OpDeliverFence).
> > +    // A number of file descriptors per message have a limitation.
> > +    // for linux, it is defined by FileDescriptorSet::MAX_DESCRIPTORS_PER_MESSAGE
> 
> This comment says that the limit is defined by a special constant on linux
> but we don't use this constant here. I think we should. maybe something like:

I am going to update it in a next patch.

> 
> #ifdef THE_PLATFORM_WHERE_THE_CONSTANT_EXISTS
>   static const uint32_t kMaxMessageNumber =
> FileDescriptorSet::MAX_DESCRIPTORS_PER_MESSAGE;
> # else
>   // default number that works everywhere else
>   static const uint32_t kMaxMessageNumber = 250;
> #endif
> 
> @@ +477,5 @@
> > +  if (messages.Length() > 0) {
> > +    mozilla::unused << SendParentAsyncMessages(messages);
> > +  }
> > +  mPendingAsyncMessage.clear();
> > +  mozilla::unused << SendParentAsyncMessages(messages);
> 
> Do we need to Send again here? Looks like we just did in the branch 3 lines
> above and I don't see how we could have other messages to send after this.
> It looks like this would make us send the same fences twice in some cases.

The avove code is wrong implementation, latest one fixed the problem. It is sent to avoid to include more than max messages.

> 
> ::: gfx/layers/ipc/LayerTransactionParent.cpp
> @@ +982,5 @@
> > +  if (messages.Length() > 0) {
> > +    mozilla::unused << SendParentAsyncMessages(messages);
> > +  }
> > +  mPendingAsyncMessage.clear();
> > +  mozilla::unused << SendParentAsyncMessages(messages);
> 
> Same comment as for the ImageBridge equivalent
> 
> By the way, the implementation looks identical, perhaps you should move it
> into CompositableTransactionParent to avoid the duplication?

Yhea, I could move it to CompositableTransactionParent.
Blocks: 1076868
Apply the comment.
Attachment #8498596 - Attachment is obsolete: true
Update a comment.
Attachment #8499039 - Attachment is obsolete: true
Attachment #8499042 - Flags: review?(nical.bugzilla)
Fix nits.
Attachment #8499042 - Attachment is obsolete: true
Attachment #8499042 - Flags: review?(nical.bugzilla)
Attachment #8499144 - Flags: review?(nical.bugzilla)
Attachment #8499144 - Flags: review?(nical.bugzilla) → review+
Fix other platforms build failure. Carry "r=nical".
Attachment #8499144 - Attachment is obsolete: true
Attachment #8499565 - Flags: review+
Fix build failure on other platform.
Attachment #8499565 - Attachment is obsolete: true
Attachment #8499676 - Flags: review+
Fix build failure. Carry "r=nical".
Attachment #8499676 - Attachment is obsolete: true
Attachment #8499732 - Flags: review+
"ASSERTION: ImageBridgeParent for the process: 'sImageBridges.count(aId) == 1', file c:\builds\moz2_slave\m-in-w32-d-0000000000000000000\build\gfx\layers\ipc/ImageBridgeParent.cpp, line 325" in a bunch of tests in mochitest-1 through mochitest-5 and mochitest-chrome and mochitest-a11y in mochitest-other, on all three flavors of debug Windows.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/51509077b97c
Fix windows problem. Carry "r=nical".
Attachment #8499732 - Attachment is obsolete: true
Attachment #8500521 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/46b5882f52f8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Blocks: 1087605
[Blocking Requested - why for this release]: This bug blocks Bug 1087605. AS in Bug 1087605 Comment 42, during WebRTC cpu usage is very high. cpu usage reduction is very important.
blocking-b2g: --- → 2.1?
A patch for b2g v2.1. It include also Bug 1082986 fix. Carry "r=nical".
Attachment #8512698 - Flags: review+
blocking-b2g: 2.1? → 2.1+
Please request b2g34 approval on this when you get a chance.
Flags: needinfo?(sotaro.ikeda.g)
Comment on attachment 8512698 [details] [diff] [review]
patch for b2gv2.1 - Reduce Fence::merge() call on compositor thread

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none.
User impact if declined: WebRTC's video freeze possibility could be increased
Testing completed: locally tested.
Risk to taking this patch (and alternatives if risky): low 
String or UUID changes made by this patch: none.
Flags: needinfo?(sotaro.ikeda.g)
Attachment #8512698 - Flags: approval-mozilla-b2g34?
Attachment #8512698 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Unable to verify as it is a back-end issue.
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-]
You need to log in before you can comment on or make changes to this bug.