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

I am going to thinks about how it is possible.

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

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

IPC functions name including class names are following. (1) PBrowserChild::SendPRenderFrameConstructor() (2) PCompositorChild::SendPLayerTransactionConstructor()

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

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

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

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() :-(

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;

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;

(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

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.

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

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

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

modified [2] attachment 8510580 [details] [diff] [review] as to deliver also PRenderFrame. It seems to work on flame.

Update nits.

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.

un-bitrot.

Fix assert.

Fix TabParent constructor.

https://tbpl.mozilla.org/?tree=Try&rev=4a7bc44fce97 test failures were decreased, but still have some test failures.

child initiated window.open() sequence does not work.

(In reply to Sotaro Ikeda [:sotaro] from comment #27) > https://tbpl.mozilla.org/?tree=Try&rev=af5c3c0b2359 tryserver errors seems to be addressed.

Update nits.

un-bitrot.

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

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?

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

Apply the comments. Carry "r=bent".

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