cc: Do not limit number of raster tasks for upload limit.

cc: Do not limit number of raster tasks for upload limit.

BUG=368936
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269617

[reveman, 2014-05-08 13:55:37]

https://codereview.chromium.org/270333003/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/270333003/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:839: // between each call to this function).
FYI, I'm OK removing this as I don't think this "assign at most 1/2 available
budget" logic is worthwhile. The worst that should happen is that we end up
cancelling all currently scheduled task and reclaim the memory and use it
immediately after when DidFinishRunningTasks is called.

https://codereview.chromium.org/270333003/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/270333003/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:878: if (tiles_that_need_to_be_rasterized->size() <
kScheduledRasterTasksLimit) {
why do we need a conditional here now? was this code wrong before?

[vmpstr, 2014-05-08 16:49:03]

lgtm % reveman

https://codereview.chromium.org/270333003/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/270333003/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:839: // between each call to this function).
On 2014/05/08 13:55:37, reveman wrote:
> FYI, I'm OK removing this as I don't think this "assign at most 1/2 available
> budget" logic is worthwhile. The worst that should happen is that we end up
> cancelling all currently scheduled task and reclaim the memory and use it
> immediately after when DidFinishRunningTasks is called.

I agree with this as well. I think tile manager throttling should only apply to
amount of tasks scheduled, not the type of tasks or how long they would take to
upload.

https://codereview.chromium.org/270333003/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/270333003/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:878: if (tiles_that_need_to_be_rasterized->size() <
kScheduledRasterTasksLimit) {
On 2014/05/08 13:55:37, reveman wrote:
> why do we need a conditional here now? was this code wrong before?

I think it should've been <= raster_bytes || size <= limit before...

[reveman, 2014-05-08 17:07:51]

https://codereview.chromium.org/270333003/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/270333003/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:878: if (tiles_that_need_to_be_rasterized->size() <
kScheduledRasterTasksLimit) {
On 2014/05/08 16:49:04, vmpstr wrote:
> On 2014/05/08 13:55:37, reveman wrote:
> > why do we need a conditional here now? was this code wrong before?
> 
> I think it should've been <= raster_bytes || size <= limit before... 

Ok, please add a variable here instead of doing
"tiles_that_need_to_be_rasterized->size() < kScheduledRasterTasksLimit" both
here and below. Maybe:

bool have_reached_scheduled_raster_tasks_limit =
    tiles_that_need_to_be_rasterized->size() >=
    kScheduledRasterTasksLimit;

[alokp, 2014-05-08 18:04:47]

https://codereview.chromium.org/270333003/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/270333003/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:878: if (tiles_that_need_to_be_rasterized->size() <
kScheduledRasterTasksLimit) {
On 2014/05/08 17:07:51, reveman wrote:
> On 2014/05/08 16:49:04, vmpstr wrote:
> > On 2014/05/08 13:55:37, reveman wrote:
> > > why do we need a conditional here now? was this code wrong before?
> > 
> > I think it should've been <= raster_bytes || size <= limit before... 
> 
> Ok, please add a variable here instead of doing
> "tiles_that_need_to_be_rasterized->size() < kScheduledRasterTasksLimit" both
> here and below. Maybe:
> 
> bool have_reached_scheduled_raster_tasks_limit =
>     tiles_that_need_to_be_rasterized->size() >=
>     kScheduledRasterTasksLimit;
> 

Done.

[alokp, 2014-05-08 18:07:29]

epenner@: This essentially reverts your patch in r233731. Thoughts/Concerns?

[reveman, 2014-05-08 18:37:07]

lgtm % epenner's review

I think the situation is a bit different since this limit was added. We're not
scheduling the whole world of possible raster tasks anymore. If this is still
useful, we should move it into PixelBufferRasterWorkerPool instead as
Image/ImageCopyRasterWorkerPool should not need to be affected by it.

[alokp, 2014-05-08 18:42:36]

epenner is OOO until the end of this month. Committing...

[alokp, 2014-05-08 18:42:42]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2014-05-08 18:47:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alokp@chromium.org/270333003/40001

[epenner, 2014-05-08 22:07:54]

The CQ bit was unchecked by epenner@chromium.org

[epenner, 2014-05-08 22:14:55]

See my comment on the bug. Indeed I'll be gone though so use best judgement. My
preference would be to allow all memory to be used rather than doubling it when
priorities change. Then this limit isn't as important. It also indirectly
prevents prepainting the world.

[alokp, 2014-05-09 17:39:32]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2014-05-09 17:41:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alokp@chromium.org/270333003/40001

[alokp, 2014-05-09 19:07:21]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2014-05-09 19:09:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alokp@chromium.org/270333003/60001

[commit-bot: I haz the power, 2014-05-10 19:19:33]

Change committed as 269617