ResourceScheduler should use renderer notifications instead of MRUCache to track renderers.

ResourceScheduler should use renderer notifications instead of MRUCache to track renderers.

The MRUCache enforced a hard limit on the number of navigations we could track. If we have more than that, we stop scheduling and load everything at once. That will only make performance worse.

The renderer notifications should provide an accurate signal when tabs come and go. Instead of having a hard limit, we'll just follow the notifications.

BUG=None


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

[darin (slow to review), 2013-03-13 00:12:27]

https://codereview.chromium.org/12600018/diff/9001/content/browser/loader/res...
File content/browser/loader/resource_scheduler_view_observer.cc (right):

https://codereview.chromium.org/12600018/diff/9001/content/browser/loader/res...
content/browser/loader/resource_scheduler_view_observer.cc:25:
ResourceSchedulerViewObserver::~ResourceSchedulerViewObserver() {
do we need to make sure that this class is destroyed on the UI thread too?  oh,
it looks like NotificationRegistrar takes care of that for us.

https://codereview.chromium.org/12600018/diff/9001/content/browser/loader/res...
content/browser/loader/resource_scheduler_view_observer.cc:28: void
OnRenderViewHostCreated(int child_id, int route_id) {
nit: these methods should be static or in an anon namespace.

https://codereview.chromium.org/12600018/diff/9001/content/browser/loader/res...
File content/browser/loader/resource_scheduler_view_observer.h (right):

https://codereview.chromium.org/12600018/diff/9001/content/browser/loader/res...
content/browser/loader/resource_scheduler_view_observer.h:18: class
CONTENT_EXPORT ResourceSchedulerViewObserver
Another name for this might be RenderViewHostTracker.  It could be interesting
to the ResourceDispatcherHostImpl to be informed when RenderViewHosts get
destroyed.  It could ensure that all of the associated network requests get
terminated, for example.

So, you could end up layering this so that RenderViewHostTracker calls
ResourceDispatcherHostImpl methods, which calls ResourceScheduler methods.

Maybe that is overkill without good use cases, but it occurs to me that this
class doesn't really need to be specific to ResourceScheduler, so maybe its name
should be more generic.

[darin (slow to review), 2013-03-13 00:13:50]

NOTE: You can bind methods on ResourceDispatcherHostImpl that are intended to
run on the IO thread using base::Unretained.  The RDHI always out-lives the IO
thread.

[James Simonsen, 2013-03-13 17:16:34]

PTAL.

On 2013/03/13 00:12:27, darin wrote:
>
https://codereview.chromium.org/12600018/diff/9001/content/browser/loader/res...
> File content/browser/loader/resource_scheduler_view_observer.cc (right):
> 
>
https://codereview.chromium.org/12600018/diff/9001/content/browser/loader/res...
> content/browser/loader/resource_scheduler_view_observer.cc:25:
> ResourceSchedulerViewObserver::~ResourceSchedulerViewObserver() {
> do we need to make sure that this class is destroyed on the UI thread too? 
oh,
> it looks like NotificationRegistrar takes care of that for us.

Done. It's good for documentation anyway.

> 
>
https://codereview.chromium.org/12600018/diff/9001/content/browser/loader/res...
> content/browser/loader/resource_scheduler_view_observer.cc:28: void
> OnRenderViewHostCreated(int child_id, int route_id) {
> nit: these methods should be static or in an anon namespace.

Ack. N/A now though, they're gone.

> 
>
https://codereview.chromium.org/12600018/diff/9001/content/browser/loader/res...
> File content/browser/loader/resource_scheduler_view_observer.h (right):
> 
>
https://codereview.chromium.org/12600018/diff/9001/content/browser/loader/res...
> content/browser/loader/resource_scheduler_view_observer.h:18: class
> CONTENT_EXPORT ResourceSchedulerViewObserver
> Another name for this might be RenderViewHostTracker.  It could be interesting
> to the ResourceDispatcherHostImpl to be informed when RenderViewHosts get
> destroyed.  It could ensure that all of the associated network requests get
> terminated, for example.
> 
> So, you could end up layering this so that RenderViewHostTracker calls
> ResourceDispatcherHostImpl methods, which calls ResourceScheduler methods.
> 
> Maybe that is overkill without good use cases, but it occurs to me that this
> class doesn't really need to be specific to ResourceScheduler, so maybe its
name
> should be more generic.

Done. Removed some of the code to cancel requests because of this too.

[James Simonsen, 2013-03-14 21:21:07]

Ping? I have another CL that builds off this one.

[James Simonsen, 2013-03-15 21:53:10]

Thanks for the tip. I've switched to using RenderViewHostObserver now.

[darin (slow to review), 2013-03-15 23:59:25]

https://codereview.chromium.org/12600018/diff/15001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/12600018/diff/15001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:1186:
CancelRequestsForRoute(child_id, route_id);
Perhaps it would be better to call CancelRequestsForRoute after OnClientDeleted?
 That way if OnClientDeleted ever wanted to look up request info (e.g.,
GetLoader), then it could do so safely.

https://codereview.chromium.org/12600018/diff/22001/content/browser/loader/re...
File content/browser/loader/render_view_host_tracker.cc (right):

https://codereview.chromium.org/12600018/diff/22001/content/browser/loader/re...
content/browser/loader/render_view_host_tracker.cc:23: registrar_.Add(this,
NOTIFICATION_RENDER_VIEW_HOST_DELETED,
is this still needed?

[darin (slow to review), 2013-03-15 23:59:54]

LGTM otherwise

[James Simonsen, 2013-03-16 00:37:53]

On 2013/03/15 23:59:25, darin wrote:
>
https://codereview.chromium.org/12600018/diff/15001/content/browser/loader/re...
> File content/browser/loader/resource_dispatcher_host_impl.cc (right):
> 
>
https://codereview.chromium.org/12600018/diff/15001/content/browser/loader/re...
> content/browser/loader/resource_dispatcher_host_impl.cc:1186:
> CancelRequestsForRoute(child_id, route_id);
> Perhaps it would be better to call CancelRequestsForRoute after
OnClientDeleted?
>  That way if OnClientDeleted ever wanted to look up request info (e.g.,
> GetLoader), then it could do so safely.

Done. Note that this means we're no longer able to DCHECK() that the
ResourceScheduler has cleaned up all of the pending requests.

> 
>
https://codereview.chromium.org/12600018/diff/22001/content/browser/loader/re...
> File content/browser/loader/render_view_host_tracker.cc (right):
> 
>
https://codereview.chromium.org/12600018/diff/22001/content/browser/loader/re...
> content/browser/loader/render_view_host_tracker.cc:23: registrar_.Add(this,
> NOTIFICATION_RENDER_VIEW_HOST_DELETED,
> is this still needed?

Nope. Removed.

[commit-bot: I haz the power, 2013-03-16 00:40:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/12600018/30001

[darin (slow to review), 2013-03-16 03:47:36]

On Fri, Mar 15, 2013 at 5:37 PM, <simonjam@chromium.org> wrote:

> On 2013/03/15 23:59:25, darin wrote:
>
> https://codereview.chromium.**org/12600018/diff/15001/**
>
content/browser/loader/**resource_dispatcher_host_impl.**cc<https://codereview.chromium.org/12600018/diff/15001/content/browser/loader/resource_dispatcher_host_impl.cc>
>
>> File content/browser/loader/**resource_dispatcher_host_impl.**cc (right):
>>
>
>
> https://codereview.chromium.**org/12600018/diff/15001/**
>
content/browser/loader/**resource_dispatcher_host_impl.**cc#newcode1186<https://codereview.chromium.org/12600018/diff/15001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1186>
>
>> content/browser/loader/**resource_dispatcher_host_impl.**cc:1186:
>> CancelRequestsForRoute(child_**id, route_id);
>> Perhaps it would be better to call CancelRequestsForRoute after
>>
> OnClientDeleted?
>
>>   That way if OnClientDeleted ever wanted to look up request info (e.g.,
>> GetLoader), then it could do so safely.
>>
>
> Done. Note that this means we're no longer able to DCHECK() that the
> ResourceScheduler has cleaned up all of the pending requests.
>
>
I see.  Hmm... still feels like the right trade-off.  It sucks when you
cannot use a class normally just because something else is shutting down,
and reordering these calls fixes that.  Hopefully that's worth it.

-Darin


>
>
>
> https://codereview.chromium.**org/12600018/diff/22001/**
>
content/browser/loader/render_**view_host_tracker.cc<https://codereview.chromium.org/12600018/diff/22001/content/browser/loader/render_view_host_tracker.cc>
>
>> File content/browser/loader/render_**view_host_tracker.cc (right):
>>
>
>
> https://codereview.chromium.**org/12600018/diff/22001/**
>
content/browser/loader/render_**view_host_tracker.cc#newcode23<https://codereview.chromium.org/12600018/diff/22001/content/browser/loader/render_view_host_tracker.cc#newcode23>
>
>> content/browser/loader/render_**view_host_tracker.cc:23:
>> registrar_.Add(this,
>> NOTIFICATION_RENDER_VIEW_HOST_**DELETED,
>> is this still needed?
>>
>
> Nope. Removed.
>
>
>
https://codereview.chromium.**org/12600018/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-03-16 05:43:49]

Retried try job too often on win_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-03-18 17:52:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/12600018/30001

[commit-bot: I haz the power, 2013-03-18 18:37:42]

Change committed as 188777