cc/tiles/tile_manager.cc - Issue 2003353003: cc: Ensure members needed on task completion get called on valid thread.

Unified Diff: cc/tiles/tile_manager.cc

Issue 2003353003: cc: Ensure members needed on task completion get called on valid thread. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@remove_task_new_state_in_dtor

Patch Set: another approach, feedback Created 4 years, 7 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

Index: cc/tiles/tile_manager.cc

diff --git a/cc/tiles/tile_manager.cc b/cc/tiles/tile_manager.cc

index be62b2fa2f5a59990c54f1ed5346ce25d041ed78..198f123e92e6b61fc1d57d3d690e69357f8d11fa 100644

--- a/cc/tiles/tile_manager.cc

+++ b/cc/tiles/tile_manager.cc

@@ -18,6 +18,7 @@

#include "base/memory/ptr_util.h"

#include "base/metrics/histogram.h"

#include "base/numerics/safe_conversions.h"

+#include "base/threading/thread_checker.h"

#include "base/trace_event/trace_event_argument.h"

#include "cc/base/histograms.h"

#include "cc/debug/devtools_instrumentation.h"

@@ -41,12 +42,9 @@ DEFINE_SCOPED_UMA_HISTOGRAM_AREA_TIMER(

"Compositing.%s.RasterTask.RasterUs",

"Compositing.%s.RasterTask.RasterPixelsPerMs");

-// TODO(prashant.n): Separate out members needed in RunOnWorkerThread() and

-// OnTaskCompleted() in RasterTaskImpl. crbug.com/613812.

