Improve TabChild::InitRenderingState() on gonk

RESOLVED FIXED in 2.1 S9 (21Nov)

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

unspecified
2.1 S9 (21Nov)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 12 obsolete attachments)

28.27 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
Assignee

Description

5 years ago
+++ This bug was initially created as a clone of Bug #1074783 +++

TabChild::InitRenderingState() take more than 100ms in flame device. It takes too long. We need to improve  it.
Assignee

Comment 1

5 years ago
I am going to thinks about how it is possible.
Assignee: nobody → sotaro.ikeda.g
Assignee

Comment 2

5 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #0)
> +++ This bug was initially created as a clone of Bug #1074783 +++
> 
> TabChild::InitRenderingState() take more than 100ms in flame device. It
> takes too long. We need to improve  it.

It depends on the timing, but almost all cases, it takes more then 60ms.
Assignee

Comment 3

5 years ago
In the TabChild::InitRenderingState(), the following two sync ipc are called, majority of cases, (1) took 70ms, (2) took 1ms. It seems to mean that b2g main thread is too busy to respond to ipc.

> (1) SendPRenderFrameConstructor()
> (2) SendPLayerTransactionConstructor()
Assignee

Comment 4

5 years ago
IPC functions name including class names are following.

(1) PBrowserChild::SendPRenderFrameConstructor()
(2) PCompositorChild::SendPLayerTransactionConstructor()
Assignee

Comment 5

5 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> IPC functions name including class names are following.
> 
> (1) PBrowserChild::SendPRenderFrameConstructor()

  This message is handled by TabParent in b2g main thread.

> (2) PCompositorChild::SendPLayerTransactionConstructor()

 This message is handled by CrossProcessCompositorParent in b2g compositor thread.
Assignee

Comment 6

5 years ago
I just tried the following 2 ways as feasible study.
- [1] Just change PBrowserChidl::SendPRenderFrameConstructor() to async transaction.
    + To asynchronize the transaction, I used AsyncTransactionTracker to handle the case that
      need to synchronously wait the async transaction's response on main thread.
      I thought there might be a case that PuppetWidget::GetLayerManager() is called
      before transaction end.
      It it happens, the GetLayerManager() returns nullptr.
- [2] Remove SendPRenderFrameConstructor()'s response.
      Instead TabChild::RecvShow() deliver the necessary information.
      By it, SendPLayerTransactionConstructor() seems not need to wait the transaction complete.
      Then asynchronize the SendPRenderFrameConstructor().
Assignee

Comment 7

5 years ago
The change becomes larger than I thought at first. The patch basically seems to work as expected. But it needs ImageBridge thread.
Assignee

Comment 8

