67.5% regression in loading.desktop at 606947:607002 |
||||||||||||
Issue descriptionSee the link to graphs below.
,
Nov 14
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/13feacade40000
,
Nov 14
📍 Found a significant difference after 1 commit. https://pinpoint-dot-chromeperf.appspot.com/job/13feacade40000 Reland "Don't make a LayerTreeFrameSink for a non-visible RenderWidget." by danakj@chromium.org https://chromium.googlesource.com/chromium/src/+/678f025f7cde75d3c4c9dddbaefde07aec197faa timeToFirstContentfulPaint: 589.6 → 993.1 (+403.5) Understanding performance regressions: http://g.co/ChromePerformanceRegressions Benchmark documentation link: https://bit.ly/loading-benchmarks
,
Nov 14
Ok so this is great. This test does: 1. Starts a RenderWidget for about:blank, it's swapped in, starts the compositor. [1:1:1114/112323.308689:ERROR:render_widget.cc(1545)] 0x1e56aa77e800 Started swapped in [1:1:1114/112323.308791:ERROR:render_widget.cc(2913)] 0x1e56aa77e800 Started compositor : hidden=0 [1:1:1114/112323.347930:ERROR:render_widget.cc(983)] 0x1e56aa77e800 Request FrameSink about:blank 2. Starts another RenderWidget, swapped out, for wikipedia [1:1:1114/112324.560918:ERROR:render_widget.cc(1545)] 0x1e56aa77b800 Started swapped out 3. Swaps in the wikipedia RenderWidget [1:1:1114/112324.681254:ERROR:render_widget.cc(2913)] 0x1e56aa77b800 Started compositor : hidden=0 4. Swaps out the about:blank widget [1:1:1114/112324.685302:ERROR:render_widget.cc(2920)] 0x1e56aa77e800 Stopped compositor : hidden=1 5. Then the compositor finishes starting for wikipedia [1:1:1114/112324.696238:ERROR:render_widget.cc(983)] 0x1e56aa77b800 Request FrameSink https://ja.wikipedia.org/wiki/%E3%82%B4%E3%83%AB%E3%82%B413%E3%81%AE%E3%82%A8%E3%83%94%E3%82%BD%E3%83%BC%E3%83%89%E4%B8%80%E8%A6%A7 Initially I thought it must be step 4 slowing things down, but it is not that. Removing the StopCompositor() has no real impact. Starting the compositor for the initially swapped out RenderWidget immediately is the difference maker. I don't know how to satisfy both "dont keep around a RenderWidget/compositor when there is no local main frame" and "navigate quickly" here.
,
Nov 14
Here's how starting the compositor immediately looks relative to the swap in about:blank: [1:1:1114/115136.707259:ERROR:render_widget.cc(1545)] 0x1f7f65834800 Started swapped in [1:1:1114/115136.707389:ERROR:render_widget.cc(2914)] 0x1f7f65834800 Started compositor : hidden=0 [1:1:1114/115136.745501:ERROR:render_widget.cc(983)] 0x1f7f65834800 Request FrameSink about:blank wikipedia: [1:1:1114/115137.964561:ERROR:render_widget.cc(1545)] 0x1f7f65832800 Started swapped out [1:1:1114/115137.964736:ERROR:render_widget.cc(2914)] 0x1f7f65832800 Started compositor : hidden=0 [1:1:1114/115137.975873:ERROR:render_widget.cc(983)] 0x1f7f65832800 Request FrameSink wikipedia: [1:1:1114/115138.096644:ERROR:render_frame_impl.cc(5912)] 0x1f7f65832800 Swapping in about:blank: [1:1:1114/115138.101639:ERROR:render_frame_impl.cc(2243)] 0x1f7f65834800 Swapping out So the compositor starts, posts to the main thread to get a frame sink, that runs, all before the swap in message comes from the browser.
,
Nov 14
Once RenderWidget is owned by the LocalFrame, which is created at the point of Swapping in, we won't have the ability to start this compositor sooner. So I feel like we actually need to SwapIn and set up the LocalFrame sooner. I have no idea what that actually looks like though.. +some more people for opinions
,
Nov 14
It seems like there is a LocalFrame for the main frame during this time, but it is a provisional one. I recall dcheng@ hoping to get rid of provisional stuff. It is my understanding that provisional implies it will become swapped in. So maybe we could have RenderWidget swapped in when there is a provisional frame, instead of only when the real frame is swapped in?
,
Nov 14
Starting it with a provisional frame makes sense to me. IIRC, a provisional frame occurs when we start navigating but haven't figured out whether or not it'll complete. Thus there is a strong intent to swap it in. What does that mean architecturally though?
,
Nov 14
#c8: A provisional main frame that's not yet swapped in will always have a proxy (see issue 756790 for why that's needed), and the proxy implies that the associated RW/RV is swapped-out, so I'm not sure how we would use a swapped-in RW for it? In the past, we sometimes created provisional frames without a proxy and with a swapped-in RW/RV, but that led to subtle bugs and races (e.g., what if the provisional frame ends up not committing if the navigation is canceled by a subsequent navigation before response is received, but in the meantime other subframes get created in this process that expect the main frame to be a proxy and the main frame's RW/RV to be swapped-out). So we had to change the design away from that; some more context in issues 591478, 756790, 651980 , 653746 , 627400.
,
Nov 14
To help clarify, there are 2 concepts of swap in/out so let me disambiguate. The RenderFrameImpl has a swapped in/out state which is when it swaps a WebFrame with another, out meaning a remote, in meaning a local. However swapping in also happens from a local provisional frame to a local frame. RenderWidget has a swapped in/out state that is only used for the main frame, and is set to swapped out when the RenderFrameImpl is swapped out and vice versa. I'm proposing changing when the latter happens, and maybe rename it to be more clear. I would not change the RenderFrame being swapped. What this would mean in future wonder world is that we'd have a RenderWidget on the Local (Provisional) Frame, and transfer it over to the real RenderWidget when navigation resolves.
,
Nov 14
Having a separate RW on the provisional frame, and transferring it over to the real RW if/when that provisional frame commits makes sense to me. I'm still not sure about the proposal prior to that though. Let's say a page A window.opens same-site page A2, and then A2 navigates to B, but B doesn't respond. At that point, we'll create a process B, which will have (1) swapped-out RW and RemoteFrame for page A, (2) swapped-out RW and RemoteFrame for page A2, and (3) provisional LocalFrame for the navigation to B. At this point, the remote frames in (1) and (2) are in the frame tree, but (3) isn't. As I understand, you're proposing to change the RW in (2) to swapped-in - is that right? But that'll result in the remote main frame that's there being associated with a swapped-in RW/RV, which we assume never happens. Suppose A now scripts the popup it opened to add a B2 subframe to A2 (still the current frame), and that commits earlier than B. That subframe B2 is created in process B, and it assumes that it will have a swapped-out RV in the tree. If the navigation to B is now canceled, that RV should stay as swapped-out. Can that still happen with your proposal? (I haven't followed the RV/RW split closely, but perhaps it's far along that we can actually keep a swapped-out RV but a swapped-in RW in this case?)
,
Nov 14
> As I understand, you're proposing to change the RW in (2) to swapped-in - is that right? I'm attempting to draw pictures to follow and I think that is not what I'm proposing. In this case (3) is a RenderView/Widget with a provisional local frame (not in the tree, but attached to the RenderWidget none-the-less) so it would swap in the RenderWidget. If the navigation to B fails, what happens to (3) RenderView/Widget at that point, are they closed? > Suppose A now scripts the popup it opened to add a B2 subframe to A2 (still the current frame), and that commits earlier than B. That subframe B2 is created in process B, and it assumes that it will have a swapped-out RV in the tree. If the navigation to B is now canceled, that RV should stay as swapped-out. This is going to be a provisional/local frame in the (2) frame tree, so it shouldn't be affected by (3)'s widget being swapped in. > (I haven't followed the RV/RW split closely, but perhaps it's far along that we can actually keep a swapped-out RV but a swapped-in RW in this case?) RenderView has no concept of swapped in/out, as its just the top of the frame tree in either case.
,
Nov 14
So based on your picture, would we have two widgets coexisting at the same time in process B: swapped-out for A2 and swapped-in for B? (I think today there would be one widget in this situation rather than two.) Would they still share the top-level RenderView? Note that on the browser side, the RenderViewHost maintains swapped-out state rather than RenderWidgetHost. We rely on that pretty heavily when deciding how to create proxies, provisional frames, etc., and it's expected to be kept in sync with the renderer's view of swapped out state.
,
Nov 14
Let's back up a sec so we're on the same page. A is a Page so it has a RenderView A does window.open that makes a new Page and a new RenderView? B navigation is another new Page and a new RenderView? So that's why I have drawn 3 RenderWidgets (each being for the main frame). What does a top-level RenderView mean? > Note that on the browser side, the RenderViewHost maintains swapped-out state rather than RenderWidgetHost. I think this correlates to the *RenderFrameImpl* swapped in/out state, not the RenderWidget's. RenderWidget is a "I can't delete you so have this bool instead".
,
Nov 14
Yes, the idea of creating a "swapped in" RenderView/Widget for the provisional LocalFrame seems to make sense to me at a high level, while keeping (2) "swapped out." And yes, hopefully the provisional LocalFrame and (3) RenderView/Widget would all get cleaned up if the navigation to B fails. From an even higher level, I think we probably want to move away from calling these "swapped in/out" states for frames, widgets, and views. The term "swapped out" dates back to when we kept a RenderViewHost around (on the swappedout:// URL) after a cross-process navigation, since we didn't have proxies at the time. For frames, "swapped in" now basically means RenderFrameImpl/LocalFrame and "swapped out" means "RenderFrameProxy/RemoteFrame," which are different objects and don't need keep track of the state explicitly. (I don't see a "swapped in/out" state on RenderFrameImpl, for example.) For widgets, on the other hand, I think "swapped out" becomes something like a "compositor off" state, for the case that no LocalFrames (even provisional ones) are using it. I think that lines up with Dana's proposal here, where the widget starts its compositor when it gains a provisional LocalFrame. We don't need to change the terminology immediately if that's hard, but I imagine the sooner we move away from the old "swapped out state" wording, the less confusing things will be. Would that make sense?
,
Nov 14
Ah, I wrote comment 17 before comments 15-16 came in. If we're relying on RenderViewHost swapped-out state, there may be some untangling needed here. I'll step back and let Alex and Dana make progress, and we can chat in person if my comment doesn't make sense. :)
,
Nov 14
I was also previously writing a comment to try disambiguate the many forms of "swapped out". I think we should rename it in RenderWidget at least.
,
Nov 14
Agreed that we should deprecate the swapped-out terminology. :) For RenderViewHost, we use the active vs inactive terminology and expose it via is_active() (although there's also is_swapped_out(), which is ugly and should go away) - perhaps that's a better way to express this? To pile on to Charlie's classification, for RenderView (i.e., page representation), the active bit should correspond to the main frame's state - it's active/swapped-in when the main frame is local, and inactive/swapped-out when the main frame is remote. > A is a Page so it has a RenderView > A does window.open that makes a new Page and a new RenderView? Right. That's in process A. > B navigation is another new Page and a new RenderView? B navigation creates a new process and representations for all frames that are in that BrowsingInstance (i.e., script-connected). That means it creates: a. a new Page, RenderView, inactive/swapped-out RenderWidget, and RemoteFrame representing A (the opener) b. a new Page, RenderView, inactive/swapped-out RenderWidget, and RemoteFrame representing A2 c. a provisional LocalFrame for B. > So that's why I have drawn 3 RenderWidgets (each being for the main frame). Sorry, I meant to only talk about the widgets in process B. My question is whether the provisional LocalFrame for B gets its own active widget in your proposal, distinct from the inactive one already created for the remote main frame in (b). And if yes, does it also reuse the RenderView from (b) or create a new "provisional" RenderView that will replace the one from (b) if/when the provisional LocalFrame for B commits?
,
Nov 14
Ah I see, no. I'm proposing the currently inactive RenderWidget for (3) in process B be made active at any time it has a provisional local frame or full local frame, instead of only when it has the latter.
,
Nov 14
#c21: Got it. So my concern with that is, going back to the example where A2 also gets a B2 subframe, B2 also needs a page representation, and currently it reuses the provisional frame's RenderView (we assume we can only have one RenderView per page). That RenderView will now think it's active based on its underlying RenderWidget, but this will be wrong if B2 commits but B is slow to commit or never commits.
,
Nov 14
RenderViewImpl does not ever query its RenderWidget to being active or not, so I don't think that it cares (correctly). Any code that cares is part of RenderWidget. Do you know where what I claim falls apart?
,
Nov 14
,
Nov 14
With the B2 example, I think we might end up with a situation where the main frame is a proxy, and main frame's RenderWidget is active whereas in fact it shouldn't be. I'm just trying to prevent that. :) If we didn't prevent it, it seems the is_swapped_out_ uses in RenderWidget would be broken, we'd still have the compositor running unnecessarily, and the browser process might get out of sync on whether or not the view is active. I also don't know if we can change B's RenderWidget to active without also changing the browser's corresponding RenderViewHost to active, and the latter might break the proxy creation logic from r501081 / issue 756790. Seems that SwapIn() and CreateFrameProxy() also look up the RenderView's GetWidget()->is_swapped_out(), so we should think through those.
,
Nov 14
> With the B2 example, I think we might end up with a situation where the main frame is a proxy, and main frame's RenderWidget is active whereas in fact it shouldn't be. I'm just trying to prevent that. :) Yess definitely. That's how I got here :) https://bugs.chromium.org/p/chromium/issues/detail?id=894899 > and the browser process might get out of sync on whether or not the view is active. Right, the communication about swapped in/out seems unidirectional from the browser. So when we make a remote main frame, we can "deactivate" the widget and when we make a local main frame (provisional or not) we can "activate" the widget. This should keep it in sync. > I also don't know if we can change B's RenderWidget to active without also changing the browser's corresponding RenderViewHost to active, and the latter might break the proxy creation logic from r501081 Ok let me look into that tomorrow. The one place I found just now renaming "swapped out" was IPC whitelisting. We reduce the IPCs send from the widget when it is a zombie, so this would increase the IPCs it will send while the frame is swapped out. Maybe we need a different bool for that then - literally the swapped out state from the frame? Needs more looking. > Seems that SwapIn() and CreateFrameProxy() also look up the RenderView's GetWidget()->is_swapped_out(), so we should think through those. These do so only to not change it when it's already in the state it wants. I think it was done because RenderWidget used to DCHECK and these failed the DCHECK so the if()s were added. I don't see them being a problem.
,
Nov 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6dcbc596f7da2bab093e93123469da0a3bc3bebf commit 6dcbc596f7da2bab093e93123469da0a3bc3bebf Author: danakj <danakj@chromium.org> Date: Fri Nov 16 16:45:42 2018 Rename "swapped out" and "for oopif" in RenderWidget. is swapped out => is frozen for oopif => for child local root frame "Swapped out" was used in RenderWidget as the term comes from RenderViewHost, and RenderFrame. The main frame can be swapped out - meaning hosted in another process. RenderWidget is a separate layer of the code however and we would like to delete them but we can't, so they must enter this state of "inactivity". Thus, frozen. Then we can more clearly separate frozen RenderWidgets from swapped out frames and freeze/thaw RenderWidgets more appropriately. "For oopif" is confusing as it seems to mean the RenderWidget is not in the same frame tree as the main frame. However it can be when there is an oopif in between the frame in question and the main frame. Rename it to be more clear about when it is true. R=ajwong@chromium.org, piman@chromium.org Change-Id: I52e31acfbf098bfb993d1aea69b699b7bcbb8cbf Bug: 905191 , 419087 Reviewed-on: https://chromium-review.googlesource.com/c/1336218 Commit-Queue: danakj <danakj@chromium.org> Reviewed-by: Antoine Labour <piman@chromium.org> Cr-Commit-Position: refs/heads/master@{#608808} [modify] https://crrev.com/6dcbc596f7da2bab093e93123469da0a3bc3bebf/content/renderer/input/widget_input_handler_impl.cc [modify] https://crrev.com/6dcbc596f7da2bab093e93123469da0a3bc3bebf/content/renderer/input/widget_input_handler_manager.cc [modify] https://crrev.com/6dcbc596f7da2bab093e93123469da0a3bc3bebf/content/renderer/render_frame_impl.cc [modify] https://crrev.com/6dcbc596f7da2bab093e93123469da0a3bc3bebf/content/renderer/render_frame_proxy.cc [modify] https://crrev.com/6dcbc596f7da2bab093e93123469da0a3bc3bebf/content/renderer/render_view_impl.cc [modify] https://crrev.com/6dcbc596f7da2bab093e93123469da0a3bc3bebf/content/renderer/render_view_impl.h [modify] https://crrev.com/6dcbc596f7da2bab093e93123469da0a3bc3bebf/content/renderer/render_widget.cc [modify] https://crrev.com/6dcbc596f7da2bab093e93123469da0a3bc3bebf/content/renderer/render_widget.h
,
Nov 16
Btw, I know a lot of the conversation here is revolving around tying a widget to the provisional local frame. I just want to mention that I'm investigating removing provisional local frames, so whatever solution we end up picking, we should have a plan for how it'll work if there's no provisional local frame.
,
Nov 16
It occurred to me that maybe we only need to warm up the process of starting the compositor by kicking off making a GpuChannelHost when the RenderView+swapped out main frame are created. However that was not enough, there are also IPCs involved in making the CompositorFrameSink. But if we create a LayerTreeFrameSink eagerly (instead of starting the compositor), and then hand that off when the compositor /is/ started (if it is..) then that resolves the regression. In the future I think that looks something like RenderViewImpl making the frame sink, and passing it along to the RenderWidget when it is initialized. For now, I can stash it on RenderWidget. But when do I know the navigation is not going to happen and I can drop the FrameSink?
,
Nov 17
https://chromium-review.googlesource.com/c/chromium/src/+/1341073 recovers this regression for me locally without changing the frozen state of RenderWidget. How it will translate to a post-provisional-frame world is not clear, but we'll have to tie it to notice of navigation starting/aborting somehow, and stash a RenderWidget (if we can make one without any WebWidget??) or just the LayerTreeFrameSink (with a RenderWidget routing id, where does that come from if we're not creating a provisional frame?) or something, until the nav completes and we can use that when making the LocalFrame.
,
Nov 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bc6ba9d2f20678ae4837aef48ea45946e4885597 commit bc6ba9d2f20678ae4837aef48ea45946e4885597 Author: danakj <danakj@chromium.org> Date: Sat Nov 17 04:03:41 2018 Followups on renaming RenderWidget "swapped out" to "is frozen". From https://crrev.com/c/1336218 R=creis@chromium.org Change-Id: Ic172b109019fad339173393e29da6870e7774fa1 Bug: 905191 , 419087 Reviewed-on: https://chromium-review.googlesource.com/c/1341077 Reviewed-by: Charlie Reis <creis@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/heads/master@{#609091} [modify] https://crrev.com/bc6ba9d2f20678ae4837aef48ea45946e4885597/content/renderer/render_frame_impl.cc [modify] https://crrev.com/bc6ba9d2f20678ae4837aef48ea45946e4885597/content/renderer/render_widget.cc [modify] https://crrev.com/bc6ba9d2f20678ae4837aef48ea45946e4885597/content/renderer/render_widget.h
,
Nov 26
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/66794d00db74fff08d3c7944bab7337494beb544 commit 66794d00db74fff08d3c7944bab7337494beb544 Author: danakj <danakj@chromium.org> Date: Mon Nov 26 19:16:11 2018 Start gpu channel and compositor mojo pipe collection eagerly When a RenderWidget is frozen, its compositor is stopped. However when the main frame is being navigated, we want to start requesting mojo pipes for the gpu and display compositor immediately so that it can happen in parallel with the navigation loading. Previously, we just always did this when creating a RenderWidget, and left them active when freezing the RenderWidget. After 678f025f7cde75 however, we don't, and this causes time-to-first-pixels regressions on navigation. So we add a WarmupCompositor() method to RenderWidget, and call this from RenderFrameImpl::CreateFrame() when it makes the provisional frame, and the WebFrameWidget, since we expect to make use of the main frame RenderWidget shortly. Then, if RenderFrameImpl::FrameDetached() occurs, due to the navigation failing, we will AbortWarmupCompositor() to drop the mojo pipes instead of holding onto them indefinitely. This recovers the loading regressions introduced, while also not allocating mojo channels for frozen RenderWidgets indefinitely. In order to do this reasonably, we drop the "callback" from the request to RenderThreadImpl::RequestNewLayerTreeFrameSink so that it always returns something immediately. This way RenderWidget does not need to worry about having a task run to collect the new frame sink, and ordering with tasks from the compositor to collect it. R=piman@chromium.org Change-Id: I6007e26e1622006652ce3619cdd080539d11e7b0 Bug: 905191 Reviewed-on: https://chromium-review.googlesource.com/c/1341073 Commit-Queue: danakj <danakj@chromium.org> Reviewed-by: danakj <danakj@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Antoine Labour <piman@chromium.org> Cr-Commit-Position: refs/heads/master@{#610909} [modify] https://crrev.com/66794d00db74fff08d3c7944bab7337494beb544/content/renderer/gpu/layer_tree_view.cc [modify] https://crrev.com/66794d00db74fff08d3c7944bab7337494beb544/content/renderer/gpu/layer_tree_view_delegate.h [modify] https://crrev.com/66794d00db74fff08d3c7944bab7337494beb544/content/renderer/gpu/layer_tree_view_unittest.cc [modify] https://crrev.com/66794d00db74fff08d3c7944bab7337494beb544/content/renderer/mus/renderer_window_tree_client.cc [modify] https://crrev.com/66794d00db74fff08d3c7944bab7337494beb544/content/renderer/mus/renderer_window_tree_client.h [modify] https://crrev.com/66794d00db74fff08d3c7944bab7337494beb544/content/renderer/render_frame_impl.cc [modify] https://crrev.com/66794d00db74fff08d3c7944bab7337494beb544/content/renderer/render_thread_impl.cc [modify] https://crrev.com/66794d00db74fff08d3c7944bab7337494beb544/content/renderer/render_thread_impl.h [modify] https://crrev.com/66794d00db74fff08d3c7944bab7337494beb544/content/renderer/render_widget.cc [modify] https://crrev.com/66794d00db74fff08d3c7944bab7337494beb544/content/renderer/render_widget.h [modify] https://crrev.com/66794d00db74fff08d3c7944bab7337494beb544/content/test/stub_layer_tree_view_delegate.cc [modify] https://crrev.com/66794d00db74fff08d3c7944bab7337494beb544/content/test/stub_layer_tree_view_delegate.h [modify] https://crrev.com/66794d00db74fff08d3c7944bab7337494beb544/third_party/blink/renderer/core/testing/sim/sim_compositor.cc [modify] https://crrev.com/66794d00db74fff08d3c7944bab7337494beb544/third_party/blink/renderer/core/testing/sim/sim_compositor.h
,
Nov 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b04f16c9469b177395888db388d55c7f83a1bd47 commit b04f16c9469b177395888db388d55c7f83a1bd47 Author: Scott Violet <sky@chromium.org> Date: Tue Nov 27 00:48:46 2018 Revert "Start gpu channel and compositor mojo pipe collection eagerly" This reverts commit 66794d00db74fff08d3c7944bab7337494beb544. Reason for revert: This breaks mash_browsertests. See https://crbug.com/908623 Original change's description: > Start gpu channel and compositor mojo pipe collection eagerly > > When a RenderWidget is frozen, its compositor is stopped. However when > the main frame is being navigated, we want to start requesting mojo > pipes for the gpu and display compositor immediately so that it can > happen in parallel with the navigation loading. > > Previously, we just always did this when creating a RenderWidget, and > left them active when freezing the RenderWidget. After 678f025f7cde75 > however, we don't, and this causes time-to-first-pixels regressions > on navigation. > > So we add a WarmupCompositor() method to RenderWidget, and call this > from RenderFrameImpl::CreateFrame() when it makes the provisional > frame, and the WebFrameWidget, since we expect to make use of the > main frame RenderWidget shortly. > > Then, if RenderFrameImpl::FrameDetached() occurs, due to the > navigation failing, we will AbortWarmupCompositor() to drop the > mojo pipes instead of holding onto them indefinitely. > > This recovers the loading regressions introduced, while also not > allocating mojo channels for frozen RenderWidgets indefinitely. > > In order to do this reasonably, we drop the "callback" from the > request to RenderThreadImpl::RequestNewLayerTreeFrameSink so that > it always returns something immediately. This way RenderWidget > does not need to worry about having a task run to collect the > new frame sink, and ordering with tasks from the compositor to > collect it. > > R=piman@chromium.org > > Change-Id: I6007e26e1622006652ce3619cdd080539d11e7b0 > Bug: 905191 > Reviewed-on: https://chromium-review.googlesource.com/c/1341073 > Commit-Queue: danakj <danakj@chromium.org> > Reviewed-by: danakj <danakj@chromium.org> > Reviewed-by: Daniel Cheng <dcheng@chromium.org> > Reviewed-by: Antoine Labour <piman@chromium.org> > Cr-Commit-Position: refs/heads/master@{#610909} TBR=danakj@chromium.org,dcheng@chromium.org,sergeyu@chromium.org,piman@chromium.org Change-Id: Ifadae837dd5dbd8c1c31212704c6ee88843bd0d1 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 905191 Reviewed-on: https://chromium-review.googlesource.com/c/1351808 Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#610982} [modify] https://crrev.com/b04f16c9469b177395888db388d55c7f83a1bd47/content/renderer/gpu/layer_tree_view.cc [modify] https://crrev.com/b04f16c9469b177395888db388d55c7f83a1bd47/content/renderer/gpu/layer_tree_view_delegate.h [modify] https://crrev.com/b04f16c9469b177395888db388d55c7f83a1bd47/content/renderer/gpu/layer_tree_view_unittest.cc [modify] https://crrev.com/b04f16c9469b177395888db388d55c7f83a1bd47/content/renderer/mus/renderer_window_tree_client.cc [modify] https://crrev.com/b04f16c9469b177395888db388d55c7f83a1bd47/content/renderer/mus/renderer_window_tree_client.h [modify] https://crrev.com/b04f16c9469b177395888db388d55c7f83a1bd47/content/renderer/render_frame_impl.cc [modify] https://crrev.com/b04f16c9469b177395888db388d55c7f83a1bd47/content/renderer/render_thread_impl.cc [modify] https://crrev.com/b04f16c9469b177395888db388d55c7f83a1bd47/content/renderer/render_thread_impl.h [modify] https://crrev.com/b04f16c9469b177395888db388d55c7f83a1bd47/content/renderer/render_widget.cc [modify] https://crrev.com/b04f16c9469b177395888db388d55c7f83a1bd47/content/renderer/render_widget.h [modify] https://crrev.com/b04f16c9469b177395888db388d55c7f83a1bd47/content/test/stub_layer_tree_view_delegate.cc [modify] https://crrev.com/b04f16c9469b177395888db388d55c7f83a1bd47/content/test/stub_layer_tree_view_delegate.h [modify] https://crrev.com/b04f16c9469b177395888db388d55c7f83a1bd47/third_party/blink/renderer/core/testing/sim/sim_compositor.cc [modify] https://crrev.com/b04f16c9469b177395888db388d55c7f83a1bd47/third_party/blink/renderer/core/testing/sim/sim_compositor.h
,
Nov 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eacb6ed3531606cf878fdcd635fc251c686e30b2 commit eacb6ed3531606cf878fdcd635fc251c686e30b2 Author: danakj <danakj@chromium.org> Date: Wed Nov 28 20:07:14 2018 Reland "Start gpu channel and compositor mojo pipe collection eagerly" This is a reland of 66794d00db74fff08d3c7944bab7337494beb544 In this reland we revert changes to RenderThreadImpl and mus to make LayerTreeFrameSink creation asynchronous again. Then in RenderWidget we must remember that a warmup is underway, we do so with a bool and a WeakPtrFactory. If it is aborted, we reset the bool and invalidate the WeakPtrFactory. If a request for a frame sink beats the warmup completing, we save the callback and run that instead of saving the frame sink on RenderWidget when the warmup completes. This ignores some weird weird corner cases like where warmup is aborted then we start another warmup or unfreeze the widget or something and will just make redundant requests. Since these are so rare they are not worth adding complexity for. Original change's description: > Start gpu channel and compositor mojo pipe collection eagerly > > When a RenderWidget is frozen, its compositor is stopped. However when > the main frame is being navigated, we want to start requesting mojo > pipes for the gpu and display compositor immediately so that it can > happen in parallel with the navigation loading. > > Previously, we just always did this when creating a RenderWidget, and > left them active when freezing the RenderWidget. After 678f025f7cde75 > however, we don't, and this causes time-to-first-pixels regressions > on navigation. > > So we add a WarmupCompositor() method to RenderWidget, and call this > from RenderFrameImpl::CreateFrame() when it makes the provisional > frame, and the WebFrameWidget, since we expect to make use of the > main frame RenderWidget shortly. > > Then, if RenderFrameImpl::FrameDetached() occurs, due to the > navigation failing, we will AbortWarmupCompositor() to drop the > mojo pipes instead of holding onto them indefinitely. > > This recovers the loading regressions introduced, while also not > allocating mojo channels for frozen RenderWidgets indefinitely. > > In order to do this reasonably, we drop the "callback" from the > request to RenderThreadImpl::RequestNewLayerTreeFrameSink so that > it always returns something immediately. This way RenderWidget > does not need to worry about having a task run to collect the > new frame sink, and ordering with tasks from the compositor to > collect it. > > R=piman@chromium.org > > Change-Id: I6007e26e1622006652ce3619cdd080539d11e7b0 > Bug: 905191 > Reviewed-on: https://chromium-review.googlesource.com/c/1341073 > Commit-Queue: danakj <danakj@chromium.org> > Reviewed-by: danakj <danakj@chromium.org> > Reviewed-by: Daniel Cheng <dcheng@chromium.org> > Reviewed-by: Antoine Labour <piman@chromium.org> > Cr-Commit-Position: refs/heads/master@{#610909} Bug: 905191 Change-Id: I6cb65e08a832a969156b96bece0f2e12eccd5b60 Reviewed-on: https://chromium-review.googlesource.com/c/1351938 Commit-Queue: danakj <danakj@chromium.org> Reviewed-by: danakj <danakj@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Antoine Labour <piman@chromium.org> Cr-Commit-Position: refs/heads/master@{#611838} [modify] https://crrev.com/eacb6ed3531606cf878fdcd635fc251c686e30b2/content/renderer/render_frame_impl.cc [modify] https://crrev.com/eacb6ed3531606cf878fdcd635fc251c686e30b2/content/renderer/render_thread_impl.cc [modify] https://crrev.com/eacb6ed3531606cf878fdcd635fc251c686e30b2/content/renderer/render_thread_impl.h [modify] https://crrev.com/eacb6ed3531606cf878fdcd635fc251c686e30b2/content/renderer/render_widget.cc [modify] https://crrev.com/eacb6ed3531606cf878fdcd635fc251c686e30b2/content/renderer/render_widget.h
,
Nov 29
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/20230dcf4bf803efc81ef997d101777a81f6c90a commit 20230dcf4bf803efc81ef997d101777a81f6c90a Author: Giovanni Ortuño Urquidi <ortuno@chromium.org> Date: Thu Nov 29 04:17:19 2018 Revert "Reland "Start gpu channel and compositor mojo pipe collection eagerly"" This reverts commit eacb6ed3531606cf878fdcd635fc251c686e30b2. Reason for revert: Makes ChromeVoxLiveRegionsUnitTests flaky: https://crbug.com/909973 https://crbug.com/909984 https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/linux-chromeos-rel/16620 [ RUN ] ChromeVoxLiveRegionsUnitTest.AddToLiveRegion [23526:23526:1128/180604.087820:WARNING:chrome_browser_main_chromeos.cc(544)] Running as stub user with profile dir: test-user [23526:23526:1128/180604.176618:WARNING:easy_unlock_service_regular.cc(523)] EasyUnlockServiceRegular::IsInLegacyHostMode: DeviceSyncClient not ready. Returning false. [23526:23526:1128/180604.197349:INFO:remote_commands_service.cc(38)] Fetching remote commands. [23526:23526:1128/180604.222292:WARNING:remote_commands_service.cc(40)] Client is not registered. [23526:23526:1128/180604.222531:INFO:remote_commands_invalidator.cc(32)] Initialize RemoteCommandsInvalidator. [23526:23526:1128/180604.222602:INFO:remote_commands_invalidator.cc(57)] Starting RemoteCommandsInvalidator. [23526:23526:1128/180604.222654:INFO:remote_commands_invalidator.cc(123)] RemoteCommandsInvalidator ReloadPolicyData. [23526:23526:1128/180604.222699:INFO:remote_commands_invalidator.cc(167)] Unregister RemoteCommandsInvalidator. [23526:23526:1128/180604.245434:WARNING:wallpaper_controller_client.cc(358)] Cannot get wallpaper files id in RemovePolicyWallpaper. This should never happen under normal circumstances. [23526:23526:1128/180604.326719:ERROR:gpu_interface_provider.cc(87)] Not implemented reached in virtual void content::GpuInterfaceProvider::RegisterOzoneGpuInterfaces(service_manager::BinderRegistry *) [23526:23526:1128/180604.607132:INFO:CONSOLE(1281)] "Running TestCase ChromeVoxLiveRegionsUnitTest.AddToLiveRegion", source: file:///b/s/w/ir/chrome/test/data/webui/test_api.js (1281) [23526:23526:1128/180604.617409:INFO:CONSOLE(53)] "done setup", source: file:///b/s/w/ir/chrome/browser/resources/chromeos/chromevox/testing/tester.js (53) [23526:23526:1128/180604.636340:ERROR:web_ui_test_handler.cc(100)] Failed: RUN_TEST_F("ChromeVoxLiveRegionsUnitTest","AddToLiveRegion") AssertionError: expected undefined to equal 'Eric' at Function.assert.strictEqual (file:///b/s/w/ir/third_party/chaijs/chai.js:2277:32) at assertEquals (file:///b/s/w/ir/chrome/test/data/webui/test_api.js:923:15) at ChromeVoxLiveRegionsUnitTest.<anonymous> (file:///b/s/w/ir/out/Release/test_data/chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions_test.unitjs:157:7) at ChromeVoxLiveRegionsUnitTest.<anonymous> (file:///b/s/w/ir/chrome/browser/resources/chromeos/chromevox/testing/chromevox_unittest_base.js:123:12) at CallbackHelper.<anonymous> (file:///b/s/w/ir/chrome/browser/resources/chromeos/chromevox/testing/callback_helper.js:32:16) at CallFunctionAction.invoke (file:///b/s/w/ir/chrome/test/data/webui/test_api.js:1430:22) at RunAllAction.invoke (file:///b/s/w/ir/chrome/test/data/webui/test_api.js:1591:35) at file:///b/s/w/ir/chrome/browser/resources/chromeos/chromevox/testing/callback_helper.js:42:14 at file:///b/s/w/ir/chrome/browser/resources/chromeos/chromevox/chromevox/injected/event_watcher.js:319:59 gen/chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions_test-gen.cc:1442: Failure Value of: RunJavascriptTestF( true, "ChromeVoxLiveRegionsUnitTest", "AddToLiveRegion") Actual: false Expected: true Original change's description: > Reland "Start gpu channel and compositor mojo pipe collection eagerly" > > This is a reland of 66794d00db74fff08d3c7944bab7337494beb544 > > In this reland we revert changes to RenderThreadImpl and mus to make > LayerTreeFrameSink creation asynchronous again. > > Then in RenderWidget we must remember that a warmup is underway, we do > so with a bool and a WeakPtrFactory. If it is aborted, we reset the bool > and invalidate the WeakPtrFactory. > > If a request for a frame sink beats the warmup completing, we save > the callback and run that instead of saving the frame sink on > RenderWidget when the warmup completes. > > This ignores some weird weird corner cases like where warmup is > aborted then we start another warmup or unfreeze the widget or something > and will just make redundant requests. Since these are so rare they > are not worth adding complexity for. > > Original change's description: > > Start gpu channel and compositor mojo pipe collection eagerly > > > > When a RenderWidget is frozen, its compositor is stopped. However when > > the main frame is being navigated, we want to start requesting mojo > > pipes for the gpu and display compositor immediately so that it can > > happen in parallel with the navigation loading. > > > > Previously, we just always did this when creating a RenderWidget, and > > left them active when freezing the RenderWidget. After 678f025f7cde75 > > however, we don't, and this causes time-to-first-pixels regressions > > on navigation. > > > > So we add a WarmupCompositor() method to RenderWidget, and call this > > from RenderFrameImpl::CreateFrame() when it makes the provisional > > frame, and the WebFrameWidget, since we expect to make use of the > > main frame RenderWidget shortly. > > > > Then, if RenderFrameImpl::FrameDetached() occurs, due to the > > navigation failing, we will AbortWarmupCompositor() to drop the > > mojo pipes instead of holding onto them indefinitely. > > > > This recovers the loading regressions introduced, while also not > > allocating mojo channels for frozen RenderWidgets indefinitely. > > > > In order to do this reasonably, we drop the "callback" from the > > request to RenderThreadImpl::RequestNewLayerTreeFrameSink so that > > it always returns something immediately. This way RenderWidget > > does not need to worry about having a task run to collect the > > new frame sink, and ordering with tasks from the compositor to > > collect it. > > > > R=piman@chromium.org > > > > Change-Id: I6007e26e1622006652ce3619cdd080539d11e7b0 > > Bug: 905191 > > Reviewed-on: https://chromium-review.googlesource.com/c/1341073 > > Commit-Queue: danakj <danakj@chromium.org> > > Reviewed-by: danakj <danakj@chromium.org> > > Reviewed-by: Daniel Cheng <dcheng@chromium.org> > > Reviewed-by: Antoine Labour <piman@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#610909} > > Bug: 905191 > Change-Id: I6cb65e08a832a969156b96bece0f2e12eccd5b60 > Reviewed-on: https://chromium-review.googlesource.com/c/1351938 > Commit-Queue: danakj <danakj@chromium.org> > Reviewed-by: danakj <danakj@chromium.org> > Reviewed-by: Daniel Cheng <dcheng@chromium.org> > Reviewed-by: Antoine Labour <piman@chromium.org> > Cr-Commit-Position: refs/heads/master@{#611838} TBR=danakj@chromium.org,dcheng@chromium.org,sergeyu@chromium.org,piman@chromium.org Change-Id: I68e348f7fac337580bf3f79a29f618ac3ce3c6d6 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 905191 Reviewed-on: https://chromium-review.googlesource.com/c/1354725 Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org> Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org> Cr-Commit-Position: refs/heads/master@{#612035} [modify] https://crrev.com/20230dcf4bf803efc81ef997d101777a81f6c90a/content/renderer/render_frame_impl.cc [modify] https://crrev.com/20230dcf4bf803efc81ef997d101777a81f6c90a/content/renderer/render_thread_impl.cc [modify] https://crrev.com/20230dcf4bf803efc81ef997d101777a81f6c90a/content/renderer/render_thread_impl.h [modify] https://crrev.com/20230dcf4bf803efc81ef997d101777a81f6c90a/content/renderer/render_widget.cc [modify] https://crrev.com/20230dcf4bf803efc81ef997d101777a81f6c90a/content/renderer/render_widget.h
,
Nov 29
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/be3c24b48af79f32b685aa7597c1f99e1302d27d commit be3c24b48af79f32b685aa7597c1f99e1302d27d Author: danakj <danakj@chromium.org> Date: Thu Nov 29 17:53:09 2018 Reland "Reland "Start gpu channel and compositor mojo pipe collection eagerly"" This is a reland of eacb6ed3531606cf878fdcd635fc251c686e30b2 The flaky test has been disabled. TBR=piman Original change's description: > Reland "Start gpu channel and compositor mojo pipe collection eagerly" > > This is a reland of 66794d00db74fff08d3c7944bab7337494beb544 > > In this reland we revert changes to RenderThreadImpl and mus to make > LayerTreeFrameSink creation asynchronous again. > > Then in RenderWidget we must remember that a warmup is underway, we do > so with a bool and a WeakPtrFactory. If it is aborted, we reset the bool > and invalidate the WeakPtrFactory. > > If a request for a frame sink beats the warmup completing, we save > the callback and run that instead of saving the frame sink on > RenderWidget when the warmup completes. > > This ignores some weird weird corner cases like where warmup is > aborted then we start another warmup or unfreeze the widget or something > and will just make redundant requests. Since these are so rare they > are not worth adding complexity for. > > Original change's description: > > Start gpu channel and compositor mojo pipe collection eagerly > > > > When a RenderWidget is frozen, its compositor is stopped. However when > > the main frame is being navigated, we want to start requesting mojo > > pipes for the gpu and display compositor immediately so that it can > > happen in parallel with the navigation loading. > > > > Previously, we just always did this when creating a RenderWidget, and > > left them active when freezing the RenderWidget. After 678f025f7cde75 > > however, we don't, and this causes time-to-first-pixels regressions > > on navigation. > > > > So we add a WarmupCompositor() method to RenderWidget, and call this > > from RenderFrameImpl::CreateFrame() when it makes the provisional > > frame, and the WebFrameWidget, since we expect to make use of the > > main frame RenderWidget shortly. > > > > Then, if RenderFrameImpl::FrameDetached() occurs, due to the > > navigation failing, we will AbortWarmupCompositor() to drop the > > mojo pipes instead of holding onto them indefinitely. > > > > This recovers the loading regressions introduced, while also not > > allocating mojo channels for frozen RenderWidgets indefinitely. > > > > In order to do this reasonably, we drop the "callback" from the > > request to RenderThreadImpl::RequestNewLayerTreeFrameSink so that > > it always returns something immediately. This way RenderWidget > > does not need to worry about having a task run to collect the > > new frame sink, and ordering with tasks from the compositor to > > collect it. > > > > R=piman@chromium.org > > > > Change-Id: I6007e26e1622006652ce3619cdd080539d11e7b0 > > Bug: 905191 > > Reviewed-on: https://chromium-review.googlesource.com/c/1341073 > > Commit-Queue: danakj <danakj@chromium.org> > > Reviewed-by: danakj <danakj@chromium.org> > > Reviewed-by: Daniel Cheng <dcheng@chromium.org> > > Reviewed-by: Antoine Labour <piman@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#610909} > > Bug: 905191 > Change-Id: I6cb65e08a832a969156b96bece0f2e12eccd5b60 > Reviewed-on: https://chromium-review.googlesource.com/c/1351938 > Commit-Queue: danakj <danakj@chromium.org> > Reviewed-by: danakj <danakj@chromium.org> > Reviewed-by: Daniel Cheng <dcheng@chromium.org> > Reviewed-by: Antoine Labour <piman@chromium.org> > Cr-Commit-Position: refs/heads/master@{#611838} Bug: 905191 Change-Id: I0320c8604d50d5d83157c058c230b471c0898b26 Reviewed-on: https://chromium-review.googlesource.com/c/1355321 Reviewed-by: danakj <danakj@chromium.org> Commit-Queue: danakj <danakj@chromium.org> Cr-Commit-Position: refs/heads/master@{#612253} [modify] https://crrev.com/be3c24b48af79f32b685aa7597c1f99e1302d27d/content/renderer/render_frame_impl.cc [modify] https://crrev.com/be3c24b48af79f32b685aa7597c1f99e1302d27d/content/renderer/render_thread_impl.cc [modify] https://crrev.com/be3c24b48af79f32b685aa7597c1f99e1302d27d/content/renderer/render_thread_impl.h [modify] https://crrev.com/be3c24b48af79f32b685aa7597c1f99e1302d27d/content/renderer/render_widget.cc [modify] https://crrev.com/be3c24b48af79f32b685aa7597c1f99e1302d27d/content/renderer/render_widget.h
,
Nov 29
It seems like these tests started flaking more/again: ChromeVoxLiveRegionsUnitTest.RevealAlertLiveRegion ChromeVoxLiveRegionsUnitTest.ProgressBarLiveRegionEvents https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/linux-chromeos-rel/16657 https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/linux-chromeos-rel/16658 https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/linux-chromeos-rel/16659 https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=RevealAlertLiveRegion https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=ProgressBarLiveRegionEvents Please take a look and address here or reopen Issue 908602 , Issue 908641 , Issue 908643 , or Issue 908652
,
Nov 29
The whole test suite seems to be flaky, we should disable them.
,
Nov 29
,
Dec 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d38eb8e01e09dc05802b062bf159f86942fce09e commit d38eb8e01e09dc05802b062bf159f86942fce09e Author: Guido Urdaneta <guidou@chromium.org> Date: Mon Dec 03 11:17:56 2018 Revert "Reland "Reland "Start gpu channel and compositor mojo pipe collection eagerly""" This reverts commit be3c24b48af79f32b685aa7597c1f99e1302d27d. Reason for revert: FindIt has detected it as culprit for at least two tests that started flaking. The FindIt analysis looks correct, but please take a look at the bugs and reland if you think this should reland. See crbug.com/910997 and crbug.com/910921 Original change's description: > Reland "Reland "Start gpu channel and compositor mojo pipe collection eagerly"" > > This is a reland of eacb6ed3531606cf878fdcd635fc251c686e30b2 > > The flaky test has been disabled. > > TBR=piman > > Original change's description: > > Reland "Start gpu channel and compositor mojo pipe collection eagerly" > > > > This is a reland of 66794d00db74fff08d3c7944bab7337494beb544 > > > > In this reland we revert changes to RenderThreadImpl and mus to make > > LayerTreeFrameSink creation asynchronous again. > > > > Then in RenderWidget we must remember that a warmup is underway, we do > > so with a bool and a WeakPtrFactory. If it is aborted, we reset the bool > > and invalidate the WeakPtrFactory. > > > > If a request for a frame sink beats the warmup completing, we save > > the callback and run that instead of saving the frame sink on > > RenderWidget when the warmup completes. > > > > This ignores some weird weird corner cases like where warmup is > > aborted then we start another warmup or unfreeze the widget or something > > and will just make redundant requests. Since these are so rare they > > are not worth adding complexity for. > > > > Original change's description: > > > Start gpu channel and compositor mojo pipe collection eagerly > > > > > > When a RenderWidget is frozen, its compositor is stopped. However when > > > the main frame is being navigated, we want to start requesting mojo > > > pipes for the gpu and display compositor immediately so that it can > > > happen in parallel with the navigation loading. > > > > > > Previously, we just always did this when creating a RenderWidget, and > > > left them active when freezing the RenderWidget. After 678f025f7cde75 > > > however, we don't, and this causes time-to-first-pixels regressions > > > on navigation. > > > > > > So we add a WarmupCompositor() method to RenderWidget, and call this > > > from RenderFrameImpl::CreateFrame() when it makes the provisional > > > frame, and the WebFrameWidget, since we expect to make use of the > > > main frame RenderWidget shortly. > > > > > > Then, if RenderFrameImpl::FrameDetached() occurs, due to the > > > navigation failing, we will AbortWarmupCompositor() to drop the > > > mojo pipes instead of holding onto them indefinitely. > > > > > > This recovers the loading regressions introduced, while also not > > > allocating mojo channels for frozen RenderWidgets indefinitely. > > > > > > In order to do this reasonably, we drop the "callback" from the > > > request to RenderThreadImpl::RequestNewLayerTreeFrameSink so that > > > it always returns something immediately. This way RenderWidget > > > does not need to worry about having a task run to collect the > > > new frame sink, and ordering with tasks from the compositor to > > > collect it. > > > > > > R=piman@chromium.org > > > > > > Change-Id: I6007e26e1622006652ce3619cdd080539d11e7b0 > > > Bug: 905191 > > > Reviewed-on: https://chromium-review.googlesource.com/c/1341073 > > > Commit-Queue: danakj <danakj@chromium.org> > > > Reviewed-by: danakj <danakj@chromium.org> > > > Reviewed-by: Daniel Cheng <dcheng@chromium.org> > > > Reviewed-by: Antoine Labour <piman@chromium.org> > > > Cr-Commit-Position: refs/heads/master@{#610909} > > > > Bug: 905191 > > Change-Id: I6cb65e08a832a969156b96bece0f2e12eccd5b60 > > Reviewed-on: https://chromium-review.googlesource.com/c/1351938 > > Commit-Queue: danakj <danakj@chromium.org> > > Reviewed-by: danakj <danakj@chromium.org> > > Reviewed-by: Daniel Cheng <dcheng@chromium.org> > > Reviewed-by: Antoine Labour <piman@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#611838} > > Bug: 905191 > Change-Id: I0320c8604d50d5d83157c058c230b471c0898b26 > Reviewed-on: https://chromium-review.googlesource.com/c/1355321 > Reviewed-by: danakj <danakj@chromium.org> > Commit-Queue: danakj <danakj@chromium.org> > Cr-Commit-Position: refs/heads/master@{#612253} TBR=danakj@chromium.org,dcheng@chromium.org,sergeyu@chromium.org,piman@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 905191 Change-Id: Ibf041a7a02c80df0f0f4a1a02803846fa443c40a Reviewed-on: https://chromium-review.googlesource.com/c/1356810 Reviewed-by: Guido Urdaneta <guidou@chromium.org> Commit-Queue: Guido Urdaneta <guidou@chromium.org> Cr-Commit-Position: refs/heads/master@{#613041} [modify] https://crrev.com/d38eb8e01e09dc05802b062bf159f86942fce09e/content/renderer/render_frame_impl.cc [modify] https://crrev.com/d38eb8e01e09dc05802b062bf159f86942fce09e/content/renderer/render_thread_impl.cc [modify] https://crrev.com/d38eb8e01e09dc05802b062bf159f86942fce09e/content/renderer/render_thread_impl.h [modify] https://crrev.com/d38eb8e01e09dc05802b062bf159f86942fce09e/content/renderer/render_widget.cc [modify] https://crrev.com/d38eb8e01e09dc05802b062bf159f86942fce09e/content/renderer/render_widget.h
,
Dec 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f8746079c0456d3b4091e7d06fd05856691a5bb7 commit f8746079c0456d3b4091e7d06fd05856691a5bb7 Author: danakj <danakj@chromium.org> Date: Tue Dec 04 20:14:22 2018 Reland again "Start gpu channel and compositor mojo pipe collection eagerly" R=piman@chromium.org TBR=piman SHERIFFS: This changes timing in the renderer, if (probably ChromeVOX) tests get flaky they should be disabled please instead of reverting this. The flaky tests causing the last iteration of revert were disabled in https://crrev.com/c/1361585. This reverts commit d38eb8e01e09dc05802b062bf159f86942fce09e. Original change's description: > Start gpu channel and compositor mojo pipe collection eagerly > > When a RenderWidget is frozen, its compositor is stopped. However when > the main frame is being navigated, we want to start requesting mojo > pipes for the gpu and display compositor immediately so that it can > happen in parallel with the navigation loading. > > Previously, we just always did this when creating a RenderWidget, and > left them active when freezing the RenderWidget. After 678f025f7cde75 > however, we don't, and this causes time-to-first-pixels regressions > on navigation. > > So we add a WarmupCompositor() method to RenderWidget, and call this > from RenderFrameImpl::CreateFrame() when it makes the provisional > frame, and the WebFrameWidget, since we expect to make use of the > main frame RenderWidget shortly. > > Then, if RenderFrameImpl::FrameDetached() occurs, due to the > navigation failing, we will AbortWarmupCompositor() to drop the > mojo pipes instead of holding onto them indefinitely. > > This recovers the loading regressions introduced, while also not > allocating mojo channels for frozen RenderWidgets indefinitely. > > In order to do this reasonably, we drop the "callback" from the > request to RenderThreadImpl::RequestNewLayerTreeFrameSink so that > it always returns something immediately. This way RenderWidget > does not need to worry about having a task run to collect the > new frame sink, and ordering with tasks from the compositor to > collect it. > > R=piman@chromium.org > > Change-Id: I6007e26e1622006652ce3619cdd080539d11e7b0 > Bug: 905191 > Reviewed-on: https://chromium-review.googlesource.com/c/1341073 > Commit-Queue: danakj <danakj@chromium.org> > Reviewed-by: danakj <danakj@chromium.org> > Reviewed-by: Daniel Cheng <dcheng@chromium.org> > Reviewed-by: Antoine Labour <piman@chromium.org> > Cr-Commit-Position: refs/heads/master@{#610909} Change-Id: I100c65758b673a27f7d90bdbd6369abf8e8c608f Reviewed-on: https://chromium-review.googlesource.com/c/1361598 Reviewed-by: danakj <danakj@chromium.org> Commit-Queue: danakj <danakj@chromium.org> Cr-Commit-Position: refs/heads/master@{#613670} [modify] https://crrev.com/f8746079c0456d3b4091e7d06fd05856691a5bb7/content/renderer/render_frame_impl.cc [modify] https://crrev.com/f8746079c0456d3b4091e7d06fd05856691a5bb7/content/renderer/render_thread_impl.cc [modify] https://crrev.com/f8746079c0456d3b4091e7d06fd05856691a5bb7/content/renderer/render_thread_impl.h [modify] https://crrev.com/f8746079c0456d3b4091e7d06fd05856691a5bb7/content/renderer/render_widget.cc [modify] https://crrev.com/f8746079c0456d3b4091e7d06fd05856691a5bb7/content/renderer/render_widget.h
,
Dec 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e29cb53e877c9a0f0c5be20e3cf23616fe800b82 commit e29cb53e877c9a0f0c5be20e3cf23616fe800b82 Author: danakj <danakj@chromium.org> Date: Tue Dec 04 21:10:23 2018 Comment improvement in RenderFrameHost. SHERIFFS: This is really a followup to https://crrev.com/c/1361598 to prevent it from being auto-reverted. That CL changes timing in the renderer so flaky tests have their timing changed and may become more (or less) flaky as a result. Such tests should be disabled instead of reverting that change (again). Thanks :) R=dcheng@chromium.org Change-Id: I987c6721b1efe7598582522707a8a984bb42901a Bug: 905191 Reviewed-on: https://chromium-review.googlesource.com/c/1361653 Reviewed-by: Antoine Labour <piman@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Commit-Queue: danakj <danakj@chromium.org> Cr-Commit-Position: refs/heads/master@{#613689} [modify] https://crrev.com/e29cb53e877c9a0f0c5be20e3cf23616fe800b82/content/renderer/render_frame_impl.cc
,
Dec 5
This stayed landed for 20 hours so that's pretty wonderful.. Perf bot is very jumpy now but is hitting back pre-regression levels often. I'm not sure why it's less consistent than before, we're making the CompositorFrameSink where we used to. The other thing we used to do is since compositor was visible, we'd send SetNeedsBeginFrames to the display before we had navigated and had a frame available. It could be that round trip for a BeginFrame gets delayed sometimes causing the jumps. In which case it is woes of unified begin frame. Going to mark fixed for now and watch the bot a bit more.
,
Dec 5
We should merge this to M72 to avoid loading regression. It first landed as #610909 but branch was at #610746.
,
Dec 5
Pls apply appropriate OSs label.
,
Dec 5
,
Dec 5
This bug requires manual review: Reverts referenced in bugdroid comments after merge request. Please contact the milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 5
Which cl you're requesting a merge for? Is the change well baked/verified in canary and safe to merge?
,
Dec 5
The last one, it's been in canary a few times and reverted due to (already) flaky tests being exposed by it. The perf bots verify the improvement, there's nothing human visible to verify in Canary.
,
Dec 5
Sorry, by last one I mean from #41
,
Dec 5
Approving merge for cl listed at #41 to M72 branch 3626 based on comments #49. Please merge ASAP. Thank you.
,
Dec 5
It looks like 3626 has a different branch point than the last dev branch did, at #612437 which already includes a reland of this CL. So there's nothing to do if the branch point isn't going to keep moving. I don't think I've ever seen this happen before, so please confirm this is intended?
,
Dec 6
Last week dev release was before M72 branch, this week dev which went out today is from M72 branch version 72.0.3626.7. M72 was branched (3626) at chromium revision 612437. So CL listed at #41 is not in branch. So it will need a merge. Also is cl listed at #42 need a merge to M72?
,
Dec 6
|
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Nov 14