Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2726343004:
cc: Optimize decode scheduling for checker-images. (Closed)
Descriptioncc: Optimize decode scheduling for checker-images.
The CheckerImageTracker currently schedules image decodes for all
checkered images as we schedule raster work for the dependent tiles.
Once image decodes have been submitted to the tracker, they can not be
cancelled and the order can not be changed. This means that as tile
priorities change, the order in which images are decoded may not be
aligned with the priority of the tiles which depend on them.
Instead use a queue for scheduling decode work in the tracker. Only
one outstanding decode is pending with the decode service at a time, and
the queue is re-build each time we schedule work for a new set of tiles.
This requires plumbing through the checker-imaged tiles in the
RasterWorkerQueue since they are now inter-leaved with tiles that
actually need to be rasterized.
BUG=689184
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2726343004
Cr-Commit-Position: refs/heads/master@{#466261}
Committed: https://chromium.googlesource.com/chromium/src/+/3e256c223b5c4057b6a9e0556041cdb8ba11c029
Patch Set 1 #Patch Set 2 : .. #
Total comments: 12
Patch Set 3 : addressed comments #
Total comments: 10
Patch Set 4 : .. #
Total comments: 19
Patch Set 5 : addressed comments #Patch Set 6 : .. #
Total comments: 48
Patch Set 7 : addressed comments #
Total comments: 1
Patch Set 8 : fixed tile iteration #
Total comments: 3
Patch Set 9 : tested #
Total comments: 29
Patch Set 10 : one more test #
Total comments: 18
Patch Set 11 : addressed comments #Patch Set 12 : rebase #
Messages
Total messages: 41 (13 generated)
Description was changed from ========== cc: Optimize decode scheduling for checker-images. The CheckerImageTracker currently schedules image decodes for all checkered images as we schedule raster work for the dependent tiles.This means that as tile priorities change, the order in which images are decoded may not be aligned with the priority of the tiles which depend on them. Instead use a queue for scheduling decode work in the tracker. Only one outstanding decode is pending with the decode service at a time, and the queue is re-build each time we schedule work for a new set of tiles. This also plumbs through the checker-images tiles in the RasterWorkerQueue. BUG= ========== to ========== cc: Optimize decode scheduling for checker-images. The CheckerImageTracker currently schedules image decodes for all checkered images as we schedule raster work for the dependent tiles.This means that as tile priorities change, the order in which images are decoded may not be aligned with the priority of the tiles which depend on them. Instead use a queue for scheduling decode work in the tracker. Only one outstanding decode is pending with the decode service at a time, and the queue is re-build each time we schedule work for a new set of tiles. This also plumbs through the checker-images tiles in the RasterWorkerQueue. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== cc: Optimize decode scheduling for checker-images. The CheckerImageTracker currently schedules image decodes for all checkered images as we schedule raster work for the dependent tiles.This means that as tile priorities change, the order in which images are decoded may not be aligned with the priority of the tiles which depend on them. Instead use a queue for scheduling decode work in the tracker. Only one outstanding decode is pending with the decode service at a time, and the queue is re-build each time we schedule work for a new set of tiles. This also plumbs through the checker-images tiles in the RasterWorkerQueue. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Optimize decode scheduling for checker-images. The CheckerImageTracker currently schedules image decodes for all checkered images as we schedule raster work for the dependent tiles.This means that as tile priorities change, the order in which images are decoded may not be aligned with the priority of the tiles which depend on them. Instead use a queue for scheduling decode work in the tracker. Only one outstanding decode is pending with the decode service at a time, and the queue is re-build each time we schedule work for a new set of tiles. This also plumbs through the checker-imaged tiles in the RasterWorkerQueue. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
khushalsagar@chromium.org changed reviewers: + vmpstr@chromium.org
+vmpstr. I'm going to add a test for this. In the meanwhile, does this look correct?
+vmpstr. I'm going to add a test for this. In the meanwhile, does this look correct?
Do you have a doc that explains what this is trying to do in more details? I don't quite understand the goal here and I think a doc would clarify it. https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/checker_image_... File cc/tiles/checker_image_tracker.cc (right): https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.cc:68: void CheckerImageTracker::ResetImageDecodeQueue() { What is this function doing? Can you comment/explain? https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.cc:168: if (image_decode_request_queue_.size() == 1) 1u https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/checker_image_... File cc/tiles/checker_image_tracker.h (right): https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.h:70: void RequestNextImageDecode(); Comment, and if this is what I think it is, then it should be called ScheduleNextImageDecode https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.h:89: std::queue<sk_sp<const SkImage>> image_decode_request_queue_; std::queue's underlying type is std::deque which is a big memory hog as we've discovered. Since this is just sk_sp (small objects), I think we should just use a vector. https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:825: bool process_tile_for_checker_images_only = tile->draw_info().resource(); This seems a bit awkward: "if a tile has a resource then process tile for checker images" seems to conflate unrelated concepts. https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:831: checker_image_tracker_.FilterImagesForCheckeringForTile( It's not clear to me what this is doing, since both of the vectors are discarded?
On 2017/03/06 20:45:26, vmpstr wrote: > Do you have a doc that explains what this is trying to do in more details? I > don't quite understand the goal here and I think a doc would clarify it. So the problem we have is that there are checker-imaged tiles on the active/pending tree, which definitely don't need to be rasterized until the images that they depend on have been decoded. But if something changes tile priorities (scrolling, impl-side animation), we still need to know what the priority order of these tiles should be so we can schedule image decodes accordingly. Its somewhat similar to what we do with process_for_images_only_tiles. They don't need to be rasterized but decide the order in which images are pre-decoded. > > https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/checker_image_... > File cc/tiles/checker_image_tracker.cc (right): > > https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/checker_image_... > cc/tiles/checker_image_tracker.cc:68: void > CheckerImageTracker::ResetImageDecodeQueue() { > What is this function doing? Can you comment/explain? > > https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/checker_image_... > cc/tiles/checker_image_tracker.cc:168: if (image_decode_request_queue_.size() == > 1) > 1u > > https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/checker_image_... > File cc/tiles/checker_image_tracker.h (right): > > https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/checker_image_... > cc/tiles/checker_image_tracker.h:70: void RequestNextImageDecode(); > Comment, and if this is what I think it is, then it should be called > ScheduleNextImageDecode > > https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/checker_image_... > cc/tiles/checker_image_tracker.h:89: std::queue<sk_sp<const SkImage>> > image_decode_request_queue_; > std::queue's underlying type is std::deque which is a big memory hog as we've > discovered. Since this is just sk_sp (small objects), I think we should just use > a vector. > > https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/tile_manager.cc > File cc/tiles/tile_manager.cc (right): > > https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/tile_manager.c... > cc/tiles/tile_manager.cc:825: bool process_tile_for_checker_images_only = > tile->draw_info().resource(); > This seems a bit awkward: "if a tile has a resource then process tile for > checker images" seems to conflate unrelated concepts. > > https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/tile_manager.c... > cc/tiles/tile_manager.cc:831: > checker_image_tracker_.FilterImagesForCheckeringForTile( > It's not clear to me what this is doing, since both of the vectors are > discarded?
https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/checker_image_... File cc/tiles/checker_image_tracker.cc (right): https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.cc:68: void CheckerImageTracker::ResetImageDecodeQueue() { On 2017/03/06 20:45:25, vmpstr wrote: > What is this function doing? Can you comment/explain? Done. I added a comment on the header. https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.cc:168: if (image_decode_request_queue_.size() == 1) On 2017/03/06 20:45:25, vmpstr wrote: > 1u Done. https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/checker_image_... File cc/tiles/checker_image_tracker.h (right): https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.h:70: void RequestNextImageDecode(); On 2017/03/06 20:45:25, vmpstr wrote: > Comment, and if this is what I think it is, then it should be called > ScheduleNextImageDecode Done. https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.h:89: std::queue<sk_sp<const SkImage>> image_decode_request_queue_; On 2017/03/06 20:45:25, vmpstr wrote: > std::queue's underlying type is std::deque which is a big memory hog as we've > discovered. Since this is just sk_sp (small objects), I think we should just use > a vector. Done. https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:825: bool process_tile_for_checker_images_only = tile->draw_info().resource(); On 2017/03/06 20:45:25, vmpstr wrote: > This seems a bit awkward: "if a tile has a resource then process tile for > checker images" seems to conflate unrelated concepts. May be this could be bool process_tile_for_checker_images_only = tile->draw_info().resource() && tile->is_checker_imaged() to make it obvious? https://codereview.chromium.org/2726343004/diff/20001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:831: checker_image_tracker_.FilterImagesForCheckeringForTile( On 2017/03/06 20:45:25, vmpstr wrote: > It's not clear to me what this is doing, since both of the vectors are > discarded? Its filtering the images in the tile with the CheckerImageTracker so it can re-build its internal queue. May be a different name for the Filter function since it makes this queue as well?
https://codereview.chromium.org/2726343004/diff/40001/cc/tiles/checker_image_... File cc/tiles/checker_image_tracker.cc (right): https://codereview.chromium.org/2726343004/diff/40001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.cc:78: pending_image_decodes_.insert(pending_image_decode->uniqueID()); I guess my confusion comes from the implementation of this function: 1. Why are we clearing pending_image_decodes_? Aren't those already in the image controller's queue? 2. Is the work you're doing with image_decode_request_queue_ equivalent to erase(begin() + 1, end())? If so, that's what it should say instead. https://codereview.chromium.org/2726343004/diff/40001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.cc:104: image_decode_request_queue_.erase(image_decode_request_queue_.begin()); "pop_front"? Unless the type doesn't have it https://codereview.chromium.org/2726343004/diff/40001/cc/tiles/checker_image_... File cc/tiles/checker_image_tracker.h (right): https://codereview.chromium.org/2726343004/diff/40001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.h:97: std::list<sk_sp<const SkImage>> image_decode_request_queue_; Can you write a cc_perftest for repeated schedules/cancels to see if std::list performs better than std::vector (this can be a one-off) https://codereview.chromium.org/2726343004/diff/40001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2726343004/diff/40001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:831: checker_image_tracker_.FilterImagesForCheckeringForTile( Yeah I think filter might be a bad name since from here it looks like you're partitioning the images into two sets and then discarding this information. Btw, when do we unlock these images? https://codereview.chromium.org/2726343004/diff/40001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:988: tile->set_is_checker_imaged(); When is this flag reset?
How does it look now? https://codereview.chromium.org/2726343004/diff/40001/cc/tiles/checker_image_... File cc/tiles/checker_image_tracker.cc (right): https://codereview.chromium.org/2726343004/diff/40001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.cc:78: pending_image_decodes_.insert(pending_image_decode->uniqueID()); On 2017/03/07 19:25:08, vmpstr wrote: > I guess my confusion comes from the implementation of this function: > > 1. Why are we clearing pending_image_decodes_? Aren't those already in the image > controller's queue? > 2. Is the work you're doing with image_decode_request_queue_ equivalent to > erase(begin() + 1, end())? If so, that's what it should say instead. Removed the function. I think instead of having a reset and then re-building the queue internally in FilterImagesForCheckering, its much cleaner if the TileManager just builds the queue and sets it. https://codereview.chromium.org/2726343004/diff/40001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.cc:104: image_decode_request_queue_.erase(image_decode_request_queue_.begin()); On 2017/03/07 19:25:08, vmpstr wrote: > "pop_front"? Unless the type doesn't have it Not needed anymore. https://codereview.chromium.org/2726343004/diff/40001/cc/tiles/checker_image_... File cc/tiles/checker_image_tracker.h (right): https://codereview.chromium.org/2726343004/diff/40001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.h:97: std::list<sk_sp<const SkImage>> image_decode_request_queue_; On 2017/03/07 19:25:08, vmpstr wrote: > Can you write a cc_perftest for repeated schedules/cancels to see if std::list > performs better than std::vector (this can be a one-off) I guess not necessary anymore. We iterate through the queue to pull the next image to decode and just clear it at the end. https://codereview.chromium.org/2726343004/diff/40001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2726343004/diff/40001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:831: checker_image_tracker_.FilterImagesForCheckeringForTile( On 2017/03/07 19:25:08, vmpstr wrote: > Yeah I think filter might be a bad name since from here it looks like you're > partitioning the images into two sets and then discarding this information. Btw, > when do we unlock these images? I pulled that function into the TileManager. Does filter make sense now? And we unlock the images after the sync tree on which they were invalidated has been activated. They could be unlocked earlier, may be after we've built the raster tasks following the invalidation, since that should add the necessary refs. https://codereview.chromium.org/2726343004/diff/40001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:988: tile->set_is_checker_imaged(); On 2017/03/07 19:25:08, vmpstr wrote: > When is this flag reset? Its not. If a tile is checker-imaged. It can only be replaced by another tile. This state is never changed for a particular tile once set.
pingy.
https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/checker_image_... File cc/tiles/checker_image_tracker.cc (right): https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.cc:117: if (outstanding_image_decode_) Leave a comment here explaining that if we have something scheduled then wait until that completes first. https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.cc:126: // The image may have already been decoded. Proceed to the next image in the Can you rephrase this comment to match the code? That is, "if this image hasn't been decoded yet, then use it. Otherwise try the next image" https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.cc:134: if (next_image_to_decode_ == image_decode_queue_.size()) { Comment here https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/checker_image_... File cc/tiles/checker_image_tracker.h (right): https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.h:77: size_t next_image_to_decode_ = 0; next_image_index? Or what is this? size_t is certainly not an image :) https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:693: DCHECK(tile->draw_info().mode() == TileDrawInfo::OOM_MODE || Can you comment this DCHECK, since it's getting a bit complicated https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:826: bool process_tile_for_checker_images_only = Comment pls https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:834: << "Checker-imaged state is sticky on a Tile"; nit: "should be sticky", since when this fails it will say "Checker-imaged state is sticky on a Tile" which makes it sound like it failed because it's sticky https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:945: checker_image_tracker_.SetImageDecodeQueue( Can you call this ScheduleImageDecodes or something? https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:1028: prioritized_tile.raster_source()->GetDiscardableImagesInRect( I'd prefer you leave this call outside and pass the vector to this function (and pass Tile* instead of PrioritizedTile). It's easier to test, and easier to reason about IMO. https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/tile_manager.h File cc/tiles/tile_manager.h (right): https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/tile_manager.h... cc/tiles/tile_manager.h:325: void FilterImagesForCheckering( Can you add tests for this function? https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/tiling_set_ras... File cc/tiles/tiling_set_raster_queue_all.cc (right): https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/tiling_set_ras... cc/tiles/tiling_set_raster_queue_all.cc:208: if (!TileNeedsRaster(tile) && !should_process_tile_for_checker_imaging) TileNeedsRaster also has an occlusion check. I think that if the tile is occluded then it shouldn't be processed for checker imaging either, so I think you need to rework this a bit.
Description was changed from ========== cc: Optimize decode scheduling for checker-images. The CheckerImageTracker currently schedules image decodes for all checkered images as we schedule raster work for the dependent tiles.This means that as tile priorities change, the order in which images are decoded may not be aligned with the priority of the tiles which depend on them. Instead use a queue for scheduling decode work in the tracker. Only one outstanding decode is pending with the decode service at a time, and the queue is re-build each time we schedule work for a new set of tiles. This also plumbs through the checker-imaged tiles in the RasterWorkerQueue. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Optimize decode scheduling for checker-images. The CheckerImageTracker currently schedules image decodes for all checkered images as we schedule raster work for the dependent tiles. Once image decodes have been submitted to the tracker, they can not be cancelled and the order can not be changed. This means that as tile priorities change, the order in which images are decoded may not be aligned with the priority of the tiles which depend on them. Instead use a queue for scheduling decode work in the tracker. Only one outstanding decode is pending with the decode service at a time, and the queue is re-build each time we schedule work for a new set of tiles. This requires plumbing through the checker-imaged tiles in the RasterWorkerQueue since they are now inter-leaved with tiles that actually need to be rasterized. BUG=689184 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
I had to re-work this quite a bit in order to not add checker-imaged tiles to the |tiles_to_raster| queue. PTAL. https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/checker_image_... File cc/tiles/checker_image_tracker.cc (right): https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.cc:117: if (outstanding_image_decode_) On 2017/03/17 18:32:13, vmpstr wrote: > Leave a comment here explaining that if we have something scheduled then wait > until that completes first. Done. https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.cc:126: // The image may have already been decoded. Proceed to the next image in the On 2017/03/17 18:32:13, vmpstr wrote: > Can you rephrase this comment to match the code? That is, "if this image hasn't > been decoded yet, then use it. Otherwise try the next image" Done. https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.cc:134: if (next_image_to_decode_ == image_decode_queue_.size()) { On 2017/03/17 18:32:13, vmpstr wrote: > Comment here Done. https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/checker_image_... File cc/tiles/checker_image_tracker.h (right): https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/checker_image_... cc/tiles/checker_image_tracker.h:77: size_t next_image_to_decode_ = 0; On 2017/03/17 18:32:13, vmpstr wrote: > next_image_index? Or what is this? size_t is certainly not an image :) Done. https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:693: DCHECK(tile->draw_info().mode() == TileDrawInfo::OOM_MODE || On 2017/03/17 18:32:13, vmpstr wrote: > Can you comment this DCHECK, since it's getting a bit complicated Its good that you pointed this out. I hadn't realized that the rest of the function would have double-counted the memory usage for the checker-imaged tiles. https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:945: checker_image_tracker_.SetImageDecodeQueue( On 2017/03/17 18:32:13, vmpstr wrote: > Can you call this ScheduleImageDecodes or something? Done. https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:1028: prioritized_tile.raster_source()->GetDiscardableImagesInRect( On 2017/03/17 18:32:13, vmpstr wrote: > I'd prefer you leave this call outside and pass the vector to this function (and > pass Tile* instead of PrioritizedTile). It's easier to test, and easier to > reason about IMO. Done. https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/tiling_set_ras... File cc/tiles/tiling_set_raster_queue_all.cc (right): https://codereview.chromium.org/2726343004/diff/60001/cc/tiles/tiling_set_ras... cc/tiles/tiling_set_raster_queue_all.cc:208: if (!TileNeedsRaster(tile) && !should_process_tile_for_checker_imaging) On 2017/03/17 18:32:13, vmpstr wrote: > TileNeedsRaster also has an occlusion check. I think that if the tile is > occluded then it shouldn't be processed for checker imaging either, so I think > you need to rework this a bit. Done.
https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/checker_image... File cc/tiles/checker_image_tracker.cc (left): https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/checker_image... cc/tiles/checker_image_tracker.cc:118: // If a decode request is pending for this image, continue checkering it. Why is this check gone? https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/checker_image... File cc/tiles/checker_image_tracker.cc (right): https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/checker_image... cc/tiles/checker_image_tracker.cc:136: while (next_image_index_to_decode_ != image_decode_queue_.size()) { It's not clear to me why you're using an index here. Realistically, this vector is pretty small, so I would prefer // We already have an outstanding decode, this function will // be called again when it completes. if (oustanding_image_decode_) return; // Process the queue until we find the first image that needs // to be scheduled, and put it as an |oustanding_image_decode_|. // If no such image is found, |outsanding_image_decode_| is nullptr // upon loop termination and the queue is empty. while (!image_decode_queue_.empty()) { auto candidate = image_decode_queue_.front(); image_decode_queue_.erase(image_decode_queue_.begin()); if (...) { continue; } outstanding_image_decode_ = std::move(candidate); break; } // We couldn't find an image that needs a decode, so there // is no more work to do. if (!outstanding_image_decode_) { DCHECK_EQ(0u, image_decode_queue_.size()) return; } ... https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile.h File cc/tiles/tile.h (right): https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile.h#newcod... cc/tiles/tile.h:116: bool raster_task_scheduled_with_checker_images() const { Add a setter as well. I want to remove the friend TileManager on the tile eventually. This needs a comment: unlike "solid color analysis performed" which mean we've done the analysis, this could be interpreted that the raster task is _currently_ scheduled, vs raster task has _ever_ been scheduled. Also, remove the "raster_task" part here. This also gives me an interesting thought: if we checker an image, can it be the case that the tile afterwards is solid color? I'm not sure it's worth looking at this, but something to consider in the future if we want to save some memory while we're decoding the image. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_draw_info.h File cc/tiles/tile_draw_info.h (right): https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_draw_inf... cc/tiles/tile_draw_info.h:80: bool is_checker_imaged() const { Just by looking at tile.h and tile_draw_info.h, the two flags you're adding seem to say very similar things, so I think you need to add a comment explaining the state. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_draw_inf... cc/tiles/tile_draw_info.h:97: resource_is_checker_imaged_ = false; What if we set a resource that _is_ checker imaged? Can you make this more robust in terms of order of operations? That is info.resource_is_checker_imaged_ = true; info.set_resource(resource); will seem to do the wrong thing here. Maybe add a bool to set_resource indicating whether the resource was checker imaged with a default value of false? https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_draw_inf... cc/tiles/tile_draw_info.h:117: bool resource_is_checker_imaged_ = false; I'm not really a fan of "checker_imaged" as a name. I don't really have better suggestions, but maybe you can come up with something. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:687: // build the decode queue for checkered images. Add a comment saying that this block must come after the solid color analysis just for robustness. I don't think anything will break if you reorder them, but who knows with all the CDL work we're doing how the analysis might change. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:750: if (tile->raster_task_scheduled_with_checker_images()) { Maybe flip the if and the dcheck? If, if tile has a resource, dcheck that it has checker images? Then the else if can be just an else. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:753: prioritized_tile.raster_source()->GetDiscardableImagesInRect( This isn't really a trivial operation, is there any way to cache the images? https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:762: CreateRasterTask(prioritized_tile, client_->GetRasterColorSpace(), It's a bit unfortunate that we're doing this here now. I don't think it really matters, but it changes what traces tell us about which part of the code is slow. Is there a trace in CreateRasterTask? If not, could you add one? https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:771: eviction_priority_queue = FreeTileResourcesUntilUsageIsWithinLimit( What happens if eviction evicts tiles that are being checker imaged now? https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:777: while (!raster_priority_queue->IsEmpty()) { You can't do this here. This part defeats the point of having a lazy priority queue, since it ensures that we always process all tiles that haven't been scheduled yet, which means we update all tile priorities, which makes this a pretty big regression. We need to rethink this. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:795: checker_image_tracker_.ScheduleImageDecodeQueue( Can you move this call out of this function and do it at a later time? AssignGpuMemory shouldn't be doing any scheduling. We might need to refactor tile manager here before you can land this patch, but I'm not a fan of the coupling we're introducing throughout this function. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:840: DCHECK(!images_to_decode_for_raster || images_to_decode_for_raster->empty()); It's not clear to me why some of the output parameters are optional. Can we avoid this? https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:848: checkered_images->insert(image->uniqueID()); Here you're adding the image to both the vector and the set. That's something that the calling code can do, eliminating one output parameter. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:851: images_to_decode_for_raster->push_back(draw_image); image to decode for raster is not a clear name. I think this is meant to basically be "images _not_ to checker" Is it possible to make this function a simple partition? iterator PartitionImagesForCheckering(tree, images) { return std::stable_partition(images.begin(), images.end(), [](image) { ... }); } then the calling code will get both sets of images that need checkering and images that don't, and it can do whatever it wants with that information. If it makes the calling code verbose in a few places, then just add another free function or another member function (ie refactor a bit) https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:904: DCHECK(tile->HasRasterTask()); comment why this is true now. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:1048: prioritized_tile.raster_source()->GetDiscardableImagesInRect( The pattern of these two calls appears throughout. I think it's worth adding a helper function. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:1096: raster_task_scheduled_with_checker_images = just make the setter return the old value https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.h File cc/tiles/tile_manager.h (right): https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.h:318: void FilterImagesForCheckering( This needs a comment explaining what each parameter is, since images_to_decode_for_raster, checkered_images, and image_decode_queue all seem to store the same thing. Possibly, this needs to be reworked to move some code outside of the function. In my mind, the output of this should be fewer vectors/sets/queues and the calling code will then use the vector to populate/append to the queue or something like that. That being said, I haven't looked at the implementation yet and just commenting on the header. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.h:321: std::vector<DrawImage>* images_to_decode_for_raster, just images_to_decode is enough I think. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tiling_set_ra... File cc/tiles/tiling_set_raster_queue_all.cc (right): https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tiling_set_ra... cc/tiles/tiling_set_raster_queue_all.cc:211: !tile->draw_info().is_checker_imaged()) braces and comment
https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/checker_image... File cc/tiles/checker_image_tracker.cc (left): https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/checker_image... cc/tiles/checker_image_tracker.cc:118: // If a decode request is pending for this image, continue checkering it. On 2017/03/29 19:11:45, vmpstr wrote: > Why is this check gone? This shouldn't be necessary I think. It was making sure if we saw an image previously that we decided to checker, we should continue to do so. But that would happen anyway because we decide based on the image size. The rest of the checks after this, |images_pending_invalidation_| and |images_decoded_once_| should never be true if the image is pending decode. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/checker_image... File cc/tiles/checker_image_tracker.cc (right): https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/checker_image... cc/tiles/checker_image_tracker.cc:136: while (next_image_index_to_decode_ != image_decode_queue_.size()) { On 2017/03/29 19:11:45, vmpstr wrote: > It's not clear to me why you're using an index here. Realistically, this vector > is pretty small, so I would prefer Erasing from the beginning of the vector is going to be O(n). And the queue is going to have multiple instances of the same image (added for every tile), so it wouldn't be trivially small I think. The index saves us from doing that by just clearing at the end when we are done with the queue, and keeping an index from where valid candidates start. Its minimal complexity isolated to this once place, so I feel like its worth it. If you think not, let me know. > > // We already have an outstanding decode, this function will > // be called again when it completes. > if (oustanding_image_decode_) > return; > > // Process the queue until we find the first image that needs > // to be scheduled, and put it as an |oustanding_image_decode_|. > // If no such image is found, |outsanding_image_decode_| is nullptr > // upon loop termination and the queue is empty. > while (!image_decode_queue_.empty()) { > auto candidate = image_decode_queue_.front(); > image_decode_queue_.erase(image_decode_queue_.begin()); > if (...) { > continue; > } > outstanding_image_decode_ = std::move(candidate); > break; > } > > // We couldn't find an image that needs a decode, so there > // is no more work to do. > if (!outstanding_image_decode_) { > DCHECK_EQ(0u, image_decode_queue_.size()) > return; > } > > ... https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile.h File cc/tiles/tile.h (right): https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile.h#newcod... cc/tiles/tile.h:116: bool raster_task_scheduled_with_checker_images() const { On 2017/03/29 19:11:45, vmpstr wrote: > Add a setter as well. I want to remove the friend TileManager on the tile > eventually. > > This needs a comment: unlike "solid color analysis performed" which mean we've > done the analysis, this could be interpreted that the raster task is _currently_ > scheduled, vs raster task has _ever_ been scheduled. Also, remove the > "raster_task" part here. > > This also gives me an interesting thought: if we checker an image, can it be the > case that the tile afterwards is solid color? I'm not sure it's worth looking at > this, but something to consider in the future if we want to save some memory > while we're decoding the image. Its definitely going to be possible and something to optimize. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_draw_info.h File cc/tiles/tile_draw_info.h (right): https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_draw_inf... cc/tiles/tile_draw_info.h:97: resource_is_checker_imaged_ = false; On 2017/03/29 19:11:45, vmpstr wrote: > What if we set a resource that _is_ checker imaged? > > Can you make this more robust in terms of order of operations? > > That is > info.resource_is_checker_imaged_ = true; > info.set_resource(resource); > > will seem to do the wrong thing here. > > Maybe add a bool to set_resource indicating whether the resource was checker > imaged with a default value of false? That makes sense. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_draw_inf... cc/tiles/tile_draw_info.h:117: bool resource_is_checker_imaged_ = false; On 2017/03/29 19:11:45, vmpstr wrote: > I'm not really a fan of "checker_imaged" as a name. I don't really have better > suggestions, but maybe you can come up with something. At this point, may be just keep that for consistency? Coz everything else in the system is called checker-imaged... https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:750: if (tile->raster_task_scheduled_with_checker_images()) { On 2017/03/29 19:11:45, vmpstr wrote: > Maybe flip the if and the dcheck? If, if tile has a resource, dcheck that it has > checker images? Then the else if can be just an else. Not sure I followed, at this point the tile should definitely not have a resource, we handled that case above. We have 3 cases to handle here: 1) A Tile which is checker-imaged and has a valid resource, but still needs image decoding. 2) A Tile which has a raster task with images to checker that is already running. 3) A Tile for which we will create a raster task right now with checker-images. 1 gets handled above and we don't reach here. The rest 2 are handled here. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:753: prioritized_tile.raster_source()->GetDiscardableImagesInRect( On 2017/03/29 19:11:46, vmpstr wrote: > This isn't really a trivial operation, is there any way to cache the images? I was thinking about it. Is there a reason for not doing this already? We can store something like image analysis performed on a tile and stash 2 vectors on it, the ones that the raster tasks need to depend on and the ones which are checkered. The state shouldn't change for any tile coz we change the checker-image state only by invalidating tiles and replacing them with new ones. This should also remove the |scheduled_draw_images_| map right? Coz if a tile is scheduled, it always depends on the same set of images? https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:771: eviction_priority_queue = FreeTileResourcesUntilUsageIsWithinLimit( On 2017/03/29 19:11:46, vmpstr wrote: > What happens if eviction evicts tiles that are being checker imaged now? The eviction here shouldn't remove resources from any tiles that we went through in the raster priority queue above right? https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:777: while (!raster_priority_queue->IsEmpty()) { On 2017/03/29 19:11:46, vmpstr wrote: > You can't do this here. This part defeats the point of having a lazy priority > queue, since it ensures that we always process all tiles that haven't been > scheduled yet, which means we update all tile priorities, which makes this a > pretty big regression. > > We need to rethink this. Sorry, I had missed looking at the implementation of the queue to see it is lazily populated. You're right, this would be horrible and can be optimized a bunch. We can keep track of the checkered images at any time. And only iterate through the tile queue until more than one is left to be added. This will still have a worst case scenario where we go through the whole queue if the last tile has 2 checkered images for instance. Do you think that's reasonable? https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:840: DCHECK(!images_to_decode_for_raster || images_to_decode_for_raster->empty()); On 2017/03/29 19:11:46, vmpstr wrote: > It's not clear to me why some of the output parameters are optional. Can we > avoid this? I think computing it once and storing it on tile should simplify this a bunch.
https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/checker_image... File cc/tiles/checker_image_tracker.cc (right): https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/checker_image... cc/tiles/checker_image_tracker.cc:136: while (next_image_index_to_decode_ != image_decode_queue_.size()) { On 2017/03/29 22:10:20, Khushal wrote: > On 2017/03/29 19:11:45, vmpstr wrote: > > It's not clear to me why you're using an index here. Realistically, this > vector > > is pretty small, so I would prefer > > Erasing from the beginning of the vector is going to be O(n). And the queue is > going to have multiple instances of the same image (added for every tile), so it > wouldn't be trivially small I think. The index saves us from doing that by just > clearing at the end when we are done with the queue, and keeping an index from > where valid candidates start. Its minimal complexity isolated to this once > place, so I feel like its worth it. If you think not, let me know. > > > > > // We already have an outstanding decode, this function will > > // be called again when it completes. > > if (oustanding_image_decode_) > > return; > > > > // Process the queue until we find the first image that needs > > // to be scheduled, and put it as an |oustanding_image_decode_|. > > // If no such image is found, |outsanding_image_decode_| is nullptr > > // upon loop termination and the queue is empty. > > while (!image_decode_queue_.empty()) { > > auto candidate = image_decode_queue_.front(); > > image_decode_queue_.erase(image_decode_queue_.begin()); > > if (...) { > > continue; > > } > > outstanding_image_decode_ = std::move(candidate); > > break; > > } > > > > // We couldn't find an image that needs a decode, so there > > // is no more work to do. > > if (!outstanding_image_decode_) { > > DCHECK_EQ(0u, image_decode_queue_.size()) > > return; > > } > > > > ... > Discussed offline. Changed to erase instead. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile.h File cc/tiles/tile.h (right): https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile.h#newcod... cc/tiles/tile.h:116: bool raster_task_scheduled_with_checker_images() const { On 2017/03/29 22:10:20, Khushal wrote: > On 2017/03/29 19:11:45, vmpstr wrote: > > Add a setter as well. I want to remove the friend TileManager on the tile > > eventually. Done. > > > > This needs a comment: unlike "solid color analysis performed" which mean we've > > done the analysis, this could be interpreted that the raster task is > _currently_ > > scheduled, vs raster task has _ever_ been scheduled. Also, remove the > > "raster_task" part here. > > > > This also gives me an interesting thought: if we checker an image, can it be > the > > case that the tile afterwards is solid color? I'm not sure it's worth looking > at > > this, but something to consider in the future if we want to save some memory > > while we're decoding the image. > > Its definitely going to be possible and something to optimize. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_draw_info.h File cc/tiles/tile_draw_info.h (right): https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_draw_inf... cc/tiles/tile_draw_info.h:80: bool is_checker_imaged() const { On 2017/03/29 19:11:45, vmpstr wrote: > Just by looking at tile.h and tile_draw_info.h, the two flags you're adding seem > to say very similar things, so I think you need to add a comment explaining the > state. Done. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_draw_inf... cc/tiles/tile_draw_info.h:97: resource_is_checker_imaged_ = false; On 2017/03/29 22:10:20, Khushal wrote: > On 2017/03/29 19:11:45, vmpstr wrote: > > What if we set a resource that _is_ checker imaged? > > > > Can you make this more robust in terms of order of operations? > > > > That is > > info.resource_is_checker_imaged_ = true; > > info.set_resource(resource); > > > > will seem to do the wrong thing here. > > > > Maybe add a bool to set_resource indicating whether the resource was checker > > imaged with a default value of false? > > That makes sense. Done. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:687: // build the decode queue for checkered images. On 2017/03/29 19:11:45, vmpstr wrote: > Add a comment saying that this block must come after the solid color analysis > just for robustness. I don't think anything will break if you reorder them, but > who knows with all the CDL work we're doing how the analysis might change. Done. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:753: prioritized_tile.raster_source()->GetDiscardableImagesInRect( On 2017/03/29 22:10:20, Khushal wrote: > On 2017/03/29 19:11:46, vmpstr wrote: > > This isn't really a trivial operation, is there any way to cache the images? > > I was thinking about it. Is there a reason for not doing this already? We can > store something like image analysis performed on a tile and stash 2 vectors on > it, the ones that the raster tasks need to depend on and the ones which are > checkered. The state shouldn't change for any tile coz we change the > checker-image state only by invalidating tiles and replacing them with new ones. > > This should also remove the |scheduled_draw_images_| map right? Coz if a tile is > scheduled, it always depends on the same set of images? Cached this on Tile. I didn't realize that |scheduled_draw_images_| is still needed in case the tile goes away before the raster task finishes. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:762: CreateRasterTask(prioritized_tile, client_->GetRasterColorSpace(), On 2017/03/29 19:11:46, vmpstr wrote: > It's a bit unfortunate that we're doing this here now. I don't think it really > matters, but it changes what traces tell us about which part of the code is > slow. > > Is there a trace in CreateRasterTask? If not, could you add one? Done. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:795: checker_image_tracker_.ScheduleImageDecodeQueue( On 2017/03/29 19:11:46, vmpstr wrote: > Can you move this call out of this function and do it at a later time? > AssignGpuMemory shouldn't be doing any scheduling. We might need to refactor > tile manager here before you can land this patch, but I'm not a fan of the > coupling we're introducing throughout this function. Moved it to ScheduleTasks. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:840: DCHECK(!images_to_decode_for_raster || images_to_decode_for_raster->empty()); On 2017/03/29 22:10:20, Khushal wrote: > On 2017/03/29 19:11:46, vmpstr wrote: > > It's not clear to me why some of the output parameters are optional. Can we > > avoid this? > > I think computing it once and storing it on tile should simplify this a bunch. Done. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:848: checkered_images->insert(image->uniqueID()); On 2017/03/29 19:11:46, vmpstr wrote: > Here you're adding the image to both the vector and the set. That's something > that the calling code can do, eliminating one output parameter. Function died. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:851: images_to_decode_for_raster->push_back(draw_image); On 2017/03/29 19:11:45, vmpstr wrote: > image to decode for raster is not a clear name. I think this is meant to > basically be "images _not_ to checker" > > Is it possible to make this function a simple partition? > > iterator PartitionImagesForCheckering(tree, images) { > return std::stable_partition(images.begin(), images.end(), [](image) { ... }); > } > > then the calling code will get both sets of images that need checkering and > images that don't, and it can do whatever it wants with that information. > > If it makes the calling code verbose in a few places, then just add another free > function or another member function (ie refactor a bit) Function died. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:904: DCHECK(tile->HasRasterTask()); On 2017/03/29 19:11:46, vmpstr wrote: > comment why this is true now. Done. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:1048: prioritized_tile.raster_source()->GetDiscardableImagesInRect( On 2017/03/29 19:11:46, vmpstr wrote: > The pattern of these two calls appears throughout. I think it's worth adding a > helper function. Happens only once now. Moved to EnsureImageAnalysisPerformed. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:1096: raster_task_scheduled_with_checker_images = On 2017/03/29 19:11:46, vmpstr wrote: > just make the setter return the old value Done. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.h File cc/tiles/tile_manager.h (right): https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tile_manager.... cc/tiles/tile_manager.h:318: void FilterImagesForCheckering( On 2017/03/29 19:11:46, vmpstr wrote: > This needs a comment explaining what each parameter is, since > > images_to_decode_for_raster, checkered_images, and image_decode_queue all seem > to store the same thing. Possibly, this needs to be reworked to move some code > outside of the function. > > In my mind, the output of this should be fewer vectors/sets/queues and the > calling code will then use the vector to populate/append to the queue or > something like that. > > That being said, I haven't looked at the implementation yet and just commenting > on the header. The function was killed. https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tiling_set_ra... File cc/tiles/tiling_set_raster_queue_all.cc (right): https://codereview.chromium.org/2726343004/diff/100001/cc/tiles/tiling_set_ra... cc/tiles/tiling_set_raster_queue_all.cc:211: !tile->draw_info().is_checker_imaged()) On 2017/03/29 19:11:46, vmpstr wrote: > braces and comment Done. https://codereview.chromium.org/2726343004/diff/120001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2726343004/diff/120001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:782: tile->raster_task_scheduled_with_checker_images()) { This part still needs to be worked out.
We discussed this a bit offline, and I think we should move the discussion to a doc, since we exposed a few questions that should be properly designed before putting it in code
I've changed the tile iteration to go only for the Soon bin tiles in the case where we fail to assign memory for these tiles. I think this should take care of correctness at least, to make sure that visible content is eventually invalidated in every case. And we do this only if there are any tiles with checker-images so it should not have any impact when checker-imaging is disabled. Does this look good enough for a first pass? I think we can accommodate other cases to further optimize decode scheduling going forward.
https://codereview.chromium.org/2726343004/diff/140001/cc/tiles/picture_layer... File cc/tiles/picture_layer_tiling.cc (right): https://codereview.chromium.org/2726343004/diff/140001/cc/tiles/picture_layer... cc/tiles/picture_layer_tiling.cc:818: bool PictureLayerTiling::ShouldProcessTileForCheckerImages( This one was to skip tiles on the active tree if they are going to be replaced/occluded upon activation. Since anything we invalidate should be on this pending tree (after it gets activated).
And tested.
I think tile manager changes could use a refactor, but generally this looks good. https://codereview.chromium.org/2726343004/diff/140001/cc/tiles/tile.h File cc/tiles/tile.h (right): https://codereview.chromium.org/2726343004/diff/140001/cc/tiles/tile.h#newcod... cc/tiles/tile.h:170: std::vector<DrawImage> images_to_decode_before_raster_; Don't put this here, because it might be very memory inefficient. https://codereview.chromium.org/2726343004/diff/140001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2726343004/diff/140001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:696: DCHECK(tile->is_image_analysis_performed()); You don't need this, because it might break if kUseColorEstimator or use_picture_analysis is false. https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/checker_image... File cc/tiles/checker_image_tracker.cc (right): https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/checker_image... cc/tiles/checker_image_tracker.cc:43: DCHECK(image_decode_queue.empty() || enable_checker_imaging_); Can you comment to explain the dcheck a bit. https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/checker_image... cc/tiles/checker_image_tracker.cc:137: auto candidate = image_decode_queue_.front(); std::move https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/checker_image... cc/tiles/checker_image_tracker.cc:140: // Once an image has been decoded, they can still be present in the decode s/they/it/ https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/picture_layer... File cc/tiles/picture_layer_tiling.cc (right): https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/picture_layer... cc/tiles/picture_layer_tiling.cc:811: bool PictureLayerTiling::ShouldDecodeCheckeredImagesForTile( None of the checks here really care if the tile is checkered imaged. Should this be named something different? https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/picture_layer... cc/tiles/picture_layer_tiling.cc:814: // decoded for checker-images. Tile isn't "decoded", can you rephrase the comment? https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/picture_layer... cc/tiles/picture_layer_tiling.cc:823: // or we may not have a pending tree. So use the occlusion on the current Can you elaborate on this? https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/picture_layer... cc/tiles/picture_layer_tiling.cc:841: } This function sounds very similar to RequiredForActivation type of thing. Can you explain (in just codereview) what exactly it's doing and how it's different? I'm a little hesitant about adding another one of these types of functions unless it's absolutely necessary. https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/picture_layer... File cc/tiles/picture_layer_tiling.h (right): https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/picture_layer... cc/tiles/picture_layer_tiling.h:108: // Returns true if the tile should be processed for decoding images skipped Can you rephrase this a bit (it sounds confusing to me). Maybe something like // During raster, some tiles maybe have been rasterized with images skipped. We still need to process these tiles to ensure we can retrieve the images that need to be decoded. This function returns true if the given tile is one of these skipped images tiles that need to processed. https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/prioritized_t... File cc/tiles/prioritized_tile.h (right): https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/prioritized_t... cc/tiles/prioritized_tile.h:40: bool should_decode_checkered_images_for_tile() const { This function name is a bit complicated. https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/tile.h File cc/tiles/tile.h (right): https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/tile.h#newcod... cc/tiles/tile.h:122: bool previous_raster_task_scheduled_with_checker_images = just call it 'return_value' or maybe even just make this a void function, since the caller can always call the next function to check the current state if desired. https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/tile_draw_info.h File cc/tiles/tile_draw_info.h (right): https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/tile_draw_inf... cc/tiles/tile_draw_info.h:95: void set_resource(Resource* resource, bool resource_is_checker_imaged) { You could just make false the default value. https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:797: if (!had_enough_memory_to_schedule_tiles_needed_now && Do you have a unittest for this? https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:815: work_to_schedule.checker_image_decode_queue.insert( This block of code appears 3 times, which makes it a good candidate for a helper function. Also, don't seem to use sync_decoded_images at all. Do we need that vector? https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:1088: PartitionImagesForCheckering(prioritized_tile, color_space, This image information is available in the other if branch at the call site. Should we just unconditionally do the partition and then either pass stuff to CreateRasterTask or do whatever the other branch is doing? https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/tiling_set_ra... File cc/tiles/tiling_set_raster_queue_all.cc (right): https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/tiling_set_ra... cc/tiles/tiling_set_raster_queue_all.cc:212: tiling_->ShouldDecodeCheckeredImagesForTile(tile); You can optimize and not call this if tile_is_valid_for_raster.
https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/checker_image... File cc/tiles/checker_image_tracker.cc (right): https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/checker_image... cc/tiles/checker_image_tracker.cc:43: DCHECK(image_decode_queue.empty() || enable_checker_imaging_); On 2017/04/18 00:20:00, vmpstr wrote: > Can you comment to explain the dcheck a bit. Done. https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/checker_image... cc/tiles/checker_image_tracker.cc:137: auto candidate = image_decode_queue_.front(); On 2017/04/18 00:20:00, vmpstr wrote: > std::move Done. https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/checker_image... cc/tiles/checker_image_tracker.cc:140: // Once an image has been decoded, they can still be present in the decode On 2017/04/18 00:20:00, vmpstr wrote: > s/they/it/ Done. https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/picture_layer... File cc/tiles/picture_layer_tiling.cc (right): https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/picture_layer... cc/tiles/picture_layer_tiling.cc:811: bool PictureLayerTiling::ShouldDecodeCheckeredImagesForTile( On 2017/04/18 00:20:00, vmpstr wrote: > None of the checks here really care if the tile is checkered imaged. Should this > be named something different? Answered above. https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/picture_layer... cc/tiles/picture_layer_tiling.cc:814: // decoded for checker-images. On 2017/04/18 00:20:00, vmpstr wrote: > Tile isn't "decoded", can you rephrase the comment? Done. https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/picture_layer... cc/tiles/picture_layer_tiling.cc:823: // or we may not have a pending tree. So use the occlusion on the current On 2017/04/18 00:20:00, vmpstr wrote: > Can you elaborate on this? The check is for making sure that we don't process active tree tiles that will be replaced on activation. Right now if there is no twin tiling, it could either be that there is no pending tree or that the tiling will be evicted. Still needs plumbing to differentiate between the 2 cases. https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/picture_layer... cc/tiles/picture_layer_tiling.cc:841: } On 2017/04/18 00:20:00, vmpstr wrote: > This function sounds very similar to RequiredForActivation type of thing. > > Can you explain (in just codereview) what exactly it's doing and how it's > different? I'm a little hesitant about adding another one of these types of > functions unless it's absolutely necessary. Its similar to RequiredForActivation for the part that figures out whether a tile on the active tree will be replaced on the pending tree. In which case, we don't require this tile for activation and also we shouldn't need to decode images for it since it will not be present on the next active tree. But the other checks concerning |can_require_tiles_for_activation_| and whether the tile is in the |current_visible_rect_| don't apply. If a tile will not be replaced, then we only want to check whether it will be occluded after activation. This part is somewhat similar to IsTileOccluded. Except IsTileOccluded will also consider tiles unoccluded on the active tree irrespective of their pending tree state (since these tiles do need to be rasterized). But for the checkered images, can avoid decoding images for such tiles. Do you think this might be a premature optimization and we won't have a lot of such cases? https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/picture_layer... File cc/tiles/picture_layer_tiling.h (right): https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/picture_layer... cc/tiles/picture_layer_tiling.h:108: // Returns true if the tile should be processed for decoding images skipped On 2017/04/18 00:20:00, vmpstr wrote: > Can you rephrase this a bit (it sounds confusing to me). > > Maybe something like > > // During raster, some tiles maybe have been rasterized with images skipped. We > still need to process these tiles to ensure we can retrieve the images that need > to be decoded. This function returns true if the given tile is one of these > skipped images tiles that need to processed. Its actually not necessary that images were skipped for this tile. Its possible for a tile on the active tree to become visible while there is a pending tree tile that will replace it. In which case, we still skip the image for the active tree tile, but we don't decode it because it won't be present after activation. That's why I went with ShouldDecodeCheckeredImageForTile, whether we need to decode the images already skipped, or to be skipped, for this tile. https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/tile.h File cc/tiles/tile.h (right): https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/tile.h#newcod... cc/tiles/tile.h:122: bool previous_raster_task_scheduled_with_checker_images = On 2017/04/18 00:20:00, vmpstr wrote: > just call it 'return_value' or maybe even just make this a void function, since > the caller can always call the next function to check the current state if > desired. Changed it to |previous_value|. The call-site was initially using the accessor but I changed it to return the value from the setter after you suggested that in a previous patchset. :P I guess it does look prettier this way. I'm good with either, let me know if you have a preference. https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/tile_draw_info.h File cc/tiles/tile_draw_info.h (right): https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/tile_draw_inf... cc/tiles/tile_draw_info.h:95: void set_resource(Resource* resource, bool resource_is_checker_imaged) { On 2017/04/18 00:20:00, vmpstr wrote: > You could just make false the default value. Do we have default values for arguments? I never noticed it in chromium code. https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:797: if (!had_enough_memory_to_schedule_tiles_needed_now && On 2017/04/18 00:20:00, vmpstr wrote: > Do you have a unittest for this? Done. AddsAllNowTilesToImageDecodeQueue in tile_manager_unittest.cc takes care of this. https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:815: work_to_schedule.checker_image_decode_queue.insert( On 2017/04/18 00:20:00, vmpstr wrote: > This block of code appears 3 times, which makes it a good candidate for a helper > function. Also, don't seem to use sync_decoded_images at all. Do we need that > vector? Moved it to AddCheckeredImagesToDecodeQueue. In the cases where we only need to re-add the decoded images to the queue, that makes it clearer. We don't need the sync_decoded_images for these cases. https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:1088: PartitionImagesForCheckering(prioritized_tile, color_space, On 2017/04/18 00:20:00, vmpstr wrote: > This image information is available in the other if branch at the call site. > Should we just unconditionally do the partition and then either pass stuff to > CreateRasterTask or do whatever the other branch is doing? I thought we should avoid it for low resolution tiles since its not needed. Wdyt? https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/tiling_set_ra... File cc/tiles/tiling_set_raster_queue_all.cc (right): https://codereview.chromium.org/2726343004/diff/160001/cc/tiles/tiling_set_ra... cc/tiles/tiling_set_raster_queue_all.cc:212: tiling_->ShouldDecodeCheckeredImagesForTile(tile); On 2017/04/18 00:20:00, vmpstr wrote: > You can optimize and not call this if tile_is_valid_for_raster. Done.
ping.
mostly nits below, so this lgtm. Even though checker imaging isn't enabled yet, this patch has some code that will run regardless, so please keep track of the performance impact that might show up here. https://codereview.chromium.org/2726343004/diff/180001/cc/test/fake_recording... File cc/test/fake_recording_source.h (right): https://codereview.chromium.org/2726343004/diff/180001/cc/test/fake_recording... cc/test/fake_recording_source.h:67: void set_fill_with_nonsolid_color(bool nonsolid) { nit: non_solid https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/checker_image... File cc/tiles/checker_image_tracker_unittest.cc (right): https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/checker_image... cc/tiles/checker_image_tracker_unittest.cc:267: std::vector<DrawImage> draw_images; std::vector<DrawImage> draw_images = {checkerable_image1, checkerable_image2};? https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/checker_image... cc/tiles/checker_image_tracker_unittest.cc:288: draw_images.push_back(checkerable_image1); nit: remove the clear, and then = std::vector<DrawImage>{..., ...} https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/checker_image... cc/tiles/checker_image_tracker_unittest.cc:300: EXPECT_EQ(image_controller_.decodes_requested().size(), 1U); Verify which one like above https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/checker_image... cc/tiles/checker_image_tracker_unittest.cc:309: EXPECT_EQ(image_controller_.num_of_locked_images(), 2); Do you need to unlock anything at shutdown or will it unlock itself somewhere? https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/picture_layer... File cc/tiles/picture_layer_tiling.cc (right): https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/picture_layer... cc/tiles/picture_layer_tiling.cc:811: bool PictureLayerTiling::ShouldDecodeCheckeredImagesForTile( If we did end up having images on a tile earlier than we do now, then a lot of this work could be skipped for tiles with no images at all.. Just a thought for possible future improvements. https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/tile_draw_info.h File cc/tiles/tile_draw_info.h (right): https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/tile_draw_inf... cc/tiles/tile_draw_info.h:95: void set_resource(Resource* resource, bool resource_is_checker_imaged) { We have default values (this is test, but still https://cs.chromium.org/chromium/src/cc/test/fake_tile_manager.h?l=19) It's not very common since it used to be disallowed. Up to you whether you want to use a default value here though. https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:800: if (tile->draw_info().is_checker_imaged() || Is this going to be false for tiles that don't have a resource at all? A common case for when we'll hit this is if the memory policy is ALLOW_NOTHING in which case the memory budget is exactly 0 (and we'll evict everything that we can). In that case, I don't think we want to decode stuff, do we? You should be able to trigger this behavior by making a tab hidden. As a random thought, I think this performs ok with partial raster as well right? Like what happens if we have a checkered resource in the resource pool and something gets invalidated. we'll grab that resource. I think it's ok, since either that tile still needs to be checker imaged or we have already invalidated due to image decode. But can you confirm? https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:870: void TileManager::AddCheckeredImagesToDecodeQueue( nit: I guess I was just hoping to reuse the function above but make sync_decoded_images be able to be nullptr (and checkered_images too). Up to you though, I think this fine as well.
https://codereview.chromium.org/2726343004/diff/180001/cc/test/fake_recording... File cc/test/fake_recording_source.h (right): https://codereview.chromium.org/2726343004/diff/180001/cc/test/fake_recording... cc/test/fake_recording_source.h:67: void set_fill_with_nonsolid_color(bool nonsolid) { On 2017/04/20 20:59:32, vmpstr wrote: > nit: non_solid I just made it match what was already on |client_|... https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/checker_image... File cc/tiles/checker_image_tracker_unittest.cc (right): https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/checker_image... cc/tiles/checker_image_tracker_unittest.cc:267: std::vector<DrawImage> draw_images; On 2017/04/20 20:59:32, vmpstr wrote: > std::vector<DrawImage> draw_images = {checkerable_image1, checkerable_image2};? Done. https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/checker_image... cc/tiles/checker_image_tracker_unittest.cc:288: draw_images.push_back(checkerable_image1); On 2017/04/20 20:59:32, vmpstr wrote: > nit: remove the clear, and then = std::vector<DrawImage>{..., ...} Done. https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/checker_image... cc/tiles/checker_image_tracker_unittest.cc:300: EXPECT_EQ(image_controller_.decodes_requested().size(), 1U); On 2017/04/20 20:59:32, vmpstr wrote: > Verify which one like above Done. https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/checker_image... cc/tiles/checker_image_tracker_unittest.cc:309: EXPECT_EQ(image_controller_.num_of_locked_images(), 2); On 2017/04/20 20:59:32, vmpstr wrote: > Do you need to unlock anything at shutdown or will it unlock itself somewhere? The tracker unlocks everything in the dtor. There is a DCHECK in the TestImageController that makes sure there is nothing locked in its dtor, since the tracker should have been destroyed first. https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/picture_layer... File cc/tiles/picture_layer_tiling.cc (right): https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/picture_layer... cc/tiles/picture_layer_tiling.cc:811: bool PictureLayerTiling::ShouldDecodeCheckeredImagesForTile( On 2017/04/20 20:59:32, vmpstr wrote: > If we did end up having images on a tile earlier than we do now, then a lot of > this work could be skipped for tiles with no images at all.. Just a thought for > possible future improvements. May we could store an enum on Tile indicating whether it has been analyzed for images and if it has any. This will at least avoid this for repeated runs on the same tile. https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/tile_draw_info.h File cc/tiles/tile_draw_info.h (right): https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/tile_draw_inf... cc/tiles/tile_draw_info.h:95: void set_resource(Resource* resource, bool resource_is_checker_imaged) { On 2017/04/20 20:59:32, vmpstr wrote: > We have default values (this is test, but still > https://cs.chromium.org/chromium/src/cc/test/fake_tile_manager.h?l=19) > > It's not very common since it used to be disallowed. Up to you whether you want > to use a default value here though. Acknowledged. https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:800: if (tile->draw_info().is_checker_imaged() || On 2017/04/20 20:59:33, vmpstr wrote: > Is this going to be false for tiles that don't have a resource at all? Yup. > > A common case for when we'll hit this is if the memory policy is ALLOW_NOTHING > in which case the memory budget is exactly 0 (and we'll evict everything that we > can). In that case, I don't think we want to decode stuff, do we? You should be > able to trigger this behavior by making a tab hidden. > If the memory policy is ALLOW_NOTHING, then |had_enough_memory_to_schedule_tiles_needed_now| should be false, since we will break at line 658, not 748. So we won't decode anything in that case. The BuildsImageDecodeQueue test in tile_manager_unittest takes care of this. > > > As a random thought, I think this performs ok with partial raster as well right? > Like what happens if we have a checkered resource in the resource pool and > something gets invalidated. we'll grab that resource. I think it's ok, since > either that tile still needs to be checker imaged or we have already invalidated > due to image decode. But can you confirm? Yup, that's exactly what will be the case. Whether the resource is valid or not depends on when the state for skipping an image on it changes. And any change in this state has to be accompanied with tiles for it being invalidated. https://codereview.chromium.org/2726343004/diff/180001/cc/tiles/tile_manager.... cc/tiles/tile_manager.cc:870: void TileManager::AddCheckeredImagesToDecodeQueue( On 2017/04/20 20:59:32, vmpstr wrote: > nit: I guess I was just hoping to reuse the function above but make > sync_decoded_images be able to be nullptr (and checkered_images too). Up to you > though, I think this fine as well. Its a few more lines of code extra at call-sites for iterating through the checker-imaged tile and adding to queue. Looked like it could use its own function.
The CQ bit was checked by khushalsagar@chromium.org
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/2726343004/#ps200001 (title: "addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by khushalsagar@chromium.org
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/2726343004/#ps220001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by khushalsagar@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1492746395825530,
"parent_rev": "cdb8d85e00d3406b80e06c5540791bac335d2176", "commit_rev":
"3e256c223b5c4057b6a9e0556041cdb8ba11c029"}
Message was sent while issue was closed.
Description was changed from ========== cc: Optimize decode scheduling for checker-images. The CheckerImageTracker currently schedules image decodes for all checkered images as we schedule raster work for the dependent tiles. Once image decodes have been submitted to the tracker, they can not be cancelled and the order can not be changed. This means that as tile priorities change, the order in which images are decoded may not be aligned with the priority of the tiles which depend on them. Instead use a queue for scheduling decode work in the tracker. Only one outstanding decode is pending with the decode service at a time, and the queue is re-build each time we schedule work for a new set of tiles. This requires plumbing through the checker-imaged tiles in the RasterWorkerQueue since they are now inter-leaved with tiles that actually need to be rasterized. BUG=689184 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Optimize decode scheduling for checker-images. The CheckerImageTracker currently schedules image decodes for all checkered images as we schedule raster work for the dependent tiles. Once image decodes have been submitted to the tracker, they can not be cancelled and the order can not be changed. This means that as tile priorities change, the order in which images are decoded may not be aligned with the priority of the tiles which depend on them. Instead use a queue for scheduling decode work in the tracker. Only one outstanding decode is pending with the decode service at a time, and the queue is re-build each time we schedule work for a new set of tiles. This requires plumbing through the checker-imaged tiles in the RasterWorkerQueue since they are now inter-leaved with tiles that actually need to be rasterized. BUG=689184 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2726343004 Cr-Commit-Position: refs/heads/master@{#466261} Committed: https://chromium.googlesource.com/chromium/src/+/3e256c223b5c4057b6a9e0556041... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/3e256c223b5c4057b6a9e0556041... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
