1067455 - Reduce Fence::merge() call

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Sotaro Ikeda [:sotaro]]

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.

Comment 1

[Sotaro Ikeda [:sotaro]]

FYI: android fence related sites.
- http://netaz.blogspot.ca/2013/10/android-fences-introduction-in-any.html
- https://android.googlesource.com/kernel/common/+/android-3.4/Documentation/sync.txt

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - Reduce Fence::merge() call on compositor thread

This is wip patch. By the patch, somehow gecko ipc notified incorrect fd num, when multiple async message is sent :-(

Comment 3

[Sotaro Ikeda [:sotaro]]

(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.

Comment 4

[Sotaro Ikeda [:sotaro]]

The problem in Comment 3 is fixed by partially reverting a fix of Bug 1042387.

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - Reduce Fence::merge() call on compositor thread

Fixed ipc fence delivery problem.

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Reduce Fence::merge() call on compositor thread

Comment 7

[Sotaro Ikeda [:sotaro]]

Get profile during home screen scrolling by applying a patch of bug 1010966 and attachment 8498219 [details] [diff] [review].

http://people.mozilla.org/~bgirard/cleopatra/#report=9cd58dabcef08ac8a23435b1f2de74f4b41f8576

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Reduce Fence::merge() call on compositor thread

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Reduce Fence::merge() call on compositor thread

Add limitation to a number of async messages per SendParentAsyncMessages() call.

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Reduce Fence::merge() call on compositor thread

Update a comment.

Comment 11

[Nicolas Silva [:nical]]

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 12

[Nicolas Silva [:nical]]

Comment on attachment 8498219 [details] [diff] [review]
patch - Reduce Fence::merge() call on compositor thread

patch marked obsolete, so removing the review flag.

Comment 13

[Sotaro Ikeda [:sotaro]]

(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.

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Reduce Fence::merge() call on compositor thread

Apply the comment.

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Reduce Fence::merge() call on compositor thread

Update a comment.

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Reduce Fence::merge() call on compositor thread

Fix nits.

Comment 17

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Reduce Fence::merge() call on compositor thread

Fix other platforms build failure. Carry "r=nical".

Comment 18

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Reduce Fence::merge() call on compositor thread

Fix build failure on other platform.

Comment 19

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Reduce Fence::merge() call on compositor thread

Fix build failure. Carry "r=nical".

Comment 20

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=62fdd3e98ece

Comment 21

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=ae40230ba430

Comment 22

[Sotaro Ikeda [:sotaro]]

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

Comment 23

[Phil Ringnalda (:philor)]

"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

Comment 24

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Reduce Fence::merge() call on compositor thread

Fix windows problem. Carry "r=nical".

Comment 25

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=09fe78b864fe

Comment 26

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/46b5882f52f8

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/46b5882f52f8

Comment 28

[Sotaro Ikeda [:sotaro]]

[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.

Comment 29

[Sotaro Ikeda [:sotaro]]

Attachment: patch for b2gv2.1 - Reduce Fence::merge() call on compositor thread

A patch for b2g v2.1. It include also Bug 1082986 fix. Carry "r=nical".

Comment 30

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=a9d0a9b1b8c3

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Please request b2g34 approval on this when you get a chance.

Comment 32

[Sotaro Ikeda [:sotaro]]

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.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/69e7ce415318

Comment 34

[Yeojin Chung [:YeojinC]]

Unable to verify as it is a back-end issue.