class RasterTaskImpl : public TileTask {

public:

- RasterTaskImpl(TileManager* tile_manager,

- Resource* resource,

+ RasterTaskImpl(std::unique_ptr<TaskCompletionInfo> completion_info,

scoped_refptr<RasterSource> raster_source,

const gfx::Rect& content_rect,

const gfx::Rect& invalid_content_rect,

@@ -55,7 +53,7 @@ class RasterTaskImpl : public TileTask {

TileResolution tile_resolution,

int layer_id,

uint64_t source_prepare_tiles_id,

- Tile* tile,

+ void* tile_ptr,

uint64_t new_content_id,

uint64_t previous_content_id,

uint64_t resource_content_id,

@@ -64,8 +62,7 @@ class RasterTaskImpl : public TileTask {

TileTask::Vector* dependencies,

bool supports_concurrent_execution)

: TileTask(supports_concurrent_execution, dependencies),

- tile_manager_(tile_manager),

- resource_(resource),

+ completion_info_(std::move(completion_info)),

raster_source_(std::move(raster_source)),

content_rect_(content_rect),

invalid_content_rect_(invalid_content_rect),

@@ -74,7 +71,7 @@ class RasterTaskImpl : public TileTask {

tile_resolution_(tile_resolution),

layer_id_(layer_id),

source_prepare_tiles_id_(source_prepare_tiles_id),

- tile_(tile),

+ tile_ptr_(tile_ptr),

new_content_id_(new_content_id),

previous_content_id_(previous_content_id),

resource_content_id_(resource_content_id),

@@ -90,7 +87,7 @@ class RasterTaskImpl : public TileTask {

DCHECK(raster_buffer_);

frame_viewer_instrumentation::ScopedRasterTask raster_task(

- tile_, tile_resolution_, source_frame_number_, layer_id_);

+ tile_ptr_, tile_resolution_, source_frame_number_, layer_id_);

ScopedRasterTaskTimer timer;

timer.SetArea(content_rect_.size().GetArea());

@@ -103,16 +100,25 @@ class RasterTaskImpl : public TileTask {

// Overridden from TileTask:

void OnTaskCompleted() override {

- tile_manager_->OnRasterTaskCompleted(std::move(raster_buffer_), tile_,

- resource_, state().IsCanceled());

+ DCHECK(origin_thread_checker_.CalledOnValidThread());

prashant.n 2016/05/26 13:43:22 Add DCHECK_IS_ON() as we want to call this explici

reveman 2016/05/26 14:49:03 the thread checker is alleady a noop in non-debug

+ TileManager::OnRasterTaskCompleted(std::move(raster_buffer_),

+ completion_info_.get(),

+ state().IsCanceled());

}

protected:

~RasterTaskImpl() override { DCHECK(!raster_buffer_); }

private:

- TileManager* tile_manager_;

- Resource* resource_;

+#if DCHECK_IS_ON()

+ base::ThreadChecker origin_thread_checker_;

+#endif

+ // This member should be accessed only in origin thread.

+ std::unique_ptr<TaskCompletionInfo> completion_info_;

+ // The following members should be accessed only in worker thread.

scoped_refptr<RasterSource> raster_source_;

gfx::Rect content_rect_;

gfx::Rect invalid_content_rect_;

@@ -121,16 +127,23 @@ class RasterTaskImpl : public TileTask {

TileResolution tile_resolution_;

int layer_id_;

uint64_t source_prepare_tiles_id_;

- Tile* tile_;

+ void* tile_ptr_;

uint64_t new_content_id_;

uint64_t previous_content_id_;

uint64_t resource_content_id_;

int source_frame_number_;

std::unique_ptr<RasterBuffer> raster_buffer_;

DISALLOW_COPY_AND_ASSIGN(RasterTaskImpl);

};

+struct RasterTaskCompletionInfo : public TileTask::TaskCompletionInfo {

+ TileManager* task_owner;

+ Resource* resource;

+ Tile* tile;

+};

TaskCategory TaskCategoryForTileTask(TileTask* task,

bool use_foreground_category) {

if (!task->supports_concurrent_execution())

@@ -936,61 +949,73 @@ scoped_refptr<TileTask> TileManager::CreateRasterTask(

it = images.erase(it);

}

- bool supports_concurrent_execution = !use_gpu_rasterization_;

+ std::unique_ptr<RasterTaskCompletionInfo> completion_info(

+ new RasterTaskCompletionInfo);

+ completion_info->task_owner = this;

+ completion_info->resource = resource;

+ completion_info->tile = tile;

std::unique_ptr<RasterBuffer> raster_buffer =

tile_task_manager_->GetRasterBufferProvider()->AcquireBufferForRaster(

resource, resource_content_id, tile->invalidated_id());

+ bool supports_concurrent_execution = !use_gpu_rasterization_;

return make_scoped_refptr(new RasterTaskImpl(

- this, resource, prioritized_tile.raster_source(), tile->content_rect(),

- tile->invalidated_content_rect(), tile->contents_scale(),

- playback_settings, prioritized_tile.priority().resolution,

- tile->layer_id(), prepare_tiles_count_, tile, tile->id(),

+ std::move(completion_info), prioritized_tile.raster_source(),

+ tile->content_rect(), tile->invalidated_content_rect(),

+ tile->contents_scale(), playback_settings,

+ prioritized_tile.priority().resolution, tile->layer_id(),

+ prepare_tiles_count_, static_cast<void*>(tile), tile->id(),

tile->invalidated_id(), resource_content_id, tile->source_frame_number(),

std::move(raster_buffer), &decode_tasks, supports_concurrent_execution));

}

+// static

void TileManager::OnRasterTaskCompleted(

std::unique_ptr<RasterBuffer> raster_buffer,

- Tile* tile,

- Resource* resource,

+ TileTask::TaskCompletionInfo* completion_info,

bool was_canceled) {

- DCHECK(tile);

- DCHECK(tiles_.find(tile->id()) != tiles_.end());

- tile_task_manager_->GetRasterBufferProvider()->ReleaseBufferForRaster(

- std::move(raster_buffer));

- TileDrawInfo& draw_info = tile->draw_info();

- DCHECK(tile->raster_task_.get());

- orphan_tasks_.push_back(tile->raster_task_);

- tile->raster_task_ = nullptr;

+ RasterTaskCompletionInfo* info =

+ static_cast<RasterTaskCompletionInfo*>(completion_info);

+ DCHECK(info->tile);

+ DCHECK(info->task_owner->tiles_.find(info->tile->id()) !=

+ info->task_owner->tiles_.end());

+ info->task_owner->tile_task_manager_->GetRasterBufferProvider()

+ ->ReleaseBufferForRaster(std::move(raster_buffer));

+ TileDrawInfo& draw_info = info->tile->draw_info();

+ DCHECK(info->tile->raster_task_.get());

+ info->task_owner->orphan_tasks_.push_back(info->tile->raster_task_);

+ info->tile->raster_task_ = nullptr;

// Unref all the images.

- auto images_it = scheduled_draw_images_.find(tile->id());

+ auto images_it =

+ info->task_owner->scheduled_draw_images_.find(info->tile->id());

const std::vector<DrawImage>& images = images_it->second;

for (const auto& image : images)

- image_decode_controller_->UnrefImage(image);

- scheduled_draw_images_.erase(images_it);

+ info->task_owner->image_decode_controller_->UnrefImage(image);

+ info->task_owner->scheduled_draw_images_.erase(images_it);

if (was_canceled) {

- ++flush_stats_.canceled_count;

+ ++info->task_owner->flush_stats_.canceled_count;

// TODO(ericrk): If more partial raster work is done in the future, it may

// be worth returning the resource to the pool with its previous ID (not

// currently tracked). crrev.com/1370333002/#ps40001 has a possible method

// of achieving this.

- resource_pool_->ReleaseResource(resource, 0 /* content_id */);

+ info->task_owner->resource_pool_->ReleaseResource(info->resource,

+ 0 /* content_id */);

return;

}

- ++flush_stats_.completed_count;

+ ++info->task_owner->flush_stats_.completed_count;

draw_info.set_use_resource();

- draw_info.resource_ = resource;

- draw_info.contents_swizzled_ = DetermineResourceRequiresSwizzle(tile);

+ draw_info.resource_ = info->resource;

+ draw_info.contents_swizzled_ =

+ info->task_owner->DetermineResourceRequiresSwizzle(info->tile);

DCHECK(draw_info.IsReadyToDraw());

draw_info.set_was_ever_ready_to_draw();

- client_->NotifyTileStateChanged(tile);

+ info->task_owner->client_->NotifyTileStateChanged(info->tile);

}

ScopedTilePtr TileManager::CreateTile(const Tile::CreateInfo& info,

« no previous file with comments | « cc/tiles/tile_manager.h ('k') | no next file » | no next file with comments »