Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
246673005:
cc: Start using raster/eviction iterators in tile manager (Closed)
|
Created:
6 years, 8 months ago by vmpstr Modified:
6 years, 6 months ago CC:
chromium-reviews, cc-bugs_chromium.org, jbedley, ccameron, danakj Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptioncc: Start using raster/eviction iterators in tile manager
This patch is a part of the series that enables raster
and eviction iterators in cc. In particular, this patch
actually starts using the iterators that have landed
previously. There should be a perf improvement for the
manage tiles case. Other than that, there should be no perf
impact.
This patch's main contribution is that it opens the door for
more optimizations to be done in the future. As well, it
simplifies the logic we have in tile manager.
BUG=329686
R=enne, reveman
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278654
Patch Set 1 #
Total comments: 16
Patch Set 2 : review #
Total comments: 11
Patch Set 3 : #Patch Set 4 : #
Total comments: 25
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 4
Patch Set 8 : #Patch Set 9 : rebase #Patch Set 10 : #
Total comments: 1
Patch Set 11 : rebase #Patch Set 12 : #
Total comments: 21
Patch Set 13 : #Patch Set 14 : update #
Total comments: 4
Patch Set 15 : update #
Total comments: 14
Patch Set 16 : update #
Total comments: 21
Patch Set 17 : update #Patch Set 18 : #
Total comments: 24
Patch Set 19 : update #
Total comments: 4
Patch Set 20 : update #Patch Set 21 : #
Total comments: 36
Patch Set 22 : update #
Total comments: 8
Patch Set 23 : update #
Total comments: 12
Patch Set 24 : update #
Total comments: 2
Patch Set 25 : update #Patch Set 26 : updated BUILD.gn #
Messages
Total messages: 46 (0 generated)
Hey, I think this is ready for a first round of comments. Several things are still missing: 1. Unittest fixes 2. Perftest fixes 3. memory limit policy (ALLOW_PREPAINT_ONLY, ALLOW_ABSOLUTE_MINIMUM, etc). For #3, I think it's just a matter of earlying out if we reach a tile that doesn't mean the criteria from the AssignGpuMemory loop. One question I had, which you guys might know, is whether this policy is meant to release tiles that don't match this criteria as well or simply not schedule new work? Exciting part is that this seems to display pages correctly (including under memory pressure). In order to run some reasonable benchmarks to see the performance, I think I will need to at least respect the memory policy, as some of our benchmarks set this to ALLOW_ABSOLUTE_MINIMUM.
This all looks pretty reasonable to me at first blush. Excited to see what perf tests look like. :)
https://codereview.chromium.org/246673005/diff/1/cc/resources/picture_layer_t... File cc/resources/picture_layer_tiling.h (right): https://codereview.chromium.org/246673005/diff/1/cc/resources/picture_layer_t... cc/resources/picture_layer_tiling.h:62: bool HasTilesRequiredForActivation() const; hm, this returns some state that depend on the position of the iterator and all following elements. I think it's a bad idea to have such a function on an iterator. If we can't land some change prior to this that moves the decision whether we can activate to tile initialization completion time, could we instead add a TilesRequiredForActivationAndNeedRasterCount() function to the PictureLayerTiling class and use that in the tile manager instead? https://codereview.chromium.org/246673005/diff/1/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/1/cc/resources/tile_manager.cc... cc/resources/tile_manager.cc:352: void TileManager::DidChangeTilePriority(Tile* tile) { Can we remove this function now? https://codereview.chromium.org/246673005/diff/1/cc/resources/tile_manager.cc... cc/resources/tile_manager.cc:458: if (state != global_state_) I don't think we need this anymore :) Please remove the GlobalStateThatImpactsTilePriority compare operator too if possible. https://codereview.chromium.org/246673005/diff/1/cc/resources/tile_manager.cc... cc/resources/tile_manager.cc:463: CleanUpReleasedTiles(); No need to do anything in this patch but we should align this with the need to keep raster tasks around. Keeping tiles around instead could provide a significant performance improvement. https://codereview.chromium.org/246673005/diff/1/cc/resources/tile_manager.cc... cc/resources/tile_manager.cc:583: bool eviction_iterator_created = false; Would it be too expensive to create a new eviction iterator every time it's needed? I would prefer that but if that's hard to make efficient then could we at least make the ctor cheap (maybe delay some of the work until used) so we can just create it here and remove eviction_iterator_created? https://codereview.chromium.org/246673005/diff/1/cc/resources/tile_manager.cc... cc/resources/tile_manager.cc:625: int& resources_left = resources_allocatable; nit: "int*" instead https://codereview.chromium.org/246673005/diff/1/cc/resources/tile_manager.cc... cc/resources/tile_manager.cc:661: int64& tile_bytes_left = nit: int64* https://codereview.chromium.org/246673005/diff/1/cc/resources/tile_manager.cc... cc/resources/tile_manager.cc:767: all_tiles_required_for_activation_have_memory_ = As discussed, I'd like to see this removed and instead determine whether all tiles required for activation have been initialized when they complete rather than here when tasks are scheduled for raster.
I've updated the patch, but I still have to rebase on top of https://codereview.chromium.org/257773009/ and https://codereview.chromium.org/251003002/ when those land. https://codereview.chromium.org/246673005/diff/1/cc/resources/picture_layer_t... File cc/resources/picture_layer_tiling.h (right): https://codereview.chromium.org/246673005/diff/1/cc/resources/picture_layer_t... cc/resources/picture_layer_tiling.h:62: bool HasTilesRequiredForActivation() const; On 2014/04/24 00:17:34, reveman wrote: > hm, this returns some state that depend on the position of the iterator and all > following elements. I think it's a bad idea to have such a function on an > iterator. If we can't land some change prior to this that moves the decision > whether we can activate to tile initialization completion time, could we instead > add a TilesRequiredForActivationAndNeedRasterCount() function to the > PictureLayerTiling class and use that in the tile manager instead? Moved this to a separate patch. https://codereview.chromium.org/246673005/diff/1/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/1/cc/resources/tile_manager.cc... cc/resources/tile_manager.cc:352: void TileManager::DidChangeTilePriority(Tile* tile) { On 2014/04/24 00:17:34, reveman wrote: > Can we remove this function now? Done. https://codereview.chromium.org/246673005/diff/1/cc/resources/tile_manager.cc... cc/resources/tile_manager.cc:458: if (state != global_state_) On 2014/04/24 00:17:34, reveman wrote: > I don't think we need this anymore :) Please remove the > GlobalStateThatImpactsTilePriority compare operator too if possible. Done. https://codereview.chromium.org/246673005/diff/1/cc/resources/tile_manager.cc... cc/resources/tile_manager.cc:463: CleanUpReleasedTiles(); On 2014/04/24 00:17:34, reveman wrote: > No need to do anything in this patch but we should align this with the need to > keep raster tasks around. Keeping tiles around instead could provide a > significant performance improvement. Sounds good. https://codereview.chromium.org/246673005/diff/1/cc/resources/tile_manager.cc... cc/resources/tile_manager.cc:583: bool eviction_iterator_created = false; On 2014/04/24 00:17:34, reveman wrote: > Would it be too expensive to create a new eviction iterator every time it's > needed? I would prefer that but if that's hard to make efficient then could we > at least make the ctor cheap (maybe delay some of the work until used) so we can > just create it here and remove eviction_iterator_created? Yeah, we can move the initialization code outside of the constructor. Done. https://codereview.chromium.org/246673005/diff/1/cc/resources/tile_manager.cc... cc/resources/tile_manager.cc:625: int& resources_left = resources_allocatable; On 2014/04/24 00:17:34, reveman wrote: > nit: "int*" instead Done. https://codereview.chromium.org/246673005/diff/1/cc/resources/tile_manager.cc... cc/resources/tile_manager.cc:661: int64& tile_bytes_left = On 2014/04/24 00:17:34, reveman wrote: > nit: int64* Done. https://codereview.chromium.org/246673005/diff/1/cc/resources/tile_manager.cc... cc/resources/tile_manager.cc:767: all_tiles_required_for_activation_have_memory_ = On 2014/04/24 00:17:34, reveman wrote: > As discussed, I'd like to see this removed and instead determine whether all > tiles required for activation have been initialized when they complete rather > than here when tasks are scheduled for raster. Moved to a separate patch.
https://codereview.chromium.org/246673005/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/246673005/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:41: bool HasTilesRequiredForActivation() const; This will go away after rebase, right? https://codereview.chromium.org/246673005/diff/20001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/20001/cc/resources/tile_manage... cc/resources/tile_manager.cc:500: *memory_nice_to_have_bytes = resource_pool_->total_memory_usage_bytes(); what's our plan for computing these properly? https://codereview.chromium.org/246673005/diff/20001/cc/resources/tile_manage... cc/resources/tile_manager.cc:570: while (hard_bytes_available < 0 || resources_available < 0) { can you fold this into the loop below instead of duplicating all the logic here?
https://codereview.chromium.org/246673005/diff/20001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/246673005/diff/20001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:41: bool HasTilesRequiredForActivation() const; On 2014/04/28 19:39:18, reveman wrote: > This will go away after rebase, right? Correct. https://codereview.chromium.org/246673005/diff/20001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/20001/cc/resources/tile_manage... cc/resources/tile_manager.cc:500: *memory_nice_to_have_bytes = resource_pool_->total_memory_usage_bytes(); On 2014/04/28 19:39:18, reveman wrote: > what's our plan for computing these properly? That's a good question. There's no good way for us to know what is nice to have at this point, short of either querying the iterators (like required for activation is currently is in this patch), iterating the raster iterators all the way to the end, or somehow asking layers for this information. Suggestions are more than welcome. For the required bytes, we can estimate the usage based on required for activation counts, but it would not account for active layer visible tiles. https://codereview.chromium.org/246673005/diff/20001/cc/resources/tile_manage... cc/resources/tile_manager.cc:570: while (hard_bytes_available < 0 || resources_available < 0) { On 2014/04/28 19:39:18, reveman wrote: > can you fold this into the loop below instead of duplicating all the logic here? It has to be outside of the loop since the raster tile iterator might return false immediately meaning we don't enter the loop at all. I can try to make this a separate function called here and in the loop, but the difference is that here we unconditionally evict to get into the budget, whereas in the loop we evict as long as the eviction tile priority is less than that of the raster tile. The signature for the function would be something like the following: // Returns true if evicted tiles required for activation bool TileManager::EvictTiles( EvictionTileIterator* eviction_iterator, int64 extra_memory_needed, int extra_resources_needed, const TilePriority& raster_priority, bool evict_unconditionally); with the logic being similar to this with something like the following: if (!evict_unconditionally && !raster_priority.IsHigherPriorityThan(eviction_priority)) break; Does that sound reasonable?
On 2014/04/28 20:56:42, vmpstr wrote: > https://codereview.chromium.org/246673005/diff/20001/cc/layers/picture_layer_... > File cc/layers/picture_layer_impl.h (right): > > https://codereview.chromium.org/246673005/diff/20001/cc/layers/picture_layer_... > cc/layers/picture_layer_impl.h:41: bool HasTilesRequiredForActivation() const; > On 2014/04/28 19:39:18, reveman wrote: > > This will go away after rebase, right? > > Correct. > > https://codereview.chromium.org/246673005/diff/20001/cc/resources/tile_manage... > File cc/resources/tile_manager.cc (right): > > https://codereview.chromium.org/246673005/diff/20001/cc/resources/tile_manage... > cc/resources/tile_manager.cc:500: *memory_nice_to_have_bytes = > resource_pool_->total_memory_usage_bytes(); > On 2014/04/28 19:39:18, reveman wrote: > > what's our plan for computing these properly? > > That's a good question. There's no good way for us to know what is nice to have > at this point, short of either querying the iterators (like required for > activation is currently is in this patch), iterating the raster iterators all > the way to the end, or somehow asking layers for this information. Suggestions > are more than welcome. > > For the required bytes, we can estimate the usage based on required for > activation counts, but it would not account for active layer visible tiles. > > https://codereview.chromium.org/246673005/diff/20001/cc/resources/tile_manage... > cc/resources/tile_manager.cc:570: while (hard_bytes_available < 0 || > resources_available < 0) { > On 2014/04/28 19:39:18, reveman wrote: > > can you fold this into the loop below instead of duplicating all the logic > here? > > It has to be outside of the loop since the raster tile iterator might return > false immediately meaning we don't enter the loop at all. I can try to make this > a separate function called here and in the loop, but the difference is that here > we unconditionally evict to get into the budget, whereas in the loop we evict as > long as the eviction tile priority is less than that of the raster tile. > > The signature for the function would be something like the following: > > // Returns true if evicted tiles required for activation > bool TileManager::EvictTiles( > EvictionTileIterator* eviction_iterator, > int64 extra_memory_needed, > int extra_resources_needed, > const TilePriority& raster_priority, > bool evict_unconditionally); > > with the logic being similar to this with something like the following: > > if (!evict_unconditionally && > !raster_priority.IsHigherPriorityThan(eviction_priority)) > break; > > Does that sound reasonable? It might actually be even more awkward, since I need to pass some combination of memory/resources available pointers and memory/resources needed...
https://codereview.chromium.org/246673005/diff/20001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/20001/cc/resources/tile_manage... cc/resources/tile_manager.cc:500: *memory_nice_to_have_bytes = resource_pool_->total_memory_usage_bytes(); On 2014/04/28 20:56:42, vmpstr wrote: > On 2014/04/28 19:39:18, reveman wrote: > > what's our plan for computing these properly? > > That's a good question. There's no good way for us to know what is nice to have > at this point, short of either querying the iterators (like required for > activation is currently is in this patch), iterating the raster iterators all > the way to the end, or somehow asking layers for this information. Suggestions > are more than welcome. > > For the required bytes, we can estimate the usage based on required for > activation counts, but it would not account for active layer visible tiles. The end result is that these values are used by the global gpu memory manager to determine how much memory we should have. "Required for activation" is something temporary and should probably not be used to determine any of these values. I don't think the tile manager should be involved in this anymore. How about we compute all this in UpdateTilePriorities to the best of our ability? nice_to_have_bytes might be hard to compute accurately but we can probably produce some number that is relevant to the current page.. https://codereview.chromium.org/246673005/diff/20001/cc/resources/tile_manage... cc/resources/tile_manager.cc:570: while (hard_bytes_available < 0 || resources_available < 0) { On 2014/04/28 20:56:42, vmpstr wrote: > On 2014/04/28 19:39:18, reveman wrote: > > can you fold this into the loop below instead of duplicating all the logic > here? > > It has to be outside of the loop since the raster tile iterator might return > false immediately meaning we don't enter the loop at all. I can try to make this > a separate function called here and in the loop, but the difference is that here > we unconditionally evict to get into the budget, whereas in the loop we evict as > long as the eviction tile priority is less than that of the raster tile. > > The signature for the function would be something like the following: > > // Returns true if evicted tiles required for activation > bool TileManager::EvictTiles( > EvictionTileIterator* eviction_iterator, > int64 extra_memory_needed, > int extra_resources_needed, > const TilePriority& raster_priority, > bool evict_unconditionally); > > with the logic being similar to this with something like the following: > > if (!evict_unconditionally && > !raster_priority.IsHigherPriorityThan(eviction_priority)) > break; > > Does that sound reasonable? It's not clear to me why these eviction cases need to be considered different. Maybe some pseudo code that describes the general process here would help. I'm thinking that what we want here is something like the following but I'm probably missing something: RasterTileIterator raster_it = .. EvictionTileIterator eviction_it = .. do { bool needs_eviction = resources_available < 0 || ... if (needs_eviction) { if (!eviction_it) break; // Eviction code... ++eviction_it; continue; } // Assign gpu memory code... ++raster_it; } while (raster_it);
PTAL https://codereview.chromium.org/246673005/diff/20001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/20001/cc/resources/tile_manage... cc/resources/tile_manager.cc:500: *memory_nice_to_have_bytes = resource_pool_->total_memory_usage_bytes(); On 2014/04/28 21:43:15, reveman wrote: > On 2014/04/28 20:56:42, vmpstr wrote: > > On 2014/04/28 19:39:18, reveman wrote: > > > what's our plan for computing these properly? > > > > That's a good question. There's no good way for us to know what is nice to > have > > at this point, short of either querying the iterators (like required for > > activation is currently is in this patch), iterating the raster iterators all > > the way to the end, or somehow asking layers for this information. Suggestions > > are more than welcome. > > > > For the required bytes, we can estimate the usage based on required for > > activation counts, but it would not account for active layer visible tiles. > > The end result is that these values are used by the global gpu memory manager to > determine how much memory we should have. "Required for activation" is something > temporary and should probably not be used to determine any of these values. > > I don't think the tile manager should be involved in this anymore. How about we > compute all this in UpdateTilePriorities to the best of our ability? > nice_to_have_bytes might be hard to compute accurately but we can probably > produce some number that is relevant to the current page.. Yeah that's the thing that always made me a bit hesitant: I'm not sure how exact these values need to be. As it stands now, I can say how much memory is being used and maybe if the next tile we didn't rasterize was prepaint or visible. Would this be sufficient to just say nice to have is "currently used + some buffer for what we know we still didn't do"? Or does this need to be more like "currently used + number_of_tiles_left_to_do * tile_bytes for those tiles"? https://codereview.chromium.org/246673005/diff/20001/cc/resources/tile_manage... cc/resources/tile_manager.cc:570: while (hard_bytes_available < 0 || resources_available < 0) { On 2014/04/28 21:43:15, reveman wrote: > On 2014/04/28 20:56:42, vmpstr wrote: > > On 2014/04/28 19:39:18, reveman wrote: > > > can you fold this into the loop below instead of duplicating all the logic > > here? > > > > It has to be outside of the loop since the raster tile iterator might return > > false immediately meaning we don't enter the loop at all. I can try to make > this > > a separate function called here and in the loop, but the difference is that > here > > we unconditionally evict to get into the budget, whereas in the loop we evict > as > > long as the eviction tile priority is less than that of the raster tile. > > > > The signature for the function would be something like the following: > > > > // Returns true if evicted tiles required for activation > > bool TileManager::EvictTiles( > > EvictionTileIterator* eviction_iterator, > > int64 extra_memory_needed, > > int extra_resources_needed, > > const TilePriority& raster_priority, > > bool evict_unconditionally); > > > > with the logic being similar to this with something like the following: > > > > if (!evict_unconditionally && > > !raster_priority.IsHigherPriorityThan(eviction_priority)) > > break; > > > > Does that sound reasonable? > > It's not clear to me why these eviction cases need to be considered different. > Maybe some pseudo code that describes the general process here would help. I'm > thinking that what we want here is something like the following but I'm probably > missing something: > > RasterTileIterator raster_it = .. > EvictionTileIterator eviction_it = .. > do { > bool needs_eviction = resources_available < 0 || ... > > if (needs_eviction) { > if (!eviction_it) > break; > > // Eviction code... > ++eviction_it; > continue; > } > > // Assign gpu memory code... > ++raster_it; > } while (raster_it); I kind of reworked this a bit (I put the memory limits / budgets into a struct). I like the solution I have right now, since it's fairly compact and easy to reason about. I don't like making the main loop here a do-while, because the first iteration would basically just have "if (!raster_it) break" in the middle of it. That is, in the cases where we don't have a single raster tile, we need to not run the assign gpu memory code. This, to me, is a red flag that a do-while is not the right approach...
David, can you take another look at this? I'm going to rebase this on top of your activation patch, so please ignore the "HasTilesRequiredForActivation" stuff for now.
How about you rebase this onto https://codereview.chromium.org/287643004/ and we land that first? https://codereview.chromium.org/246673005/diff/20001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/20001/cc/resources/tile_manage... cc/resources/tile_manager.cc:500: *memory_nice_to_have_bytes = resource_pool_->total_memory_usage_bytes(); On 2014/04/28 22:44:38, vmpstr wrote: > On 2014/04/28 21:43:15, reveman wrote: > > On 2014/04/28 20:56:42, vmpstr wrote: > > > On 2014/04/28 19:39:18, reveman wrote: > > > > what's our plan for computing these properly? > > > > > > That's a good question. There's no good way for us to know what is nice to > > have > > > at this point, short of either querying the iterators (like required for > > > activation is currently is in this patch), iterating the raster iterators > all > > > the way to the end, or somehow asking layers for this information. > Suggestions > > > are more than welcome. > > > > > > For the required bytes, we can estimate the usage based on required for > > > activation counts, but it would not account for active layer visible tiles. > > > > The end result is that these values are used by the global gpu memory manager > to > > determine how much memory we should have. "Required for activation" is > something > > temporary and should probably not be used to determine any of these values. > > > > I don't think the tile manager should be involved in this anymore. How about > we > > compute all this in UpdateTilePriorities to the best of our ability? > > nice_to_have_bytes might be hard to compute accurately but we can probably > > produce some number that is relevant to the current page.. > > Yeah that's the thing that always made me a bit hesitant: I'm not sure how exact > these values need to be. As it stands now, I can say how much memory is being > used and maybe if the next tile we didn't rasterize was prepaint or visible. > Would this be sufficient to just say nice to have is "currently used + some > buffer for what we know we still didn't do"? Or does this need to be more like > "currently used + number_of_tiles_left_to_do * tile_bytes for those tiles"? Doesn't have to be precise. I don't think memory_required_bytes and memory_nice_to_have_bytes should at all be based on how much we currently use (initialized or with pending raster tasks). https://codereview.chromium.org/246673005/diff/60001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/246673005/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:42: bool HasTilesRequiredForActivation() const; Everything related to HasTilesRequiredForActivation is unnecessary once https://codereview.chromium.org/287643004/ lands, right? https://codereview.chromium.org/246673005/diff/60001/cc/resources/picture_lay... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/246673005/diff/60001/cc/resources/picture_lay... cc/resources/picture_layer_tiling.cc:800: DCHECK(it->second); I don't think this is worthwhile unless we actually set it->second to NULL somewhere? https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager.cc:549: bool TileManager::EvictTiles(EvictionTileIterator* eviction_iterator, Could you rename this in a way that makes it more clear what it does? FreeTileResourcesUntilUsageIsWithinBudget? And maybe return true on success, false otherwise. https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager.cc:551: MemoryBudget* current_budget, I think it's a bit confusing with a budget that is being adjusted. It would be much easier to understand if we were reducing our usage until we're within a budget. What would it take to change that? https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager.cc:552: const TilePriority& raster_priority, "max_priority"? https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager.cc:553: bool evict_unconditionally) { Instead of having evict_unconditionally, could you pass the highest possible priority as max_priority? https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager.cc:555: while (required_budget.Exceeds(*current_budget)) { Would it be a bit cleaner and more consistent with the raster iterator loop if you have a for loop here that advance the iterator and instead early out of the loop when within budget? https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager.cc:567: break; it would be nice if you flip this around and make it: if (priority.IsHigherPriorityThan(max_priority)) break; max_priority is not the same as |raster_priority| so this might not work if we really need this to not pass if priority is equal. Which brings me to: Are we currently allowing a "distance_to_visible - std::numeric_limits<float>::epsilon()" tile that need raster to evict an initialized tile with "distance_to_visible"? And in that case, do we want that? Should we be worried that some floating point inexactness will cause us to alternative between tiles and raster forever? Would it make sense to define a minimal TilePriority delta that is required between raster and eviction? https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager.cc:576: current_budget->soft_memory_bytes += eviction_bytes_if_allocated; Hard to see if this is always doing the right thing. I think it would be easier to understand if this function was simply reducing the usage until within a specified limit and the idea of a soft/hard limit was restricted to AssignGpuMemoryToTiles. https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager.cc:626: EvictTiles(&eviction_it, required_budget, &budget, TilePriority(), true); I think it would make more sense to evict tiles if we're still above the limit after the loop below instead of before it. https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager.cc:640: bool tile_violates_memory_policy = false; Please add a TileViolatesMemoryPolicy helper function instead of this variable.
Hi, I think most of the comments concern eviction here. In general I do prefer that the thing that evicts also somehow instructs the call site of how much stuff it evicted. Otherwise we need to recalculate that amount somehow. Passing the budget by pointer accomplishes that, but there might be other ways. It might be easier if we chat offline about this. https://codereview.chromium.org/246673005/diff/60001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/246673005/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:42: bool HasTilesRequiredForActivation() const; On 2014/05/20 23:01:00, reveman wrote: > Everything related to HasTilesRequiredForActivation is unnecessary once > https://codereview.chromium.org/287643004/ lands, right? That's correct. I'm keeping it here just for testing, but it all goes away with the activation patch. One concern with rebasing is that I'll probably just end up calling the check after every rasterizer notification, since I have no other hints to determine whether it's a good time to activate. https://codereview.chromium.org/246673005/diff/60001/cc/resources/picture_lay... File cc/resources/picture_layer_tiling.cc (right): https://codereview.chromium.org/246673005/diff/60001/cc/resources/picture_lay... cc/resources/picture_layer_tiling.cc:800: DCHECK(it->second); On 2014/05/20 23:01:00, reveman wrote: > I don't think this is worthwhile unless we actually set it->second to NULL > somewhere? Sorry that was a left over from debugging. Removed. https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager.cc:549: bool TileManager::EvictTiles(EvictionTileIterator* eviction_iterator, On 2014/05/20 23:01:00, reveman wrote: > Could you rename this in a way that makes it more clear what it does? > FreeTileResourcesUntilUsageIsWithinBudget? And maybe return true on success, > false otherwise. Done on the rename. It already returns a bool when it evicts rfa tiles. I guess we can change that since we'll be checking for activation independently of this. https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager.cc:551: MemoryBudget* current_budget, On 2014/05/20 23:01:00, reveman wrote: > I think it's a bit confusing with a budget that is being adjusted. It would be > much easier to understand if we were reducing our usage until we're within a > budget. What would it take to change that? We still somehow need to know how much we actually released. It tries to be within budget, it might fail, or it might release a lot (way more than the needed budget). I mean it still needs to know the current budget in order to meet the required budget, so I could pass it by copy, but still adjust it (just hiding the information from the caller). I kind of prefer to have it this way to avoid recalculating how much we actually evicted from the call site. https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager.cc:552: const TilePriority& raster_priority, On 2014/05/20 23:01:00, reveman wrote: > "max_priority"? Done. https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager.cc:553: bool evict_unconditionally) { On 2014/05/20 23:01:00, reveman wrote: > Instead of having evict_unconditionally, could you pass the highest possible > priority as max_priority? It's in some sense max priority + 1, as in equal priorities aren't evicted (as this will cause us to evict and rerasterize tiles of the same priority -- something we want to avoid). https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager.cc:555: while (required_budget.Exceeds(*current_budget)) { On 2014/05/20 23:01:00, reveman wrote: > Would it be a bit cleaner and more consistent with the raster iterator loop if > you have a for loop here that advance the iterator and instead early out of the > loop when within budget? That would cause us to actually initialize the eviction iterator even if we're already within budget (which is relatively expensive). Conceptually I think the loop says the intent "while we're not within budget, evict" instead of "for all eviction tiles, if we're already within budget, break"... https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager.cc:567: break; On 2014/05/20 23:01:00, reveman wrote: > it would be nice if you flip this around and make it: > > if (priority.IsHigherPriorityThan(max_priority)) > break; > > max_priority is not the same as |raster_priority| so this might not work if we > really need this to not pass if priority is equal. Which brings me to: > > Are we currently allowing a "distance_to_visible - > std::numeric_limits<float>::epsilon()" tile that need raster to evict an > initialized tile with "distance_to_visible"? And in that case, do we want that? > Should we be worried that some floating point inexactness will cause us to > alternative between tiles and raster forever? Would it make sense to define a > minimal TilePriority delta that is required between raster and eviction? I definitely think we should not allow equal priorities to pass here. As for the epsilon case, we could make that a part of "IsHigherPriorityThan" if it's a concern. In practice I don't think there will be many cases like that, since one will be biased to be better, but I don't see them alternating since the calculation doesn't change... https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager.cc:576: current_budget->soft_memory_bytes += eviction_bytes_if_allocated; On 2014/05/20 23:01:00, reveman wrote: > Hard to see if this is always doing the right thing. I think it would be easier > to understand if this function was simply reducing the usage until within a > specified limit and the idea of a soft/hard limit was restricted to > AssignGpuMemoryToTiles. This would mean that ASsignGpuMemoryToTiles would have to recalculate these budgets again, which is something I'd like to avoid... Also, the whole concept of Exceeds takes into account soft/hard limits, so I'm not too sure this function can be made to work with just one memory limit and still be clean in its usage. https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager.cc:626: EvictTiles(&eviction_it, required_budget, &budget, TilePriority(), true); On 2014/05/20 23:01:00, reveman wrote: > I think it would make more sense to evict tiles if we're still above the limit > after the loop below instead of before it. We kind of create the budget and the iterator here, so I think it makes sense... I mean if you really insist, I can change this, but to me "Make sure you're within budget before you start assigning memory" makes as much as sense to me as "Ensure once more you're within budget after assigning everything" https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager.cc:640: bool tile_violates_memory_policy = false; On 2014/05/20 23:01:00, reveman wrote: > Please add a TileViolatesMemoryPolicy helper function instead of this variable. Done.
https://codereview.chromium.org/246673005/diff/60001/cc/layers/picture_layer_... File cc/layers/picture_layer_impl.h (right): https://codereview.chromium.org/246673005/diff/60001/cc/layers/picture_layer_... cc/layers/picture_layer_impl.h:42: bool HasTilesRequiredForActivation() const; On 2014/05/27 22:41:32, vmpstr wrote: > On 2014/05/20 23:01:00, reveman wrote: > > Everything related to HasTilesRequiredForActivation is unnecessary once > > https://codereview.chromium.org/287643004/ lands, right? > > That's correct. I'm keeping it here just for testing, but it all goes away with > the activation patch. One concern with rebasing is that I'll probably just end > up calling the check after every rasterizer notification, since I have no other > hints to determine whether it's a good time to activate. See my comment on latest patch. https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager.cc:551: MemoryBudget* current_budget, On 2014/05/27 22:41:32, vmpstr wrote: > On 2014/05/20 23:01:00, reveman wrote: > > I think it's a bit confusing with a budget that is being adjusted. It would be > > much easier to understand if we were reducing our usage until we're within a > > budget. What would it take to change that? > > We still somehow need to know how much we actually released. It tries to be > within budget, it might fail, or it might release a lot (way more than the > needed budget). I mean it still needs to know the current budget in order to > meet the required budget, so I could pass it by copy, but still adjust it (just > hiding the information from the caller). > > I kind of prefer to have it this way to avoid recalculating how much we actually > evicted from the call site. I think the system where we have a budget that we keep adjusting is a bit backwards and a side effect of how the tile manager has evolved. I think this is a good time to fix this. IMO, it makes more sense to have a current usage and a budget that we need to stay within. A simplified version of AssignGpuMemoryToTiles would ideally look something like this: while (raster_it) { Tile* tile = *raster_it; MemoryUsage tile_memory_usage = tile->MemoryRequiredToInitialize(); if (!FreeTileResourcesUntilWithinBudget(budget_ - tile_memory_usage)) break; AppendRasterTask(raster_queue, tile); ++raster_it; } Maybe just get rid of the temporary tiles_that_need_to_be_rasterized vector and build the raster queue directly in AssignGpuMemoryToTiles. Let's chat offline. https://codereview.chromium.org/246673005/diff/60001/cc/resources/tile_manage... cc/resources/tile_manager.cc:576: current_budget->soft_memory_bytes += eviction_bytes_if_allocated; On 2014/05/27 22:41:32, vmpstr wrote: > On 2014/05/20 23:01:00, reveman wrote: > > Hard to see if this is always doing the right thing. I think it would be > easier > > to understand if this function was simply reducing the usage until within a > > specified limit and the idea of a soft/hard limit was restricted to > > AssignGpuMemoryToTiles. > > This would mean that ASsignGpuMemoryToTiles would have to recalculate these > budgets again, which is something I'd like to avoid... Also, the whole concept > of Exceeds takes into account soft/hard limits, so I'm not too sure this > function can be made to work with just one memory limit and still be clean in > its usage. I find this pretty confusing. Why do we care what priority a tile is when evicting it and adjusting the budget? What if the tile priority changes? Does that mess up the our budget calculations? This is part of what I think can be much simpler if we switch to a model where we have a current usage and instead adjusted the usage to to stay within the budget needed to be able to raster a tile but let's chat more about this offline. https://codereview.chromium.org/246673005/diff/110001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/110001/cc/resources/tile_manag... cc/resources/tile_manager.cc:595: case NUM_TILE_MEMORY_LIMIT_POLICIES: Please remove NUM_TILE_MEMORY_LIMIT_POLICIES and move NOTREACHED() out of the switch statement now that it's no longer needed. https://codereview.chromium.org/246673005/diff/110001/cc/resources/tile_manag... cc/resources/tile_manager.cc:753: all_tiles_required_for_activation_have_memory_ = I don't think all_tiles_required_for_activation_have_memory_ makes sense anymore. No good way to accurately determine this and also not necessary. We can remove it completely or maybe replace it with some_tiles_required_for_activation_are_pending_ to avoid some unnecessary calls to CheckIfReadyToActivate. https://codereview.chromium.org/246673005/diff/110001/cc/resources/tile_prior... File cc/resources/tile_priority.h (right): https://codereview.chromium.org/246673005/diff/110001/cc/resources/tile_prior... cc/resources/tile_priority.h:126: NUM_TILE_MEMORY_LIMIT_POLICIES = 4, Remove this enum value https://codereview.chromium.org/246673005/diff/110001/cc/resources/tile_prior... cc/resources/tile_priority.h:129: // or reordering fields. and update this comment.
Hi, As discussed offline, we agree that the final product here is probably going to be different. That is, evicting before allocating memory allows us to actually do the allocation and task creation in the assign gpu memory loop (which might as well be called AllocateMemoryAndCreateTasks). That being said, I would really like to push back doing that in this patch. Please consider this patch as the thing to land. The problem is that first, rebasing is becoming hard since things keep shifting around tile manager and layers (and adding restructuring here will only make it harder). Secondly, there might be non obvious bugs in this patch that I would like to catch early: this means that if the patch also restructures the way things are done, it would become harder to identify where the bug is. One thing that is currently missing, which I will add ASAP is memory management. I talked to ccameron, and it seems that I need to at least report "required" bytes correctly. This means that after breaking out of a loop, I'll have to keep iterating the raster iterator until I hit the first non-now tile.
On 2014/05/29 18:36:10, vmpstr wrote: > Hi, > > As discussed offline, we agree that the final product here is probably going to > be different. That is, evicting before allocating memory allows us to actually > do the allocation and task creation in the assign gpu memory loop (which might > as well be called AllocateMemoryAndCreateTasks). > > That being said, I would really like to push back doing that in this patch. > Please consider this patch as the thing to land. The problem is that first, > rebasing is becoming hard since things keep shifting around tile manager and > layers (and adding restructuring here will only make it harder). Secondly, there > might be non obvious bugs in this patch that I would like to catch early: this > means that if the patch also restructures the way things are done, it would > become harder to identify where the bug is. Ok, I'll review this asap with the assumption that the current 2 stage assign-memory-to-tiles+schedule-tasks system will not change as part of this patch. > > One thing that is currently missing, which I will add ASAP is memory management. > I talked to ccameron, and it seems that I need to at least report "required" > bytes correctly. This means that after breaking out of a loop, I'll have to keep > iterating the raster iterator until I hit the first non-now tile. This seems like the wrong approach. I'm not sure the tile manager should be involved in any of the gpu memory manager communication. It made sense to do this in the tile manager in the past when it knew about all tiles and owned the resource pool but that's not the case anymore. Could we instead compute "required memory" and whatever else we might need when updating tile priorities?
On 2014/05/29 20:26:36, reveman wrote: > On 2014/05/29 18:36:10, vmpstr wrote: > > Hi, > > > > As discussed offline, we agree that the final product here is probably going > to > > be different. That is, evicting before allocating memory allows us to actually > > do the allocation and task creation in the assign gpu memory loop (which might > > as well be called AllocateMemoryAndCreateTasks). > > > > That being said, I would really like to push back doing that in this patch. > > Please consider this patch as the thing to land. The problem is that first, > > rebasing is becoming hard since things keep shifting around tile manager and > > layers (and adding restructuring here will only make it harder). Secondly, > there > > might be non obvious bugs in this patch that I would like to catch early: this > > means that if the patch also restructures the way things are done, it would > > become harder to identify where the bug is. > > Ok, I'll review this asap with the assumption that the current 2 stage > assign-memory-to-tiles+schedule-tasks system will not change as part of this > patch. > Thanks! > > > > One thing that is currently missing, which I will add ASAP is memory > management. > > I talked to ccameron, and it seems that I need to at least report "required" > > bytes correctly. This means that after breaking out of a loop, I'll have to > keep > > iterating the raster iterator until I hit the first non-now tile. > > This seems like the wrong approach. I'm not sure the tile manager should be > involved in any of the gpu memory manager communication. It made sense to do > this in the tile manager in the past when it knew about all tiles and owned the > resource pool but that's not the case anymore. Could we instead compute > "required memory" and whatever else we might need when updating tile priorities? Yeah, I think that makes sense. ccameron@ is working on trying to remove the memory stats altogether... I'll talk to him a bit more to see what's happening with it. If it's sticking around, then doing it during update tile priorities might be doable and would make more sense (some corner cases with solid color maybe or maybe not).
+ccameron to cc
On 2014/05/29 20:37:54, vmpstr wrote: > On 2014/05/29 20:26:36, reveman wrote: > > On 2014/05/29 18:36:10, vmpstr wrote: > > > Hi, > > > > > > As discussed offline, we agree that the final product here is probably going > > to > > > be different. That is, evicting before allocating memory allows us to > actually > > > do the allocation and task creation in the assign gpu memory loop (which > might > > > as well be called AllocateMemoryAndCreateTasks). > > > > > > That being said, I would really like to push back doing that in this patch. > > > Please consider this patch as the thing to land. The problem is that first, > > > rebasing is becoming hard since things keep shifting around tile manager and > > > layers (and adding restructuring here will only make it harder). Secondly, > > there > > > might be non obvious bugs in this patch that I would like to catch early: > this > > > means that if the patch also restructures the way things are done, it would > > > become harder to identify where the bug is. > > > > Ok, I'll review this asap with the assumption that the current 2 stage > > assign-memory-to-tiles+schedule-tasks system will not change as part of this > > patch. > > > > Thanks! > > > > > > > One thing that is currently missing, which I will add ASAP is memory > > management. > > > I talked to ccameron, and it seems that I need to at least report "required" > > > bytes correctly. This means that after breaking out of a loop, I'll have to > > keep > > > iterating the raster iterator until I hit the first non-now tile. > > > > This seems like the wrong approach. I'm not sure the tile manager should be > > involved in any of the gpu memory manager communication. It made sense to do > > this in the tile manager in the past when it knew about all tiles and owned > the > > resource pool but that's not the case anymore. Could we instead compute > > "required memory" and whatever else we might need when updating tile > priorities? > > Yeah, I think that makes sense. ccameron@ is working on trying to remove the > memory stats altogether... I'll talk to him a bit more to see what's happening > with it. If it's sticking around, then doing it during update tile priorities > might be doable and would make more sense (some corner cases with solid color > maybe or maybe not). Don't think that code should take solid color tiles into account. Better to assume they all need memory.
Let me know when this is ready for me to review. https://codereview.chromium.org/246673005/diff/170001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/170001/cc/resources/tile_manag... cc/resources/tile_manager.cc:1305: if (b_priority.priority_bin == a_priority.priority_bin && Can we move all iterator related changes into a separate patch (this and changes below)?
PTAL https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_manag... cc/resources/tile_manager.cc:460: *memory_required_bytes = resource_pool_->total_memory_usage_bytes() + ccameron@, can you please take a look to see if this is still needed; if so, is this acceptable for the memory manager or should this provide some other information? https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_manag... cc/resources/tile_manager.cc:1414: // Now we have to return true iff b is lower priority than a. This block is parallel to what landed for the raster iterator, let me know if you prefer this as a separate patch.
https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile.cc File cc/resources/tile.cc (right): https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile.cc#ne... cc/resources/tile.cc:48: return; I don't think this early out makes sense anymore. https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile.cc#ne... cc/resources/tile.cc:55: return; neither does this one https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_manag... cc/resources/tile_manager.cc:459: size_t* memory_used_bytes) const { Let's remove this function completely in a separate patch. Doesn't make sense to have the tile manager provide these values anymore. I think LTHI is in a much better position to provide these values. ie. the staging pool resources used with one-copy rasterizer should probably be included too. https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_manag... cc/resources/tile_manager.cc:556: case NUM_TILE_MEMORY_LIMIT_POLICIES: please move NOTREACHED below so it fails if some random invalid value has been written to .memory_limit_policy. https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_manag... cc/resources/tile_manager.cc:1414: // Now we have to return true iff b is lower priority than a. On 2014/06/13 21:08:38, vmpstr wrote: > This block is parallel to what landed for the raster iterator, let me know if > you prefer this as a separate patch. Yes, I wouldn't mind that. https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_manag... cc/resources/tile_manager.h:225: // memory returns to soft limit after going over. not sure what this comment does here. doesn't make sense to me in this context. can you remove it in this patch unless there's a good reason for it to be here? https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_manag... cc/resources/tile_manager.h:301: bool evict_unconditionally); The return value of this function seem to be unused. Can we remove it? https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_prior... File cc/resources/tile_priority.h (right): https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_prior... cc/resources/tile_priority.h:126: NUM_TILE_MEMORY_LIMIT_POLICIES = 4, Please remove this now that it's not needed. https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_prior... cc/resources/tile_priority.h:129: // or reordering fields. This comment is incorrect and I don't think it's needed at all after removing NUM_TILE_MEMORY_LIMIT_POLICIES. https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_prior... cc/resources/tile_priority.h:139: // Be sure to update TreePriorityAsValue when adding new fields. I don't think this comment is useful. Please remove it as part of this patch if you don't mind.
PTAL https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile.cc File cc/resources/tile.cc (right): https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile.cc#ne... cc/resources/tile.cc:48: return; On 2014/06/14 14:53:24, reveman wrote: > I don't think this early out makes sense anymore. Done. https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile.cc#ne... cc/resources/tile.cc:55: return; On 2014/06/14 14:53:24, reveman wrote: > neither does this one Done. https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_manag... cc/resources/tile_manager.cc:459: size_t* memory_used_bytes) const { On 2014/06/14 14:53:24, reveman wrote: > Let's remove this function completely in a separate patch. Doesn't make sense to > have the tile manager provide these values anymore. I think LTHI is in a much > better position to provide these values. ie. the staging pool resources used > with one-copy rasterizer should probably be included too. Sounds good. https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_manag... cc/resources/tile_manager.cc:556: case NUM_TILE_MEMORY_LIMIT_POLICIES: On 2014/06/14 14:53:24, reveman wrote: > please move NOTREACHED below so it fails if some random invalid value has been > written to .memory_limit_policy. Done. https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_manag... cc/resources/tile_manager.h:225: // memory returns to soft limit after going over. On 2014/06/14 14:53:25, reveman wrote: > not sure what this comment does here. doesn't make sense to me in this context. > can you remove it in this patch unless there's a good reason for it to be here? Yeah, I'm not sure what this comment means here. Removed. https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_manag... cc/resources/tile_manager.h:301: bool evict_unconditionally); On 2014/06/14 14:53:25, reveman wrote: > The return value of this function seem to be unused. Can we remove it? Done. https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_prior... File cc/resources/tile_priority.h (right): https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_prior... cc/resources/tile_priority.h:126: NUM_TILE_MEMORY_LIMIT_POLICIES = 4, On 2014/06/14 14:53:25, reveman wrote: > Please remove this now that it's not needed. Done. https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_prior... cc/resources/tile_priority.h:129: // or reordering fields. On 2014/06/14 14:53:25, reveman wrote: > This comment is incorrect and I don't think it's needed at all after removing > NUM_TILE_MEMORY_LIMIT_POLICIES. Done. https://codereview.chromium.org/246673005/diff/210001/cc/resources/tile_prior... cc/resources/tile_priority.h:139: // Be sure to update TreePriorityAsValue when adding new fields. On 2014/06/14 14:53:25, reveman wrote: > I don't think this comment is useful. Please remove it as part of this patch if > you don't mind. Done.
https://codereview.chromium.org/246673005/diff/250001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/246673005/diff/250001/cc/resources/tile_manag... cc/resources/tile_manager.h:259: struct MemoryBudget { I'm not a fan of this struct. Having a budget instead of a usage that is adjusted feels very backwards and it seem to make this iterator code more complicated and harder to understand imo. No need to change the resource pool in this patch but I think we should still replace this struct with a MemoryUsage struct that is manipulated in a similar way as the current budget struct. Add/Subtract functions and being able to create an instance from a Tile would be nice. ie. MemoryUsage::FromTile(const Tile& tile) https://codereview.chromium.org/246673005/diff/250001/cc/resources/tile_manag... cc/resources/tile_manager.h:299: bool evict_unconditionally); I would prefer this as: FreeTileResourcesUntilUsageIsWithinLimit( EvictionTileIterator* eviction_iterator, const MemoryUsage& limit, const TilePriority& max_priority, bool evict_unconditionally, MemoryUsage* usage);
PTAL https://codereview.chromium.org/246673005/diff/250001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/246673005/diff/250001/cc/resources/tile_manag... cc/resources/tile_manager.h:259: struct MemoryBudget { On 2014/06/16 22:06:52, reveman wrote: > I'm not a fan of this struct. Having a budget instead of a usage that is > adjusted feels very backwards and it seem to make this iterator code more > complicated and harder to understand imo. > > No need to change the resource pool in this patch but I think we should still > replace this struct with a MemoryUsage struct that is manipulated in a similar > way as the current budget struct. > > Add/Subtract functions and being able to create an instance from a Tile would be > nice. ie. MemoryUsage::FromTile(const Tile& tile) Mostly done. I can't easily have FromTile on the MemoryUsage, since it depends on both tree priority and BytesRequiredIfAllocated (on tile manager). I've added TileManager::TileMemoryUsage instead. https://codereview.chromium.org/246673005/diff/250001/cc/resources/tile_manag... cc/resources/tile_manager.h:299: bool evict_unconditionally); On 2014/06/16 22:06:52, reveman wrote: > I would prefer this as: > > FreeTileResourcesUntilUsageIsWithinLimit( > EvictionTileIterator* eviction_iterator, > const MemoryUsage& limit, > const TilePriority& max_priority, > bool evict_unconditionally, > MemoryUsage* usage); Done.
https://codereview.chromium.org/246673005/diff/270001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/270001/cc/resources/tile_manag... cc/resources/tile_manager.cc:524: // tile. nit: I don't think this comment is worth much now that the code is so easy to understand. https://codereview.chromium.org/246673005/diff/270001/cc/resources/tile_manag... cc/resources/tile_manager.cc:623: memory_usage.Add(tile_usage); It's confusing to add this here but then remove it below. Can you do something like this instead? FreeTileResourcesUntilUsageIsWithinLimit( &eviction_it, memory_limit - tile_usage, ...); https://codereview.chromium.org/246673005/diff/270001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/246673005/diff/270001/cc/resources/tile_manag... cc/resources/tile_manager.h:267: const GlobalStateThatImpactsTilePriority& state); I don't think this is a good idea. See my comment below. https://codereview.chromium.org/246673005/diff/270001/cc/resources/tile_manag... cc/resources/tile_manager.h:277: int64 hard_memory_bytes_; I don't think there should be such a thing as soft/hard memory usage. IMO, there's just memory usage and soft/hard memory limits. Can you reduce this to just memory_bytes_ and handle the idea of hard vs soft limit outside of this struct? This should also allow you to add a simple MemoryUsage::FromTile function. https://codereview.chromium.org/246673005/diff/270001/cc/resources/tile_manag... cc/resources/tile_manager.h:306: void FreeTileResourcesUntilUsageIsWithinBudget( IsWithinLimit https://codereview.chromium.org/246673005/diff/270001/cc/resources/tile_manag... cc/resources/tile_manager.h:311: MemoryUsage* usage); Btw, you can have this function return true when it successfully managed to reduce usage to the limit or less if useful somehow..
https://codereview.chromium.org/246673005/diff/270001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/246673005/diff/270001/cc/resources/tile_manag... cc/resources/tile_manager.h:277: int64 hard_memory_bytes_; On 2014/06/17 00:46:33, reveman wrote: > I don't think there should be such a thing as soft/hard memory usage. IMO, > there's just memory usage and soft/hard memory limits. Can you reduce this to > just memory_bytes_ and handle the idea of hard vs soft limit outside of this > struct? This should also allow you to add a simple MemoryUsage::FromTile > function. Conceptually, I agree. However, it makes it a bit awkward that hard memory limit is always used, soft memory limit is only sometimes used and resource count is shared among the two. What I'm trying to say is that I will likely end up with something like the following: MemoryUsage soft_limit = ...; MemoryUsage hard_limit = ...; MemoryUsage soft_usage = ...; MemoryUsage hard_usage = ...; won't i? That and then juggling both becomes much more of a pain. Part of the reason I wanted this struct is to encapsulate all of that logic outside of tile manager. That is, if we just go to memory limit, then we can just go back to keeping track of int64s, no? Maybe I misunderstand. Could you elaborate a little bit more? As I understand what you're proposing now, I think we should stick with keeping soft/hard in the usage. After all, the tiles do use either hard or soft memory...
https://codereview.chromium.org/246673005/diff/270001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/246673005/diff/270001/cc/resources/tile_manag... cc/resources/tile_manager.h:277: int64 hard_memory_bytes_; On 2014/06/17 02:07:40, vmpstr wrote: > On 2014/06/17 00:46:33, reveman wrote: > > I don't think there should be such a thing as soft/hard memory usage. IMO, > > there's just memory usage and soft/hard memory limits. Can you reduce this to > > just memory_bytes_ and handle the idea of hard vs soft limit outside of this > > struct? This should also allow you to add a simple MemoryUsage::FromTile > > function. > > Conceptually, I agree. However, it makes it a bit awkward that hard memory limit > is always used, soft memory limit is only sometimes used and resource count is > shared among the two. > > What I'm trying to say is that I will likely end up with something like the > following: > > MemoryUsage soft_limit = ...; > MemoryUsage hard_limit = ...; > MemoryUsage soft_usage = ...; > MemoryUsage hard_usage = ...; > > won't i? That and then juggling both becomes much more of a pain. Part of the > reason I wanted this struct is to encapsulate all of that logic outside of tile > manager. That is, if we just go to memory limit, then we can just go back to > keeping track of int64s, no? > > Maybe I misunderstand. Could you elaborate a little bit more? As I understand > what you're proposing now, I think we should stick with keeping soft/hard in the > usage. After all, the tiles do use either hard or soft memory... I don't think there should be soft/hard_usage. Only usage. The only way I can reason about soft/hard memory limits is when having all tiles use the same kind of memory but some of them being constrained by a soft limit and some being constrained by a hard limit when being assigned memory. I find the concept of tiles using different types of memory very confusing and not sure what the correct behavior is in such a system. What I'm suggesting is that we have: MemoryUsage soft_limit = ...; MemoryUsage hard_limit = ...; as you suggested above but these are simply constants derived from GlobalState and not something that would change during a call to AssignGpuMem. "MemoryUsage usage" will change. It's increased or decreased independent of what type of limit a tile is affected by. The only time soft/hard memory limit comes into play would be when trying to assign memory to a tile. If tile is affected by soft limit then we need to evict tiles until usage is within "soft_limit - MemoryUsage::ForTile(tile_that_needs_raster)" otherwise "hard_limit - ..". Does that make sense? I think the MemoryUsage struct is very useful for writing code that is easy to read.
PTAL https://codereview.chromium.org/246673005/diff/270001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/270001/cc/resources/tile_manag... cc/resources/tile_manager.cc:524: // tile. On 2014/06/17 00:46:33, reveman wrote: > nit: I don't think this comment is worth much now that the code is so easy to > understand. Done. https://codereview.chromium.org/246673005/diff/270001/cc/resources/tile_manag... cc/resources/tile_manager.cc:623: memory_usage.Add(tile_usage); On 2014/06/17 00:46:33, reveman wrote: > It's confusing to add this here but then remove it below. Can you do something > like this instead? > > FreeTileResourcesUntilUsageIsWithinLimit( > &eviction_it, memory_limit - tile_usage, ...); Done. https://codereview.chromium.org/246673005/diff/270001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/246673005/diff/270001/cc/resources/tile_manag... cc/resources/tile_manager.h:267: const GlobalStateThatImpactsTilePriority& state); On 2014/06/17 00:46:33, reveman wrote: > I don't think this is a good idea. See my comment below. Done. https://codereview.chromium.org/246673005/diff/270001/cc/resources/tile_manag... cc/resources/tile_manager.h:277: int64 hard_memory_bytes_; On 2014/06/17 04:24:16, reveman wrote: > On 2014/06/17 02:07:40, vmpstr wrote: > > On 2014/06/17 00:46:33, reveman wrote: > > > I don't think there should be such a thing as soft/hard memory usage. IMO, > > > there's just memory usage and soft/hard memory limits. Can you reduce this > to > > > just memory_bytes_ and handle the idea of hard vs soft limit outside of this > > > struct? This should also allow you to add a simple MemoryUsage::FromTile > > > function. > > > > Conceptually, I agree. However, it makes it a bit awkward that hard memory > limit > > is always used, soft memory limit is only sometimes used and resource count is > > shared among the two. > > > > What I'm trying to say is that I will likely end up with something like the > > following: > > > > MemoryUsage soft_limit = ...; > > MemoryUsage hard_limit = ...; > > MemoryUsage soft_usage = ...; > > MemoryUsage hard_usage = ...; > > > > won't i? That and then juggling both becomes much more of a pain. Part of the > > reason I wanted this struct is to encapsulate all of that logic outside of > tile > > manager. That is, if we just go to memory limit, then we can just go back to > > keeping track of int64s, no? > > > > Maybe I misunderstand. Could you elaborate a little bit more? As I understand > > what you're proposing now, I think we should stick with keeping soft/hard in > the > > usage. After all, the tiles do use either hard or soft memory... > > I don't think there should be soft/hard_usage. Only usage. The only way I can > reason about soft/hard memory limits is when having all tiles use the same kind > of memory but some of them being constrained by a soft limit and some being > constrained by a hard limit when being assigned memory. I find the concept of > tiles using different types of memory very confusing and not sure what the > correct behavior is in such a system. > > What I'm suggesting is that we have: > MemoryUsage soft_limit = ...; > MemoryUsage hard_limit = ...; > as you suggested above but these are simply constants derived from GlobalState > and not something that would change during a call to AssignGpuMem. "MemoryUsage > usage" will change. It's increased or decreased independent of what type of > limit a tile is affected by. The only time soft/hard memory limit comes into > play would be when trying to assign memory to a tile. If tile is affected by > soft limit then we need to evict tiles until usage is within "soft_limit - > MemoryUsage::ForTile(tile_that_needs_raster)" otherwise "hard_limit - ..". > > Does that make sense? I think the MemoryUsage struct is very useful for writing > code that is easy to read. Ahhhh, yes that makes sense. I thought you meant having separate usage as well as limit, but now that I reread your original comment, I'm not sure what I was thinking :P Take a look now please. Note I still can't do FromTile here, since I need resource_pool_ to get the format (as well as mts, which is private.. TileManager happens to be a friend). We can probably rework the friendship/size calculations later. https://codereview.chromium.org/246673005/diff/270001/cc/resources/tile_manag... cc/resources/tile_manager.h:306: void FreeTileResourcesUntilUsageIsWithinBudget( On 2014/06/17 00:46:33, reveman wrote: > IsWithinLimit Done. https://codereview.chromium.org/246673005/diff/270001/cc/resources/tile_manag... cc/resources/tile_manager.h:311: MemoryUsage* usage); On 2014/06/17 00:46:33, reveman wrote: > Btw, you can have this function return true when it successfully managed to > reduce usage to the limit or less if useful somehow.. Done. https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... cc/resources/tile_manager.h:264: MemoryUsage& operator+=(const MemoryUsage& other); I decided to go with operators, since it is kind of natural in this case.
https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... cc/resources/tile_manager.cc:504: EvictionTileIterator* eviction_iterator, nit: this could just be named iterator, iter or it as I think the function provides a context that make it clear what type of iterator this is. https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... cc/resources/tile_manager.cc:507: bool evict_unconditionally, I think this would be cleaner if you split this into two different functions instead of an evict_unconditionally argument. FreeTileResourcesUntilUsageIsWithinLimit and FreeTileResourcesWithLowerPriorityUntilUsageIsWithinLimit. https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... cc/resources/tile_manager.cc:513: Tile* eviction_tile = **eviction_iterator; nit: simply |tile| would be fine unless you prefer this https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... cc/resources/tile_manager.cc:514: DCHECK(eviction_tile); nit: maybe move this DCHECK into the iterator as an error here seems like broken implementation. Or would it ever make sense to return a NULL tile but have the boolean check above be true? https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... cc/resources/tile_manager.cc:516: TilePriority eviction_priority = nit: do you need this temporary variable? I think the code would be just as easy to read without it. https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... cc/resources/tile_manager.cc:565: global_state_.num_resources_limit); You can use "const" here. I don't care much if you use "const" for local variables or not but please be consistent. You're using const for a number of variables below so unless you remove that we should also use it here or it might look like these variables are not actually const. https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... cc/resources/tile_manager.cc:601: if (tile_version.IsReadyToDraw()) Why would the iterator return a tile that is already ready to draw? Can this be a DCHECK instead? https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... cc/resources/tile_manager.cc:633: FreeResourcesForTileAndNotifyClientIfTileWasReadyToDraw(tile); It seems wrong to have to release resources here when using the raster iterator. Why do we need this? https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... cc/resources/tile_manager.cc:715: initial_memory_usage.memory_bytes() - memory_usage.memory_bytes(); How about using resource_pool->acquired_mem.. here instead of initial_memory_usage? https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... cc/resources/tile_manager.h:277: MemoryUsage TileMemoryUsage(Tile* tile); I don't think this needs to be a member of the TileManager as you can get the format/size of an existing resource using Resource::format()/size(). I think having these static MemoryUsage functions would be nice: static MemoryUsage MemoryUsage::FromConfig( const gfx::Size& size, ResourceFormat format); static MemoryUsage MemoryUsage::FromTile(const Tile& tile);
PTAL https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... cc/resources/tile_manager.cc:504: EvictionTileIterator* eviction_iterator, On 2014/06/17 17:15:08, reveman wrote: > nit: this could just be named iterator, iter or it as I think the function > provides a context that make it clear what type of iterator this is. Done. https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... cc/resources/tile_manager.cc:507: bool evict_unconditionally, On 2014/06/17 17:15:08, reveman wrote: > I think this would be cleaner if you split this into two different functions > instead of an evict_unconditionally argument. > > FreeTileResourcesUntilUsageIsWithinLimit and > FreeTileResourcesWithLowerPriorityUntilUsageIsWithinLimit. Done. https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... cc/resources/tile_manager.cc:513: Tile* eviction_tile = **eviction_iterator; On 2014/06/17 17:15:08, reveman wrote: > nit: simply |tile| would be fine unless you prefer this Done. https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... cc/resources/tile_manager.cc:514: DCHECK(eviction_tile); On 2014/06/17 17:15:08, reveman wrote: > nit: maybe move this DCHECK into the iterator as an error here seems like broken > implementation. Or would it ever make sense to return a NULL tile but have the > boolean check above be true? This was a carryover from debugging, removed. https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... cc/resources/tile_manager.cc:516: TilePriority eviction_priority = On 2014/06/17 17:15:08, reveman wrote: > nit: do you need this temporary variable? I think the code would be just as easy > to read without it. Done. https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... cc/resources/tile_manager.cc:565: global_state_.num_resources_limit); On 2014/06/17 17:15:08, reveman wrote: > You can use "const" here. I don't care much if you use "const" for local > variables or not but please be consistent. You're using const for a number of > variables below so unless you remove that we should also use it here or it might > look like these variables are not actually const. I removed the other const. I kind of prefer it without. https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... cc/resources/tile_manager.cc:601: if (tile_version.IsReadyToDraw()) On 2014/06/17 17:15:08, reveman wrote: > Why would the iterator return a tile that is already ready to draw? Can this be > a DCHECK instead? Done. https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... cc/resources/tile_manager.cc:633: FreeResourcesForTileAndNotifyClientIfTileWasReadyToDraw(tile); On 2014/06/17 17:15:08, reveman wrote: > It seems wrong to have to release resources here when using the raster iterator. > Why do we need this? Hmm. Initially this was here because of tile versions. However, I don't think it makes sense to OOM a tile and then also free all the remainder of the resources. Please take a look at the latest patch. https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... cc/resources/tile_manager.cc:715: initial_memory_usage.memory_bytes() - memory_usage.memory_bytes(); On 2014/06/17 17:15:08, reveman wrote: > How about using resource_pool->acquired_mem.. here instead of > initial_memory_usage? Done. https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/246673005/diff/290001/cc/resources/tile_manag... cc/resources/tile_manager.h:277: MemoryUsage TileMemoryUsage(Tile* tile); On 2014/06/17 17:15:08, reveman wrote: > I don't think this needs to be a member of the TileManager as you can get the > format/size of an existing resource using Resource::format()/size(). > > I think having these static MemoryUsage functions would be nice: > > static MemoryUsage MemoryUsage::FromConfig( > const gfx::Size& size, ResourceFormat format); > > static MemoryUsage MemoryUsage::FromTile(const Tile& tile); Done. https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (left): https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.cc:872: tile_version.set_rasterize_on_demand(); I'm removing this bit. I don't think it's useful and was mostly a fire fighting thing. If we need to go back to immediately setting evicted tiles as ROD, then we can revisit the proper thing to do.
https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.cc:586: size_t bytes_that_exceeded_memory_budget = 0; I don't think this function can produce a useful value for this anymore. I prefer if we remove it. https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.cc:613: const size_t bytes_if_allocated = BytesConsumedIfAllocated(tile); "const" here but nowhere else? is this temporary variable useful at all? https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.cc:619: have_hit_soft_memory |= !tile_uses_hard_limit; Can we relax this now? I don't think we should enforce this here unless it's critical for this code to execute correctly. https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.cc:623: MemoryUsage additional_memory_required; can you move this and all the code that use it into the "if (!tile_version.raster_task_)" scope and basically make it const? https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.cc:628: bool memory_reduced_successfully = I think this would be cleaner without a temporary memory_reduced_successfully variable. Not sure you even need a comment as the function name says it all imo. https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.cc:708: memory_usage.memory_bytes(); hm, should this be the other way around, "usage - acquired" instead? https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.h:289: inline size_t BytesConsumedIfAllocated(const Tile* tile) const { Current usage of this function that can't simply be replaced by MemoryUsage::FromConfig seems questionable. Can you have a look at the code to see if this can't be removed as part of this patch in favor of using MemoryUsage::FromConfig? https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.h:304: EvictionTileIterator* eviction_iterator, nit: s/eviction_iterator/iterator/ https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.h:308: EvictionTileIterator* eviction_iterator, nit: s/eviction_iterator/iterator/ https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.h:329: size_t resources_releasable_; Do we still need these? bytes/resources_releasable doesn't seem like something that the tile manager should be maintaining anymore..
PTAL https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.cc:586: size_t bytes_that_exceeded_memory_budget = 0; On 2014/06/17 19:30:07, reveman wrote: > I don't think this function can produce a useful value for this anymore. I > prefer if we remove it. I've added another spot where it's added (when we count up the visible tiles left). I think after that change, this still makes sense. That's the number of bytes we're over in order to display all visible content. https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.cc:613: const size_t bytes_if_allocated = BytesConsumedIfAllocated(tile); On 2014/06/17 19:30:07, reveman wrote: > "const" here but nowhere else? is this temporary variable useful at all? Removed in favour of memory usage https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.cc:619: have_hit_soft_memory |= !tile_uses_hard_limit; On 2014/06/17 19:30:07, reveman wrote: > Can we relax this now? I don't think we should enforce this here unless it's > critical for this code to execute correctly. Hmm.. This is still useful in the sense that we break out of the loop if we run out of memory, where memory means either hard or soft. This check ensures that we don't break out of the loop based on the fact that we're out of soft memory, but then still have visible tiles afterwards. I mean it's not really protecting the out of memory case, but it's verifying that all tiles that use hard memory come before all tiles that use soft memory. Since it's only used in this dcheck, it doesn't really matter if we remove it. However, I would prefer we keep it for the time being. https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.cc:623: MemoryUsage additional_memory_required; On 2014/06/17 19:30:07, reveman wrote: > can you move this and all the code that use it into the "if > (!tile_version.raster_task_)" scope and basically make it const? Done. https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.cc:628: bool memory_reduced_successfully = On 2014/06/17 19:30:07, reveman wrote: > I think this would be cleaner without a temporary memory_reduced_successfully > variable. Not sure you even need a comment as the function name says it all imo. I prefer the temporary, since I don't like putting a function with multiple parameters inside an if condition. https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.cc:708: memory_usage.memory_bytes(); On 2014/06/17 19:30:07, reveman wrote: > hm, should this be the other way around, "usage - acquired" instead? You're right. Good catch https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.h:289: inline size_t BytesConsumedIfAllocated(const Tile* tile) const { On 2014/06/17 19:30:07, reveman wrote: > Current usage of this function that can't simply be replaced by > MemoryUsage::FromConfig seems questionable. Can you have a look at the code to > see if this can't be removed as part of this patch in favor of using > MemoryUsage::FromConfig? Done. https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.h:304: EvictionTileIterator* eviction_iterator, On 2014/06/17 19:30:07, reveman wrote: > nit: s/eviction_iterator/iterator/ Done. https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.h:308: EvictionTileIterator* eviction_iterator, On 2014/06/17 19:30:07, reveman wrote: > nit: s/eviction_iterator/iterator/ Done. https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.h:329: size_t resources_releasable_; On 2014/06/17 19:30:07, reveman wrote: > Do we still need these? bytes/resources_releasable doesn't seem like something > that the tile manager should be maintaining anymore.. It's mostly used in DCHECKs as well as memory_state_from_last_assign_.bytes_unreleasable. I've changed this to use MemoryUsage. I'd like to hold off on any changes that start moving responsibilities around classes for now. That is, I'd like this to still be calculated by the tile manager even if it's not the best spot. This is definitely an opportunity for follow-up.
https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.cc:619: have_hit_soft_memory |= !tile_uses_hard_limit; On 2014/06/17 21:34:05, vmpstr wrote: > On 2014/06/17 19:30:07, reveman wrote: > > Can we relax this now? I don't think we should enforce this here unless it's > > critical for this code to execute correctly. > > Hmm.. This is still useful in the sense that we break out of the loop if we run > out of memory, where memory means either hard or soft. This check ensures that > we don't break out of the loop based on the fact that we're out of soft memory, > but then still have visible tiles afterwards. > > I mean it's not really protecting the out of memory case, but it's verifying > that all tiles that use hard memory come before all tiles that use soft memory. > Since it's only used in this dcheck, it doesn't really matter if we remove it. > However, I would prefer we keep it for the time being. I have to insist on removing this. I think it made sense to have it before but it seems incorrect to make assumptions about the order in which the iterator will return tiles here. Whatever the iterator returns is correct IMO. If the iterator decides that some invisible tile should be initialized before a visible tile then we should just respect that decision and handle it probably here. Maybe move the DCHECK to the iterator somewhere if you're worried about it doing the wrong thing. https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.cc:628: bool memory_reduced_successfully = On 2014/06/17 21:34:05, vmpstr wrote: > On 2014/06/17 19:30:07, reveman wrote: > > I think this would be cleaner without a temporary memory_reduced_successfully > > variable. Not sure you even need a comment as the function name says it all > imo. > > I prefer the temporary, since I don't like putting a function with multiple > parameters inside an if condition. Ok, that's fine. I don't think the comments make sense anymore though. The name of the function is a better description than the comments. https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/246673005/diff/330001/cc/resources/tile_manag... cc/resources/tile_manager.h:329: size_t resources_releasable_; On 2014/06/17 21:34:06, vmpstr wrote: > On 2014/06/17 19:30:07, reveman wrote: > > Do we still need these? bytes/resources_releasable doesn't seem like something > > that the tile manager should be maintaining anymore.. > > It's mostly used in DCHECKs as well as > memory_state_from_last_assign_.bytes_unreleasable. > > I've changed this to use MemoryUsage. I'd like to hold off on any changes that > start moving responsibilities around classes for now. That is, I'd like this to > still be calculated by the tile manager even if it's not the best spot. This is > definitely an opportunity for follow-up. As with computing bytes_required_but_not_allocated_, I'm going to ask for this to be done in a pre-patch rather than a follow-up unless there's a good reason that can't be done. https://codereview.chromium.org/246673005/diff/350001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/350001/cc/resources/tile_manag... cc/resources/tile_manager.cc:609: tile_uses_hard_limit ? hard_memory_limit : soft_memory_limit; Please move this to the scope where it's used. https://codereview.chromium.org/246673005/diff/350001/cc/resources/tile_manag... cc/resources/tile_manager.cc:665: // bytes. The only part of this patch that I'm not a fan of now is this loop and the tile manager members related to it. I can't think of a good reason to why we shouldn't remove the need for this prior to this patch. Even if the memory manager still needs "required bytes", it should not be computed by the tile manager. Let's land a patch that removes the need for this unless you know of a good reason why it can't be done in advance to this patch?
https://codereview.chromium.org/246673005/diff/350001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/350001/cc/resources/tile_manag... cc/resources/tile_manager.cc:609: tile_uses_hard_limit ? hard_memory_limit : soft_memory_limit; On 2014/06/17 22:26:08, reveman wrote: > Please move this to the scope where it's used. Done. https://codereview.chromium.org/246673005/diff/350001/cc/resources/tile_manag... cc/resources/tile_manager.cc:665: // bytes. On 2014/06/17 22:26:08, reveman wrote: > The only part of this patch that I'm not a fan of now is this loop and the tile > manager members related to it. I can't think of a good reason to why we > shouldn't remove the need for this prior to this patch. Even if the memory > manager still needs "required bytes", it should not be computed by the tile > manager. Let's land a patch that removes the need for this unless you know of a > good reason why it can't be done in advance to this patch? Done. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:454: // TODO(vmpstr): Remove this function. I'll try and remove this in parallel with this patch. This is no longer used as of https://codereview.chromium.org/338593003
(PTAL)
https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:308: ++it) { Completely unrelated. How about we only release tiles without raster tasks? That would allow us to remove orphan_raster_tasks_ which will be a nice cleanup and possibly small performance improvement. Definitely for a follow up and not part of this patch though. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:397: // Update internal state. Silly comment. Keep it if you like but I don't mind if you remove it. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:402: CleanUpReleasedTiles(); Note: we should instead call this after CheckForCompletedTasks() if we keep around released tiles until they have no raster task. No need to change anything in this patch though. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:454: // TODO(vmpstr): Remove this function. On 2014/06/17 23:48:18, vmpstr wrote: > I'll try and remove this in parallel with this patch. This is no longer used as > of https://codereview.chromium.org/338593003 Great! Please remove the function in this or a separate patch so it's clear what code is left in case it affects this patch. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:516: const TilePriority& max_priority, other_priority https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:528: *usage -= MemoryUsage::FromTile(tile); nit: please use blank lines consistently. why group this line with "Tile* tile = **iterator;" in the function above but with "FreeResourcesFo.." here instead? https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:577: &eviction_it, hard_memory_limit, &memory_usage); Why this call here rather than after the loop? Makes me wonder if it has to happen before the loop.. If you're doing it after then it sort of becomes a noop if usage has already been reduced as a result of calling FreeTileResourcesWithLowerPriorityUntilUsageIsWithinLimit, which is almost always going to be the case, right? Maybe at least worth a comment if not moved. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:579: bool oomed_soft = false; I don't think you need this anymore. You break out of the loop whenever you set it. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:617: memory_usage += additional_memory_required; hm, this might adjust memory_usage even if the tile can't be scheduled. ie. we might have reached kScheduledRasterTasksLimit. should we add up the usage at the time we push the tiles to the need_to_be_rasterized queue? also, maybe change the name of this variable to something like "memory_usage_required_by_tile_to_be_scheduled" to make it really clear what it is and what's happening when using it. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:632: // be added as they are not affected by |bytes_allocatable|. I don't think this comment apply anymore now that we can simply break out of the loop when failing to schedule a tile. I think it can be removed. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:635: tiles_that_need_to_be_rasterized->size() < kScheduledRasterTasksLimit; I wonder if some of this can be moved up to make this even cleaner and represent the intent of this loop better. Something like this maybe: { Tile* tile = *it; if (tiles_that_need_to_be_rasterized->size() >= kScheduledRasterTasksLimit) { all_tiles_that_need_to_be_rasterized_are_scheduled_ = false; break; } if (!raster_task) { FreeTileResourcesWithLowerPrio... if (!memory_usage_successfully_reduced_for_tile_to_be_scheduled) { all_tiles_that_need_to_be_rasterized_are_scheduled_ = false; break; } memory_usage += memory_usage_required_by_tile_to_be_scheduled; } tiles_that_need_to_be_rasterized->push_back(tile); } This way we also don't evict tiles unnecessarily unless we're actually allowed to schedule a new tile. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:651: global_state_.hard_memory_limit_in_bytes); does this counter still make sense? how is showing a counter of hard_memory_limit_in_bytes useful only after we've failed to assign memory to a visible tile? https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:659: resource_pool_->acquired_memory_usage_bytes(); not sure this is completely true (as it doesn't include currently scheduled tasks) but I'm also not sure how useful this value really is to report. this looks like another chunk of code that doesn't belong in the tile manager. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.h:302: const TilePriority& max_priority, nit: max_priority is a lie. we're not freeing tiles with this priority. only tiles with lower priority. maybe "other_priority" unless you have a better suggestion? https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.h:317: bool all_tiles_that_need_to_be_rasterized_have_memory_; I think this should be renamed to all_tiles_that_need_to_be_rasterized_are_scheduled as that's what it represents afaict. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.h:318: bool all_tiles_required_for_activation_have_memory_; Is this still used?
PTAL https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:308: ++it) { On 2014/06/18 03:35:42, reveman wrote: > Completely unrelated. How about we only release tiles without raster tasks? That > would allow us to remove orphan_raster_tasks_ which will be a nice cleanup and > possibly small performance improvement. Definitely for a follow up and not part > of this patch though. That sounds good. Filed crbug.com/386039 https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:397: // Update internal state. On 2014/06/18 03:35:42, reveman wrote: > Silly comment. Keep it if you like but I don't mind if you remove it. Removed. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:402: CleanUpReleasedTiles(); On 2014/06/18 03:35:42, reveman wrote: > Note: we should instead call this after CheckForCompletedTasks() if we keep > around released tiles until they have no raster task. No need to change anything > in this patch though. I've moved it. The previous code would effectively call it after as well, so it's better for consistency. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:454: // TODO(vmpstr): Remove this function. On 2014/06/18 03:35:42, reveman wrote: > On 2014/06/17 23:48:18, vmpstr wrote: > > I'll try and remove this in parallel with this patch. This is no longer used > as > > of https://codereview.chromium.org/338593003 > > Great! Please remove the function in this or a separate patch so it's clear what > code is left in case it affects this patch. That's here https://codereview.chromium.org/342483007/ I just have to test it a bit more before publishing. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:516: const TilePriority& max_priority, On 2014/06/18 03:35:42, reveman wrote: > other_priority Done. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:528: *usage -= MemoryUsage::FromTile(tile); On 2014/06/18 03:35:42, reveman wrote: > nit: please use blank lines consistently. why group this line with "Tile* tile = > **iterator;" in the function above but with "FreeResourcesFo.." here instead? Done. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:577: &eviction_it, hard_memory_limit, &memory_usage); On 2014/06/18 03:35:42, reveman wrote: > Why this call here rather than after the loop? Makes me wonder if it has to > happen before the loop.. If you're doing it after then it sort of becomes a noop > if usage has already been reduced as a result of calling > FreeTileResourcesWithLowerPriorityUntilUsageIsWithinLimit, which is almost > always going to be the case, right? Maybe at least worth a comment if not moved. I think this has to happen before. Check out the latest patch. I mean if we can't reduce usage to be within memory limit before the loop, then I think we have to cancel the currently running tasks, right? Currently, the problem is that we would finish the raster (and all of the scheduled but not running tasks) before releasing that memory, which I think is wrong. What do you think? https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:579: bool oomed_soft = false; On 2014/06/18 03:35:42, reveman wrote: > I don't think you need this anymore. You break out of the loop whenever you set > it. Done. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:617: memory_usage += additional_memory_required; On 2014/06/18 03:35:42, reveman wrote: > hm, this might adjust memory_usage even if the tile can't be scheduled. ie. we > might have reached kScheduledRasterTasksLimit. should we add up the usage at the > time we push the tiles to the need_to_be_rasterized queue? > > also, maybe change the name of this variable to something like > "memory_usage_required_by_tile_to_be_scheduled" to make it really clear what it > is and what's happening when using it. Done. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:632: // be added as they are not affected by |bytes_allocatable|. On 2014/06/18 03:35:42, reveman wrote: > I don't think this comment apply anymore now that we can simply break out of the > loop when failing to schedule a tile. I think it can be removed. Done. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:635: tiles_that_need_to_be_rasterized->size() < kScheduledRasterTasksLimit; On 2014/06/18 03:35:42, reveman wrote: > I wonder if some of this can be moved up to make this even cleaner and represent > the intent of this loop better. Something like this maybe: > > { > Tile* tile = *it; > > if (tiles_that_need_to_be_rasterized->size() >= > kScheduledRasterTasksLimit) { > all_tiles_that_need_to_be_rasterized_are_scheduled_ = false; > break; > } > > if (!raster_task) { > FreeTileResourcesWithLowerPrio... > > if (!memory_usage_successfully_reduced_for_tile_to_be_scheduled) { > all_tiles_that_need_to_be_rasterized_are_scheduled_ = false; > break; > } > memory_usage += memory_usage_required_by_tile_to_be_scheduled; > } > > tiles_that_need_to_be_rasterized->push_back(tile); > } > > This way we also don't evict tiles unnecessarily unless we're actually allowed > to schedule a new tile. Done (more or less?). I think it kind of naturally evolved to be this after I did the rest of the updates. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:651: global_state_.hard_memory_limit_in_bytes); On 2014/06/18 03:35:42, reveman wrote: > does this counter still make sense? how is showing a counter of > hard_memory_limit_in_bytes useful only after we've failed to assign memory to a > visible tile? Removed. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:659: resource_pool_->acquired_memory_usage_bytes(); On 2014/06/18 03:35:42, reveman wrote: > not sure this is completely true (as it doesn't include currently scheduled > tasks) but I'm also not sure how useful this value really is to report. this > looks like another chunk of code that doesn't belong in the tile manager. Yeah, I agree. However, I would prefer to keep it here for now. The reasoning behind the math is that after all of the rasterization is scheduled (or even after this run actually schedules the tasks), the memory is correct, which I think is good enough for the hud. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.h:302: const TilePriority& max_priority, On 2014/06/18 03:35:43, reveman wrote: > nit: max_priority is a lie. we're not freeing tiles with this priority. only > tiles with lower priority. maybe "other_priority" unless you have a better > suggestion? other_priority sounds fine. Done. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.h:317: bool all_tiles_that_need_to_be_rasterized_have_memory_; On 2014/06/18 03:35:43, reveman wrote: > I think this should be renamed to > all_tiles_that_need_to_be_rasterized_are_scheduled as that's what it represents > afaict. Done. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.h:318: bool all_tiles_required_for_activation_have_memory_; On 2014/06/18 03:35:42, reveman wrote: > Is this still used? Nope, removed.
https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:308: ++it) { On 2014/06/18 06:45:00, vmpstr wrote: > On 2014/06/18 03:35:42, reveman wrote: > > Completely unrelated. How about we only release tiles without raster tasks? > That > > would allow us to remove orphan_raster_tasks_ which will be a nice cleanup and > > possibly small performance improvement. Definitely for a follow up and not > part > > of this patch though. > > That sounds good. Filed crbug.com/386039 Thanks! https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:577: &eviction_it, hard_memory_limit, &memory_usage); On 2014/06/18 06:45:00, vmpstr wrote: > On 2014/06/18 03:35:42, reveman wrote: > > Why this call here rather than after the loop? Makes me wonder if it has to > > happen before the loop.. If you're doing it after then it sort of becomes a > noop > > if usage has already been reduced as a result of calling > > FreeTileResourcesWithLowerPriorityUntilUsageIsWithinLimit, which is almost > > always going to be the case, right? Maybe at least worth a comment if not > moved. > > I think this has to happen before. Check out the latest patch. I mean if we > can't reduce usage to be within memory limit before the loop, then I think we > have to cancel the currently running tasks, right? Currently, the problem is > that we would finish the raster (and all of the scheduled but not running tasks) > before releasing that memory, which I think is wrong. What do you think? Ah, I think I understand. See my comments on latest patch. https://codereview.chromium.org/246673005/diff/390001/cc/resources/tile_manag... cc/resources/tile_manager.cc:659: resource_pool_->acquired_memory_usage_bytes(); On 2014/06/18 06:45:00, vmpstr wrote: > On 2014/06/18 03:35:42, reveman wrote: > > not sure this is completely true (as it doesn't include currently scheduled > > tasks) but I'm also not sure how useful this value really is to report. this > > looks like another chunk of code that doesn't belong in the tile manager. > > Yeah, I agree. However, I would prefer to keep it here for now. The reasoning > behind the math is that after all of the rasterization is scheduled (or even > after this run actually schedules the tasks), the memory is correct, which I > think is good enough for the hud. I don't think it will be correct. memory_usage and acquired_memory_usage_bytes both include tiles scheduled by the previous call to AssignGpuMem but not this call. That memory is unreleasable but not accounted for here as this math only considers the memory for tiles scheduled in this AssignGpuMem call as unreleasable. Unless my reasoning above is incorrect, I suggest that we simply remove the bytes_unreleasable field from memory_stats struct. https://codereview.chromium.org/246673005/diff/410001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/410001/cc/resources/tile_manag... cc/resources/tile_manager.cc:578: &eviction_it, hard_memory_limit, &memory_usage); How about we call FreeTileResourcesWithLowerPriorityUntilUsageIsWithinLimit in the loop below independent of the value of tile_version.raster_task_? It's hard to see if this respects hard/soft limit correctly when handled up here. I realize now that this is more like it worked before and I asked you to change it. I think that was a mistake. Sorry about that. So memory_usage_required_by_tile_to_be_scheduled will be zero when we already have a raster task but it still requires memory usage to be reduced to the limit required for the tile to be scheduled. https://codereview.chromium.org/246673005/diff/410001/cc/resources/tile_manag... cc/resources/tile_manager.cc:580: bool oomed_hard = false; How about inverting this and renaming it to had_enough_memory as that's the only use of it now? https://codereview.chromium.org/246673005/diff/410001/cc/resources/tile_manag... cc/resources/tile_manager.cc:617: MemoryUsage& memory_limit = maybe add a comment or rename it to something like memory_limit_to_be_within_for_tile_to_be_scheduled https://codereview.chromium.org/246673005/diff/410001/cc/resources/tile_manag... cc/resources/tile_manager.cc:620: bool memory_reduced_successfully = nit: memory_usage_is_within_limit might be better if you make the change I suggested above.
PTAL https://codereview.chromium.org/246673005/diff/410001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/410001/cc/resources/tile_manag... cc/resources/tile_manager.cc:578: &eviction_it, hard_memory_limit, &memory_usage); On 2014/06/18 16:41:10, reveman wrote: > How about we call FreeTileResourcesWithLowerPriorityUntilUsageIsWithinLimit in > the loop below independent of the value of tile_version.raster_task_? It's hard > to see if this respects hard/soft limit correctly when handled up here. > > I realize now that this is more like it worked before and I asked you to change > it. I think that was a mistake. Sorry about that. > > So memory_usage_required_by_tile_to_be_scheduled will be zero when we already > have a raster task but it still requires memory usage to be reduced to the limit > required for the tile to be scheduled. Done. https://codereview.chromium.org/246673005/diff/410001/cc/resources/tile_manag... cc/resources/tile_manager.cc:580: bool oomed_hard = false; On 2014/06/18 16:41:09, reveman wrote: > How about inverting this and renaming it to had_enough_memory as that's the only > use of it now? Done. https://codereview.chromium.org/246673005/diff/410001/cc/resources/tile_manag... cc/resources/tile_manager.cc:617: MemoryUsage& memory_limit = On 2014/06/18 16:41:09, reveman wrote: > maybe add a comment or rename it to something like > memory_limit_to_be_within_for_tile_to_be_scheduled I renamed this to tile_memory_limit, since it's not exactly memory_limit_to_be_within_for_tile_to_be_scheduled (it doesn't account for memory_usage_required_by_tile_to_be_scheduled, which is subtracted in the FreeTile... call. I added a comment. https://codereview.chromium.org/246673005/diff/410001/cc/resources/tile_manag... cc/resources/tile_manager.cc:620: bool memory_reduced_successfully = On 2014/06/18 16:41:09, reveman wrote: > nit: memory_usage_is_within_limit might be better if you make the change I > suggested above. Done.
Just a couple of nits. Can you also rebase this on https://codereview.chromium.org/342483007/ so I can see the final patch in its full glory ? :) https://codereview.chromium.org/246673005/diff/430001/cc/layers/heads_up_disp... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/246673005/diff/430001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:469: const double megabyte = 1024.0 * 1024.0; nit: kMegabyte while touching this file. https://codereview.chromium.org/246673005/diff/430001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/430001/cc/resources/tile_manag... cc/resources/tile_manager.cc:574: bool had_enough_memory = true; nit: maybe even had_enough_memory_to_schedule_visible_tiles or had_enough_memory_to_schedule_tiles_needed_now to make it very clear what this is. https://codereview.chromium.org/246673005/diff/430001/cc/resources/tile_manag... cc/resources/tile_manager.cc:601: // already accounted for in memory_usage. Otherwise, we'll have to spend nit: "acquire" instead of "spend" as that matches the naming of the resource pool API. ie. acquired_memory_usage.. https://codereview.chromium.org/246673005/diff/430001/cc/resources/tile_manag... cc/resources/tile_manager.cc:609: bool tile_uses_hard_limit = priority.priority_bin == TilePriority::NOW; nit: I think it would be more useful to have the variable name describe why the tile should be using the hard limit. ie. tile_is_visible or tile_is_needed_now. I think this line needs a comment otherwise. ie. why does this tile priority justify using the hard limit? https://codereview.chromium.org/246673005/diff/430001/cc/resources/tile_manag... cc/resources/tile_manager.cc:637: // Note that if we should try and further reduce memory in case the above loop nit: I'm having trouble reading this comment. maybe remove the "if"? https://codereview.chromium.org/246673005/diff/430001/cc/resources/tile_manag... cc/resources/tile_manager.cc:639: // remain within the memory limit. nit: this is not guaranteed to result in us being within the limit. ie. limit could be 0 and we have pending raster tasks not yet canceled. maybe "This ensures that we always free as many tile resources as possible to stay within the limit"
PTAL. I absentmindedly submitted tryjobs, but they will probably all fail since this is rebased on a patch that didn't land yet. https://codereview.chromium.org/246673005/diff/430001/cc/layers/heads_up_disp... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/246673005/diff/430001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:469: const double megabyte = 1024.0 * 1024.0; On 2014/06/18 18:55:13, reveman wrote: > nit: kMegabyte while touching this file. Done. https://codereview.chromium.org/246673005/diff/430001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/430001/cc/resources/tile_manag... cc/resources/tile_manager.cc:574: bool had_enough_memory = true; On 2014/06/18 18:55:13, reveman wrote: > nit: maybe even had_enough_memory_to_schedule_visible_tiles or > had_enough_memory_to_schedule_tiles_needed_now to make it very clear what this > is. Done. https://codereview.chromium.org/246673005/diff/430001/cc/resources/tile_manag... cc/resources/tile_manager.cc:601: // already accounted for in memory_usage. Otherwise, we'll have to spend On 2014/06/18 18:55:14, reveman wrote: > nit: "acquire" instead of "spend" as that matches the naming of the resource > pool API. ie. acquired_memory_usage.. Done. https://codereview.chromium.org/246673005/diff/430001/cc/resources/tile_manag... cc/resources/tile_manager.cc:609: bool tile_uses_hard_limit = priority.priority_bin == TilePriority::NOW; On 2014/06/18 18:55:13, reveman wrote: > nit: I think it would be more useful to have the variable name describe why the > tile should be using the hard limit. ie. tile_is_visible or tile_is_needed_now. > I think this line needs a comment otherwise. ie. why does this tile priority > justify using the hard limit? Done. https://codereview.chromium.org/246673005/diff/430001/cc/resources/tile_manag... cc/resources/tile_manager.cc:637: // Note that if we should try and further reduce memory in case the above loop On 2014/06/18 18:55:13, reveman wrote: > nit: I'm having trouble reading this comment. maybe remove the "if"? Yeah that if isn't supposed to be there. Removed. https://codereview.chromium.org/246673005/diff/430001/cc/resources/tile_manag... cc/resources/tile_manager.cc:639: // remain within the memory limit. On 2014/06/18 18:55:13, reveman wrote: > nit: this is not guaranteed to result in us being within the limit. ie. limit > could be 0 and we have pending raster tasks not yet canceled. maybe "This > ensures that we always free as many tile resources as possible to stay within > the limit" Done.
LGTM after updating the description and fixing two minor nits The stats for this patch "(+235 lines, -1834 lines)" are pretty impressive but IMO not as impressive as how much it reduces the complexity and improves readability of the tile manager. Thanks for all the hard work! https://codereview.chromium.org/246673005/diff/450001/cc/resources/tile.cc File cc/resources/tile.cc (right): https://codereview.chromium.org/246673005/diff/450001/cc/resources/tile.cc#ne... cc/resources/tile.cc:28: tile_manager_(tile_manager), nit: please remove |tile_manager_| member if no longer needed https://codereview.chromium.org/246673005/diff/450001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/246673005/diff/450001/cc/resources/tile_manag... cc/resources/tile_manager.cc:607: // didn't run at all (we had no raster tasks). This ensures that we always nit: this can also happen when we break out of the loop because of TilePriorityViolatesMemoryPolicy. maybe just "in case the above loop didn't reduce memory..." instead
The CQ bit was checked by vmpstr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/246673005/490001
Message was sent while issue was closed.
Change committed as 278654 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