5 years ago
Code becomes clear than attachment 8510573 [details] [diff] [review]. But the patch does not work. In the patch, asynchronize just SendPRenderFrameConstructor(), SendPLayerTransactionConstructor() is called same way as current gecko. It causes CrossProcessCompositorParent::AllocPLayerTransactionParent() to fail. SendPRenderFrameConstructor() have to be completed before calling SendPLayerTransactionConstructor() :-(
Assignee

Comment 9

5 years ago
I also tried another way.
- [3] asynchronize SendPRenderFrameConstructor() and the transaction complete event is
      delivered by main thread's IPC.
      SendPLayerTransactionConstructor() is triggered by the transaction complete event.
      But this way could have a problem. If PuppetWidget::GetLayerManager() is called
      before transaction end, the GetLayerManager() returns nullptr;
Assignee

Comment 10

5 years ago
This patch seems works on flame. But the patch have the following link. GetLayerManager()'s caller need to handle this case correctly if it happens.

> If PuppetWidget::GetLayerManager() is called before transaction end, the GetLayerManager() returns nullptr;
Assignee

Comment 11

5 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> Created attachment 8510629 [details] [diff] [review]
> patch pattern [3] - Asynchronize SendPRenderFrameConstructor() by using
> normal main thread ipc.
> 
> This patch seems works on flame. But the patch have the following link.
> GetLayerManager()'s caller need to handle this case correctly if it happens.

Sorry, it was in correct. If the transaction did not end GetLayerManager() creates and returns ClientLayerManager without PLayerTransactionChild. When the transaction is end, LayerTransactionChild is set to ShadowLayerForwarder that is used by the ClientLayerManager.

http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/PuppetWidget.cpp#356
Assignee

Comment 12

5 years ago
First PuppetWidget::GetLayerManager() is called from TabChild and the call allocates ClientLayerManager.
Second GetLayerManager() is triggered by nsDocShell::SetIsActive() and it's call sequence is like the following.

//from js code call
nsDocShell::SetIsActive()
->PresShell::SetIsActive()
 ->TabChild::MakeVisible()
  ->PuppetWidget::Invalidate()
   ->new PaintTask(this)
   ->NS_DispatchToCurrentThread(mPaintTask.get());
PuppetWidget::PaintTask::Run()
->PuppetWidget::Paint
 ->nsView::WillPaintWindow()
  ->nsViewManager::WillPaintWindow()
   ->nsIWidget::GetLayerManager()
    ->PuppetWidget::GetLayerManager()

From third call, it seems to be called under RefreshDriverTimer::Tick() call.
Assignee

Comment 13

5 years ago
Comment 12 is just typical case, PuppetWidget::GetLayerManager() could be called from another places.
Assignee

Comment 14

5 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #13)
> Comment 12 is just typical case, PuppetWidget::GetLayerManager() could be
> called from another places.

patch pattern [3] attachment 8510629 [details] [diff] [review] seems to work in normal situation, but it is fragile from architecture point of view.
Assignee

Comment 15

5 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> Created attachment 8510580 [details] [diff] [review]
> patch pattern [2] - TabChild::RecvShow() deliver the necessary information
> 
> Code becomes clear than attachment 8510573 [details] [diff] [review]. But
> the patch does not work. In the patch, asynchronize just
> SendPRenderFrameConstructor(), SendPLayerTransactionConstructor() is called
> same way as current gecko. It causes
> CrossProcessCompositorParent::AllocPLayerTransactionParent() to fail.
> SendPRenderFrameConstructor() have to be completed before calling
> SendPLayerTransactionConstructor() :-(

This problem seems fixlable.
Assignee

Comment 16

5 years ago
modified [2] attachment 8510580 [details] [diff] [review] as to deliver also PRenderFrame. It seems to work on flame.
Attachment #8510580 - Attachment is obsolete: true
Assignee

Comment 17

5 years ago
Update nits.
Attachment #8511128 - Attachment is obsolete: true
Assignee

Comment 18

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=443b3c91c1ba

pushed attachment 8511135 [details] [diff] [review] to tryserver. TabParent::Show() sees not correctly decide when RenderFrameParent should be created.
Assignee

Updated

5 years ago
Attachment #8510629 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8510573 - Attachment is obsolete: true
Assignee

Comment 19

5 years ago
un-bitrot.
Attachment #8511135 - Attachment is obsolete: true
Assignee

Comment 20

5 years ago
Fix assert.
Attachment #8516888 - Attachment is obsolete: true
Assignee

Comment 22

5 years ago
Fix TabParent constructor.
Attachment #8516915 - Attachment is obsolete: true
Assignee

Comment 23

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=4a7bc44fce97

test failures were decreased, but still have some test failures.
Assignee

Comment 24

5 years ago
child initiated window.open() sequence does not work.
Assignee

Comment 26

5 years ago
Attachment #8517742 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8517753 - Attachment description: patch - Improve TabChild::InitRenderingState() on gonk → patch - Improve TabChild::InitRenderingState()
Assignee

Comment 28

5 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #27)
> https://tbpl.mozilla.org/?tree=Try&rev=af5c3c0b2359

tryserver errors seems to be addressed.
Assignee

Comment 29

5 years ago
Update nits.
Attachment #8517753 - Attachment is obsolete: true
Assignee

Comment 30

5 years ago
un-bitrot.
Attachment #8518179 - Attachment is obsolete: true
Assignee

Comment 31

5 years ago
Comment on attachment 8518184 [details] [diff] [review]
patch - Improve TabChild::InitRenderingState()

bent, can you review the patch?
Attachment #8518184 - Flags: review?(bent.mozilla)
Comment on attachment 8518184 [details] [diff] [review]
patch - Improve TabChild::InitRenderingState()

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

This looks good to me, there are lots of code simplifications you can make if you remove your IPDL union MaybeRenderFrame:

::: dom/ipc/PBrowser.ipdl
@@ +69,5 @@
>    void_t;
>  };
>  
> +union MaybeRenderFrame {
> +  PRenderFrame;

You should be able to use 'nullable PRenderFrame' directly in an IPDL call instead of needing to make a union for this. Then all your c++ can just use pointers and null-check like normal.

@@ +434,5 @@
>       * point.
>       */
> +    Show(nsIntSize size, ScrollingBehavior scrolling,
> +         TextureFactoryIdentifier textureFactoryIdentifier, uint64_t layersId,
> +         MaybeRenderFrame renderFrame);

Nit: One arg per line looks more readable imo.

::: dom/ipc/TabChild.cpp
@@ +1498,5 @@
> +  TextureFactoryIdentifier textureFactoryIdentifier;
> +  uint64_t layersId = 0;
> +  MaybeRenderFrame maybeRenderFrame = mozilla::void_t();
> +  PRenderFrameChild* renderFrame = newChild->SendPRenderFrameConstructor();
> +  if (renderFrame) {

IPDL calls can never fail in the child->parent direction (we abort if they fail), so null checking isn't needed.

@@ +1905,3 @@
>  {
> +    MOZ_ASSERT((!mDidFakeShow && aRenderFrame.type() == MaybeRenderFrame::TPRenderFrameChild) ||
> +                (mDidFakeShow && aRenderFrame.type() == MaybeRenderFrame::Tvoid_t));

Nit: the second line's indent is off by 1

::: dom/ipc/TabParent.cpp
@@ +582,5 @@
> +    uint64_t layersId = 0;
> +    bool success = false;
> +    RenderFrameParent* renderFrame = nullptr;
> +    // If TabParent is initialized by parent side and RenderFrame is not
> +    // created yet, create it. If TabParent is initialized by child side,

This comment seems a little unclear. What about saying "If TabParent is initialized by parent side then the RenderFrame must also be created here." ?

@@ +583,5 @@
> +    bool success = false;
> +    RenderFrameParent* renderFrame = nullptr;
> +    // If TabParent is initialized by parent side and RenderFrame is not
> +    // created yet, create it. If TabParent is initialized by child side,
> +    // child side will create RenderFrame. 

Nit: a little trailing whitespace here and another below.

@@ +585,5 @@
> +    // If TabParent is initialized by parent side and RenderFrame is not
> +    // created yet, create it. If TabParent is initialized by child side,
> +    // child side will create RenderFrame. 
> +    MOZ_ASSERT(!GetRenderFrame());
> +    if (IsInitedByParent() && !GetRenderFrame()) {

It shouldn't be necessary to check GetRenderFrame() again after it has been asserted, right?

@@ +592,5 @@
> +            new RenderFrameParent(frameLoader,
> +                                  scrolling,
> +                                  &textureFactoryIdentifier,
> +                                  &layersId,
> +                                  &success);

Currently 'success' can only become false if you pass in a null frameLoader. Can you just check that before calling new to avoid the allocation? And then you could assert that success is always true if frameLoader is non-null.

@@ +1958,5 @@
>    return new RenderFrameParent(frameLoader,
> +                               scrolling,
> +                               &textureFactoryIdentifier,
> +                               &layersId,
> +                               &success);

Hrm, so what should happen here if frameLoader is null (and therefore success will be false)? Should you still return an actor here?

@@ +1975,5 @@
> +                                  TextureFactoryIdentifier* aTextureFactoryIdentifier,
> +                                  uint64_t* aLayersId)
> +{
> +  RenderFrameParent* renderFrame = static_cast<RenderFrameParent*>(aRenderFrame);
> +  *aScrolling = UseAsyncPanZoom() ? ASYNC_PAN_ZOOM : DEFAULT_SCROLLING;

Is there any risk that this value could be different from the one you set when you created the RenderFrameParent above? Like, if someone changes a pref or something?

::: dom/ipc/TabParent.h
@@ +451,5 @@
>      bool mSendOfflineStatus;
>  
>      uint32_t mChromeFlags;
>  
> +    bool mInitedByParent;

Please add a comment about what this means.

::: layout/ipc/RenderFrameParent.h
@@ +110,5 @@
>    bool HitTest(const nsRect& aRect);
>  
> +  void GetTextureFactoryIdentifier(TextureFactoryIdentifier* aTextureFactoryIdentifier);
> +
> +  void GetLayersId(uint64_t* aLayersId);

Why not just have this return uint64_t? And inline it?
Attachment #8518184 - Flags: review?(bent.mozilla) → review+
Assignee

Comment 33

5 years ago
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #32)
> Comment on attachment 8518184 [details] [diff] [review]
> patch - Improve TabChild::InitRenderingState()
> 
> Review of attachment 8518184 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me, there are lots of code simplifications you can make
> if you remove your IPDL union MaybeRenderFrame:
> 
> ::: dom/ipc/PBrowser.ipdl
> @@ +69,5 @@
> >    void_t;
> >  };
> >  
> > +union MaybeRenderFrame {
> > +  PRenderFrame;
> 
> You should be able to use 'nullable PRenderFrame' directly in an IPDL call
> instead of needing to make a union for this. Then all your c++ can just use
> pointers and null-check like normal.

Thanks! I did not know about it. It will be updated in next patch.

> 
> @@ +434,5 @@
> >       * point.
> >       */
> > +    Show(nsIntSize size, ScrollingBehavior scrolling,
> > +         TextureFactoryIdentifier textureFactoryIdentifier, uint64_t layersId,
> > +         MaybeRenderFrame renderFrame);
> 
> Nit: One arg per line looks more readable imo.

It will be updated in a next patch.

> 
> ::: dom/ipc/TabChild.cpp
> @@ +1498,5 @@
> > +  TextureFactoryIdentifier textureFactoryIdentifier;
> > +  uint64_t layersId = 0;
> > +  MaybeRenderFrame maybeRenderFrame = mozilla::void_t();
> > +  PRenderFrameChild* renderFrame = newChild->SendPRenderFrameConstructor();
> > +  if (renderFrame) {
> 
> IPDL calls can never fail in the child->parent direction (we abort if they
> fail), so null checking isn't needed.

It will be removed in a next patch.

> 
> @@ +1905,3 @@
> >  {
> > +    MOZ_ASSERT((!mDidFakeShow && aRenderFrame.type() == MaybeRenderFrame::TPRenderFrameChild) ||
> > +                (mDidFakeShow && aRenderFrame.type() == MaybeRenderFrame::Tvoid_t));
> 
> Nit: the second line's indent is off by 1

It will be updated in a next patch.

> 
> ::: dom/ipc/TabParent.cpp
> @@ +582,5 @@
> > +    uint64_t layersId = 0;
> > +    bool success = false;
> > +    RenderFrameParent* renderFrame = nullptr;
> > +    // If TabParent is initialized by parent side and RenderFrame is not
> > +    // created yet, create it. If TabParent is initialized by child side,
> 
> This comment seems a little unclear. What about saying "If TabParent is
> initialized by parent side then the RenderFrame must also be created here." ?

Thanks. It will be updated in a next patch.

> 
> @@ +583,5 @@
> > +    bool success = false;
> > +    RenderFrameParent* renderFrame = nullptr;
> > +    // If TabParent is initialized by parent side and RenderFrame is not
> > +    // created yet, create it. If TabParent is initialized by child side,
> > +    // child side will create RenderFrame. 
> 
> Nit: a little trailing whitespace here and another below.

It will be updated in a next patch.

> 
> @@ +585,5 @@
> > +    // If TabParent is initialized by parent side and RenderFrame is not
> > +    // created yet, create it. If TabParent is initialized by child side,
> > +    // child side will create RenderFrame. 
> > +    MOZ_ASSERT(!GetRenderFrame());
> > +    if (IsInitedByParent() && !GetRenderFrame()) {
> 
> It shouldn't be necessary to check GetRenderFrame() again after it has been
> asserted, right?

Yes, it will be updated in a next patch.
 
> @@ +592,5 @@
> > +            new RenderFrameParent(frameLoader,
> > +                                  scrolling,
> > +                                  &textureFactoryIdentifier,
> > +                                  &layersId,
> > +                                  &success);
> 
> Currently 'success' can only become false if you pass in a null frameLoader.
> Can you just check that before calling new to avoid the allocation? And then
> you could assert that success is always true if frameLoader is non-null.

It will be applied in a next patch.

> 
> @@ +1958,5 @@
> >    return new RenderFrameParent(frameLoader,
> > +                               scrolling,
> > +                               &textureFactoryIdentifier,
> > +                               &layersId,
> > +                               &success);
> 
> Hrm, so what should happen here if frameLoader is null (and therefore
> success will be false)? Should you still return an actor here?
> 

It is called as a result of PBrowserChild::SendPRenderFrameConstructor(). Therefore if we return null as a actor, the content process will be aborted. But in normal situation, frameLoader is null should not happen because "newChild->SendBrowserFrameOpenWindow()" should create the frame loader.

> @@ +1975,5 @@
> > +                                  TextureFactoryIdentifier* aTextureFactoryIdentifier,
> > +                                  uint64_t* aLayersId)
> > +{
> > +  RenderFrameParent* renderFrame = static_cast<RenderFrameParent*>(aRenderFrame);
> > +  *aScrolling = UseAsyncPanZoom() ? ASYNC_PAN_ZOOM : DEFAULT_SCROLLING;
> 
> Is there any risk that this value could be different from the one you set
> when you created the RenderFrameParent above? Like, if someone changes a
> pref or something?

It seems better to get UseAsyncPanZoom() from RenderFrameParent. It will be updated in a next patch.

> 
> ::: dom/ipc/TabParent.h
> @@ +451,5 @@
> >      bool mSendOfflineStatus;
> >  
> >      uint32_t mChromeFlags;
> >  
> > +    bool mInitedByParent;
> 
> Please add a comment about what this means.

It will be added in a next patch.

> 
> ::: layout/ipc/RenderFrameParent.h
> @@ +110,5 @@
> >    bool HitTest(const nsRect& aRect);
> >  
> > +  void GetTextureFactoryIdentifier(TextureFactoryIdentifier* aTextureFactoryIdentifier);
> > +
> > +  void GetLayersId(uint64_t* aLayersId);
> 
> Why not just have this return uint64_t? And inline it?

It will be updated in a next patch.
Assignee

Comment 34

5 years ago
Apply the comments. Carry "r=bent".
Attachment #8518184 - Attachment is obsolete: true
Attachment #8523139 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/cabc3b75da32
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED

Updated

4 years ago
Target Milestone: --- → 2.1 S9 (21Nov)
You need to log in before you can comment on or make changes to this bug.