Clean up tile deletion

Clean up tile deletion

Now that raster task lifetime has been cleaned up, we no longer need to
keep tiles alive during raster task execution. This change causes tiles
to be cleaned up immediately upon being released.

R=vmpstr
BUG=672606
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2612413003
Cr-Commit-Position: refs/heads/master@{#442682}
Committed: https://chromium.googlesource.com/chromium/src/+/e5410705426f111d9ea78d121b70cb87ddf38187

[ericrk, 2017-01-06 01:11:47]

Description was changed from

==========
Clean up tile deletion

Now that raster task lifetime has been cleaned up, we no longer need to
keep tiles alive during raster task execution. This change causes tiles
to be cleaned up immediately upon being released.

R=vmpstr
BUG=672606
==========

to

==========
Clean up tile deletion

Now that raster task lifetime has been cleaned up, we no longer need to
keep tiles alive during raster task execution. This change causes tiles
to be cleaned up immediately upon being released.

R=vmpstr
BUG=672606
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[ericrk, 2017-01-06 01:15:38]

fix_perftest

[ericrk, 2017-01-06 01:15:49]

The CQ bit was checked by ericrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 01:17:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 01:58:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 01:58:54]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[vmpstr, 2017-01-09 18:38:16]

https://codereview.chromium.org/2612413003/diff/20001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/2612413003/diff/20001/cc/tiles/tile_manager.c...
cc/tiles/tile_manager.cc:422: delete tile;
This patch is awesome, thanks. Can you go a bit further and make tile be a
proper unique_ptr, instead of ScopedTilePtr (which just has a custom deleter),
and instead call this release function in the dtor of the tile?

Previously it needed to be a deleter because we delayed the deletion, but since
we're not delaying it anymore, we can just have some cleanup calls from the
dtor.

[ericrk, 2017-01-09 19:47:54]

https://codereview.chromium.org/2612413003/diff/20001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/2612413003/diff/20001/cc/tiles/tile_manager.c...
cc/tiles/tile_manager.cc:422: delete tile;
On 2017/01/09 18:38:15, vmpstr wrote:
> This patch is awesome, thanks. Can you go a bit further and make tile be a
> proper unique_ptr, instead of ScopedTilePtr (which just has a custom deleter),
> and instead call this release function in the dtor of the tile?
> 
> Previously it needed to be a deleter because we delayed the deletion, but
since
> we're not delaying it anymore, we can just have some cleanup calls from the
> dtor.

Good suggestion. Done.

[ericrk, 2017-01-09 19:47:59]

The CQ bit was checked by ericrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 19:48:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vmpstr, 2017-01-09 20:02:09]

lgtm

https://codereview.chromium.org/2612413003/diff/40001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/2612413003/diff/40001/cc/tiles/tile_manager.c...
cc/tiles/tile_manager.cc:1031: Tile* tile = found != tiles_.end() ?
found->second : nullptr;
nit: can you structure it with one less conditional like:

Tile* tile = nullptr;
if (found != tiles_.end()) {
  tile = found->second;
  DCHECK(...);
  tile->raster_task_ = nullptr;
}

https://codereview.chromium.org/2612413003/diff/40001/cc/tiles/tile_manager.c...
cc/tiles/tile_manager.cc:1071: std::unique_ptr<Tile> tile(
nit: can you make this

auto tile = base::MakeUnique<Tile>(...);

[commit-bot: I haz the power, 2017-01-09 20:33:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 20:33:27]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[ericrk, 2017-01-09 21:23:41]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2017-01-09 21:23:42]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/2612413003/#ps60001
(title: "Actually release resources when !tile")

[commit-bot: I haz the power, 2017-01-09 21:24:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ericrk, 2017-01-09 21:35:22]

https://codereview.chromium.org/2612413003/diff/40001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/2612413003/diff/40001/cc/tiles/tile_manager.c...
cc/tiles/tile_manager.cc:1031: Tile* tile = found != tiles_.end() ?
found->second : nullptr;
On 2017/01/09 20:02:09, vmpstr wrote:
> nit: can you structure it with one less conditional like:
> 
> Tile* tile = nullptr;
> if (found != tiles_.end()) {
>   tile = found->second;
>   DCHECK(...);
>   tile->raster_task_ = nullptr;
> }

Done.

https://codereview.chromium.org/2612413003/diff/40001/cc/tiles/tile_manager.c...
cc/tiles/tile_manager.cc:1071: std::unique_ptr<Tile> tile(
On 2017/01/09 20:02:09, vmpstr wrote:
> nit: can you make this
> 
> auto tile = base::MakeUnique<Tile>(...);

Constructor is private, which stops this from working :(

[commit-bot: I haz the power, 2017-01-09 22:39:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 22:39:04]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[ericrk, 2017-01-09 23:05:36]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2017-01-09 23:06:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 00:22:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 00:22:31]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[ericrk, 2017-01-10 18:24:10]

The CQ bit was checked by ericrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 18:24:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ericrk, 2017-01-10 19:27:12]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2017-01-10 19:27:12]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/2612413003/#ps80001
(title: ""Fix Tile* comparison in tests"")

[commit-bot: I haz the power, 2017-01-10 19:27:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 21:00:11]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484076432250440,
"parent_rev": "f38dd70ecdbd4fb194a1165cf8b8b5b08034ce20", "commit_rev":
"e5410705426f111d9ea78d121b70cb87ddf38187"}

[commit-bot: I haz the power, 2017-01-10 21:00:41]

Description was changed from

==========
Clean up tile deletion

Now that raster task lifetime has been cleaned up, we no longer need to
keep tiles alive during raster task execution. This change causes tiles
to be cleaned up immediately upon being released.

R=vmpstr
BUG=672606
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Clean up tile deletion

Now that raster task lifetime has been cleaned up, we no longer need to
keep tiles alive during raster task execution. This change causes tiles
to be cleaned up immediately upon being released.

R=vmpstr
BUG=672606
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2612413003
Cr-Commit-Position: refs/heads/master@{#442682}
Committed:
https://chromium.googlesource.com/chromium/src/+/e5410705426f111d9ea78d121b70...
==========

[commit-bot: I haz the power, 2017-01-10 21:00:43]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/e5410705426f111d9ea78d121b70...