Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
257773009:
cc: Change required_for_activation bookkeeping. (Closed)
|
Created:
6 years, 8 months ago by vmpstr Modified:
6 years, 7 months ago CC:
chromium-reviews, cc-bugs_chromium.org, Ken Russell (switch to Gerrit), bajones, Zhenyao Mo, alokp Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptioncc: Change required_for_activation bookkeeping.
This patch changes the bookkeeping of whether required for activation
tiles all received memory during AssignGpuMemory. The new approach
doesn't rely on the fact that we process every tile in that loop, thus
allowing us to early out or use another approach at getting tiles that
does not guarantee that every tile will be visited.
The end goal is to evaluate if we can activate when the rasterizer
notifies us that all tasks given to it that are required for
activation are finished rather than trying to figure this out
when we schedule tasks.
R=reveman, enne
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267400
Patch Set 1 #Patch Set 2 : test fixes #
Total comments: 7
Patch Set 3 : review #
Total comments: 4
Patch Set 4 : review #
Total comments: 2
Patch Set 5 : #
Total comments: 10
Patch Set 6 : rebase + update #
Total comments: 1
Patch Set 7 : #Patch Set 8 : rebase #Patch Set 9 : rebase #
Total comments: 4
Messages
Total messages: 28 (0 generated)
Please take a look
On 2014/04/25 17:35:06, vmpstr wrote: > Please take a look Also, bookkeeping is one of few words that has 3 consecutive double letters.
Might be worth explaining in the description that the end goal is to evaluate if we can activate when the rasterizer notifies us that all tasks given to it that are required for activation are finished rather than trying to figure this out when we schedule tasks. https://codereview.chromium.org/257773009/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/257773009/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:142: size_t RequiredForActivationTileNeedsRasterCount() const { TilesRequiredForActivationThatNeedsRasterCount? https://codereview.chromium.org/257773009/diff/20001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/257773009/diff/20001/cc/resources/tile_manage... cc/resources/tile_manager.cc:679: !tile->required_for_activation()) { hm, is this really correct? should we not be able to release tiles required for activation if we have something more important that needs memory? https://codereview.chromium.org/257773009/diff/20001/cc/test/fake_tile_manage... File cc/test/fake_tile_manager_client.h (right): https://codereview.chromium.org/257773009/diff/20001/cc/test/fake_tile_manage... cc/test/fake_tile_manager_client.h:12: class Tile; nit: blank line before "class FakeTileManagerClient"?
https://codereview.chromium.org/257773009/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/257773009/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:142: size_t RequiredForActivationTileNeedsRasterCount() const { On 2014/04/25 20:44:08, reveman wrote: > TilesRequiredForActivationThatNeedsRasterCount? Done. https://codereview.chromium.org/257773009/diff/20001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/257773009/diff/20001/cc/resources/tile_manage... cc/resources/tile_manager.cc:679: !tile->required_for_activation()) { On 2014/04/25 20:44:08, reveman wrote: > hm, is this really correct? should we not be able to release tiles required for > activation if we have something more important that needs memory? Yeah that's true, but the problem is that with this patch we have to account for these tiles as processed during AssignGpuMemory loop. If we never put the tiles into the prioritized tile set, then we will erroneously think that we didn't schedule all required for activation tiles. Note that this check still exists around lines 862-867 https://codereview.chromium.org/257773009/diff/20001/cc/test/fake_tile_manage... File cc/test/fake_tile_manager_client.h (right): https://codereview.chromium.org/257773009/diff/20001/cc/test/fake_tile_manage... cc/test/fake_tile_manager_client.h:12: class Tile; On 2014/04/25 20:44:08, reveman wrote: > nit: blank line before "class FakeTileManagerClient"? Done.
PTAL.
https://codereview.chromium.org/257773009/diff/20001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/257773009/diff/20001/cc/resources/tile_manage... cc/resources/tile_manager.cc:679: !tile->required_for_activation()) { On 2014/04/25 22:09:52, vmpstr wrote: > On 2014/04/25 20:44:08, reveman wrote: > > hm, is this really correct? should we not be able to release tiles required > for > > activation if we have something more important that needs memory? > > Yeah that's true, but the problem is that with this patch we have to account for > these tiles as processed during AssignGpuMemory loop. If we never put the tiles > into the prioritized tile set, then we will erroneously think that we didn't > schedule all required for activation tiles. > > Note that this check still exists around lines 862-867 Ok, thanks for explaining. https://codereview.chromium.org/257773009/diff/40001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/257773009/diff/40001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:147: size_t TilesRequiredForActivationThatNeedsRasterCount() const { Sorry, term "Initialized" or "ReadyToDraw" is probably better here than "Raster" as we use those already. ie. NotifyTileInitialized. UninitializedTilesRequiredForActivationCount? https://codereview.chromium.org/257773009/diff/40001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/257773009/diff/40001/cc/resources/tile_manage... cc/resources/tile_manager.cc:967: required_for_activation_processed_count == nit: maybe have the name of these match better. ie. processed_required_for_activation_tile_count and total_required_for_activation_tile_count?
PTAL https://codereview.chromium.org/257773009/diff/40001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/257773009/diff/40001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:147: size_t TilesRequiredForActivationThatNeedsRasterCount() const { On 2014/04/25 22:23:59, reveman wrote: > Sorry, term "Initialized" or "ReadyToDraw" is probably better here than "Raster" > as we use those already. ie. NotifyTileInitialized. > > UninitializedTilesRequiredForActivationCount? Makes sense. Done. https://codereview.chromium.org/257773009/diff/40001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/257773009/diff/40001/cc/resources/tile_manage... cc/resources/tile_manager.cc:967: required_for_activation_processed_count == On 2014/04/25 22:23:59, reveman wrote: > nit: maybe have the name of these match better. ie. > processed_required_for_activation_tile_count and > total_required_for_activation_tile_count? Done.
lgtm % enne's review https://codereview.chromium.org/257773009/diff/50001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/257773009/diff/50001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:232: size_t unitialized_tiles_required_for_activation_count_; uninitialized_
https://codereview.chromium.org/257773009/diff/50001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/257773009/diff/50001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:232: size_t unitialized_tiles_required_for_activation_count_; On 2014/04/25 23:39:53, reveman wrote: > uninitialized_ Gah, I knew I'd mess up somewhere. Fixed.
On 2014/04/25 23:42:10, vmpstr wrote: > https://codereview.chromium.org/257773009/diff/50001/cc/layers/picture_layer_... > File cc/layers/picture_layer_impl.h (right): > > https://codereview.chromium.org/257773009/diff/50001/cc/layers/picture_layer_... > cc/layers/picture_layer_impl.h:232: size_t > unitialized_tiles_required_for_activation_count_; > On 2014/04/25 23:39:53, reveman wrote: > > uninitialized_ > > Gah, I knew I'd mess up somewhere. Fixed. ping
https://codereview.chromium.org/257773009/diff/70001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/257773009/diff/70001/cc/layers/layer_impl.h#n... cc/layers/layer_impl.h:182: virtual void NotifyTileInitialized(const Tile* tile) {} Can this get merged with the AddDamageRect code that just landed? https://codereview.chromium.org/257773009/diff/70001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/257773009/diff/70001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:232: size_t uninitialized_tiles_required_for_activation_count_; Ah, I think I get it, because TileManager has no global pass over all tiles, then it doesn't know how many tiles are required for activation anymore, and the layer has to keep track of that. https://codereview.chromium.org/257773009/diff/70001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/257773009/diff/70001/cc/resources/tile_manage... cc/resources/tile_manager.cc:956: size_t total_required_for_activation_tile_count = 0; style nit: size_t -> int https://codereview.chromium.org/257773009/diff/70001/cc/resources/tile_manage... cc/resources/tile_manager.cc:960: if ((*it)->IsOnActiveOrPendingTree()) { Shouldn't this just be on pending? https://codereview.chromium.org/257773009/diff/70001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/257773009/diff/70001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:1219: void LayerTreeHostImpl::NotifyTileInitialized(const Tile* tile) { This function also already landed in https://chromiumcodereview.appspot.com/219963005/.
PTAL https://codereview.chromium.org/257773009/diff/70001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/257773009/diff/70001/cc/layers/layer_impl.h#n... cc/layers/layer_impl.h:182: virtual void NotifyTileInitialized(const Tile* tile) {} On 2014/04/29 19:44:19, enne wrote: > Can this get merged with the AddDamageRect code that just landed? Done. https://codereview.chromium.org/257773009/diff/70001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/257773009/diff/70001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:232: size_t uninitialized_tiles_required_for_activation_count_; On 2014/04/29 19:44:19, enne wrote: > Ah, I think I get it, because TileManager has no global pass over all tiles, > then it doesn't know how many tiles are required for activation anymore, and the > layer has to keep track of that. Ya, that's exactly right. The plan is to not have this check in AssignGpuMemory at all, and instead the tile manager will check this before sending activation signals to check whether it can actually activate. This is just a first step that makes it possible to not iterate every tile. https://codereview.chromium.org/257773009/diff/70001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/257773009/diff/70001/cc/resources/tile_manage... cc/resources/tile_manager.cc:956: size_t total_required_for_activation_tile_count = 0; On 2014/04/29 19:44:19, enne wrote: > style nit: size_t -> int Done. https://codereview.chromium.org/257773009/diff/70001/cc/resources/tile_manage... cc/resources/tile_manager.cc:960: if ((*it)->IsOnActiveOrPendingTree()) { On 2014/04/29 19:44:19, enne wrote: > Shouldn't this just be on pending? Done. https://codereview.chromium.org/257773009/diff/70001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/257773009/diff/70001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:1219: void LayerTreeHostImpl::NotifyTileInitialized(const Tile* tile) { On 2014/04/29 19:44:19, enne wrote: > This function also already landed in > https://chromiumcodereview.appspot.com/219963005/. Fixed.
lgtm https://codereview.chromium.org/257773009/diff/90001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/257773009/diff/90001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:232: size_t uninitialized_tiles_required_for_activation_count_; size_t -> int
The CQ bit was checked by vmpstr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/257773009/150001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel on tryserver.chromium
The CQ bit was checked by vmpstr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/257773009/150001
Message was sent while issue was closed.
Change committed as 267400
Message was sent while issue was closed.
It looks like this patch caused the context_lost tests to start failing repeatably on Mac OS X and intermittently on Windows on the GPU bots. Links to the first failing builds on several bots: http://build.chromium.org/p/chromium.gpu/builders/Win7%20Release%20%28NVIDIA%... http://build.chromium.org/p/chromium.gpu/builders/Win7%20Release%20%28ATI%29/... http://build.chromium.org/p/chromium.gpu/builders/Win7%20Release%20%28Intel%2... http://build.chromium.org/p/chromium.gpu/builders/Mac%20Debug%20%28Intel%29/b... http://build.chromium.org/p/chromium.gpu/builders/Mac%20Release%20%28ATI%29/b... http://build.chromium.org/p/chromium.gpu/builders/Mac%2010.8%20Release%20%28I... http://build.chromium.org/p/chromium.gpu/builders/Mac%2010.8%20Debug%20%28Int... http://build.chromium.org/p/chromium.gpu/builders/Mac%2010.8%20Release%20%28A... Sorry that this wasn't caught by the commit queue; we are still in the process of making the GPU bots required jobs in the CQ. I'm reverting this patch to get the tree green again.
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/269633006/ by kbr@chromium.org. The reason for reverting is: Caused context_lost tests to hang on Win and Mac GPU bots. See original CL for links to failing builds. .
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/268683002/ by pfeldman@chromium.org. The reason for reverting is: Breaks DevTools rendering badly. See crbug.com/369086..
Message was sent while issue was closed.
https://codereview.chromium.org/257773009/diff/150001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/257773009/diff/150001/cc/resources/tile_manag... cc/resources/tile_manager.cc:547: tile_version.set_rasterize_on_demand(); I think we need to call NotifyTileInitialized() here. Maybe we should always call set_rasterize_on_demand() and handle use_rasterize_on_demand in PictureLayerImpl instead.
Message was sent while issue was closed.
https://codereview.chromium.org/257773009/diff/150001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/257773009/diff/150001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:428: --uninitialized_tiles_required_for_activation_count_; what if 2 different versions of the tile have been initialized?
Message was sent while issue was closed.
https://codereview.chromium.org/257773009/diff/150001/cc/layers/picture_layer... File cc/layers/picture_layer_impl.cc (right): https://codereview.chromium.org/257773009/diff/150001/cc/layers/picture_layer... cc/layers/picture_layer_impl.cc:428: --uninitialized_tiles_required_for_activation_count_; On 2014/05/01 14:17:00, reveman wrote: > what if 2 different versions of the tile have been initialized? That's a good point, this count doesn't account for the tiles that were already initialized but still needed rasterization (ie, we did a different version of the same tile). I'll have to think about that one. https://codereview.chromium.org/257773009/diff/150001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/257773009/diff/150001/cc/resources/tile_manag... cc/resources/tile_manager.cc:547: tile_version.set_rasterize_on_demand(); On 2014/05/01 13:38:09, reveman wrote: > I think we need to call NotifyTileInitialized() here. Maybe we should always > call set_rasterize_on_demand() and handle use_rasterize_on_demand in > PictureLayerImpl instead. I don't think this is required here... The rasterized check is really to determine if all rfa tiles had memory, and they don't really have memory in this case. We mark this on demand and activate anyway. I'm not sure why the count should be decremented, as those tiles still technically need to be rasterized/initialized.
Message was sent while issue was closed.
FYI, this also caused Gmail to stop rendering in 36.0.1969.0 (Official Build 267516) canary on Mac OS. Need more tests which would have caught this failure.
Message was sent while issue was closed.
On 2014/05/02 01:29:10, Ken Russell wrote: > FYI, this also caused Gmail to stop rendering in 36.0.1969.0 (Official Build > 267516) canary on Mac OS. > > Need more tests which would have caught this failure. Also not that is have caused issuses in ChromeOS oobe/login UI: https://code.google.com/p/chromium/issues/detail?id=369256 https://code.google.com/p/chromium/issues/detail?id=370170 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
