Add LoadState reporting to the mojo proxy resolver.

Add LoadState reporting to the mojo proxy resolver.

BUG=11746
Committed: https://crrev.com/a83d9e83e4a03050fec591b4375bec9a9d46bd3f
Cr-Commit-Position: refs/heads/master@{#320227}

[Sam McNally, 2015-02-18 03:56:23]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2015-02-20 01:00:45]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2015-02-20 05:26:18]

Patchset #1 (id:40001) has been deleted

[Sam McNally, 2015-02-20 05:33:45]

Patchset #1 (id:60001) has been deleted

[Sam McNally, 2015-02-20 05:46:50]

Patchset #1 (id:80001) has been deleted

[Sam McNally, 2015-02-20 05:47:23]

sammc@chromium.org changed reviewers:
+ amistry@chromium.org, eroman@chromium.org

[eroman, 2015-02-25 00:08:06]

How much of a performance issue is this going to be? For each proxy resolution
call (which happens at least once per URLRequest) we will be ping-ponging back
the load state changes (of which depending on the PAC script there may be
multiple per request).

I imagine you are doing it this way because the callers to GetLoadState() expect
synchronous completion and you don't want to do a sync IPC to get the load
status when asked on the browser side?

I suppose this is the way it needs to be done, but certainly feels unfortunate
to have to do this.

https://codereview.chromium.org/939503004/diff/100001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_impl.cc (right):

https://codereview.chromium.org/939503004/diff/100001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:104: net::ProxyResolver::RequestHandle
handle) {
Does this need another parameter? the handle is already part of the Job

https://codereview.chromium.org/939503004/diff/100001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:160:
client_->LoadStateChanged(load_state);
What is the implication of calling this? Other callbacks on the
TracingProxyResolver are resilient to cancellation/deletion happening while
dispatching the callback, but it doesn't look like that is the case for this
loadstate change one.

What may client->LoadStateChanged() do in response?

https://codereview.chromium.org/939503004/diff/100001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/939503004/diff/100001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:49: int error_ = ERR_FAILED;
we should actually be using net::Error throughout.

https://codereview.chromium.org/939503004/diff/100001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/939503004/diff/100001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.cc:741: LOAD_STATE_RESOLVING_PROXY_FOR_URL);
Note that OnDnsOperationComplete() is called in response to synchronous
completions as well. This may get called when the last set state was already
LOAD_STATE_RESOLVING_PROXY_FOR_URL.

[Anand Mistry (off Chromium), 2015-02-25 04:42:28]

https://codereview.chromium.org/939503004/diff/100001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_impl.cc (right):

https://codereview.chromium.org/939503004/diff/100001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:160:
client_->LoadStateChanged(load_state);
As an optimisation, I wonder if we should try to coalesce state changes. The
case I'm thinking of is when a DNS resolution finishes. Then, one of two cases
is likely to happen:
1. The script executes for a short amount of time and fires another DNS request.
It's likely that the script is only executing for 10s of us before starting the
next request. In that case, the state changes and then changes back within a
very short amount of time.
2. The script executes for a short amount of time and finishes, returning the
result. In this case, the state is changed, but since the request has finished,
the state is then no longer valid.

In both cases, the intermediate state is unlikely to be observed. So instead of
changing the state and sending messages for updates, maybe set a 1ms timer
before sending the state change message, and cancel that timer if one of the two
events happen. That way, sending the Mojo messages is avoided entirely for these
cases. My suspicion is that case #2 is likely to be very common.

[Sam McNally, 2015-02-25 07:22:28]

Patchset #3 (id:140001) has been deleted

[eroman, 2015-02-25 17:02:34]

To add to that, the LoadState() is currently just used to display a loading
status to the user.

Which is to say, the freshness of GetLoadState() is a human-reaction time thing.
So I don't think there would be any real downside to it being delayed by say
100ms.

[Sam McNally, 2015-02-26 04:00:01]

On 2015/02/25 17:02:34, eroman wrote:
> To add to that, the LoadState() is currently just used to display a loading
> status to the user.
> 
> Which is to say, the freshness of GetLoadState() is a human-reaction time
thing.
> So I don't think there would be any real downside to it being delayed by say
> 100ms.

Done with a 10ms delay instead. From Sydney I'm seeing dns resolution usually
taking 150-500ms and running JS taking mostly <0.3ms with some occasional spikes
of ~1.5ms (measured using the time between LoadState changes). 10ms seems like a
nice in between value.

https://codereview.chromium.org/939503004/diff/100001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_impl.cc (right):

https://codereview.chromium.org/939503004/diff/100001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:104: net::ProxyResolver::RequestHandle
handle) {
On 2015/02/25 00:08:05, eroman wrote:
> Does this need another parameter? the handle is already part of the Job

Done.

https://codereview.chromium.org/939503004/diff/100001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:160:
client_->LoadStateChanged(load_state);
On 2015/02/25 00:08:05, eroman wrote:
> What is the implication of calling this? Other callbacks on the
> TracingProxyResolver are resilient to cancellation/deletion happening while
> dispatching the callback, but it doesn't look like that is the case for this
> loadstate change one.
> 
> What may client->LoadStateChanged() do in response?

client_ is the mojo binding so it only sends a message to the
ProxyResolverMojo::Job. Cancellation is signaled by closing the message pipe so
if the Job has been cancelled on the other end but this end hasn't noticed yet,
the message will be discarded.

https://codereview.chromium.org/939503004/diff/100001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/939503004/diff/100001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:49: int error_ = ERR_FAILED;
On 2015/02/25 00:08:05, eroman wrote:
> we should actually be using net::Error throughout.

Done in https://codereview.chromium.org/896203003/.

https://codereview.chromium.org/939503004/diff/100001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/939503004/diff/100001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.cc:741: LOAD_STATE_RESOLVING_PROXY_FOR_URL);
On 2015/02/25 00:08:06, eroman wrote:
> Note that OnDnsOperationComplete() is called in response to synchronous
> completions as well. This may get called when the last set state was already
> LOAD_STATE_RESOLVING_PROXY_FOR_URL.

Added a check for pending_dns_completed_synchronously_ and also for cancelled_
being set.

[eroman, 2015-03-06 05:13:02]

mostly looks good except for some comments about the load state coalescer

https://codereview.chromium.org/939503004/diff/160001/net/base/load_state_cha...
File net/base/load_state_change_coalescer.cc (right):

https://codereview.chromium.org/939503004/diff/160001/net/base/load_state_cha...
net/base/load_state_change_coalescer.cc:24: pending_load_state_change_.Cancel();
Personally I think a OneShotTimer would be easier to follow in this code.

https://codereview.chromium.org/939503004/diff/160001/net/base/load_state_cha...
net/base/load_state_change_coalescer.cc:28:
pending_load_state_change_.Reset(base::Bind(
Won't this mean that If the load state is continuously changing, at a rate
faster than timeout_, we will never report a loadstate changed?

I think there should be a maximum reporting delay.

To be honest I don't think the LoadState timer optimization should be included
in this CL, would be easier to review it as a follow-up.

https://codereview.chromium.org/939503004/diff/160001/net/base/load_state_cha...
File net/base/load_state_change_coalescer.h (right):

https://codereview.chromium.org/939503004/diff/160001/net/base/load_state_cha...
net/base/load_state_change_coalescer.h:5: #ifndef
NET_BASE_LOAD_STATE_CHANGE_COALESCER_H_
please move this into the net/proxy directory. This is very specific to the mojo
proxy resolver, I can't imagine any other code using this functionality.

https://codereview.chromium.org/939503004/diff/160001/net/base/load_state_cha...
net/base/load_state_change_coalescer.h:19: // LoadState is reported before the
timeout has elapsed, the pending LoadState
"LoadState is discarded"

https://codereview.chromium.org/939503004/diff/160001/net/base/load_states.h
File net/base/load_states.h (right):

https://codereview.chromium.org/939503004/diff/160001/net/base/load_states.h#...
net/base/load_states.h:34: template <typename RequestHandle>
I don't feel this belongs in the generic load_states.h file since it is only
used by the proxy code.

https://codereview.chromium.org/939503004/diff/160001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/939503004/diff/160001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.cc:740:
!parent_->on_load_state_changed_.is_null()) {
nit: can you move this check to be the first?

[Sam McNally, 2015-03-06 06:47:48]

Patchset #4 (id:180001) has been deleted

[Sam McNally, 2015-03-06 09:16:59]

Patchset #5 (id:220001) has been deleted

[Sam McNally, 2015-03-06 09:30:37]

Patchset #5 (id:240001) has been deleted

[Sam McNally, 2015-03-06 09:33:02]

https://codereview.chromium.org/939503004/diff/160001/net/base/load_state_cha...
File net/base/load_state_change_coalescer.cc (right):

https://codereview.chromium.org/939503004/diff/160001/net/base/load_state_cha...
net/base/load_state_change_coalescer.cc:24: pending_load_state_change_.Cancel();
On 2015/03/06 05:13:01, eroman wrote:
> Personally I think a OneShotTimer would be easier to follow in this code.

Done.

https://codereview.chromium.org/939503004/diff/160001/net/base/load_state_cha...
net/base/load_state_change_coalescer.cc:28:
pending_load_state_change_.Reset(base::Bind(
On 2015/03/06 05:13:01, eroman wrote:
> Won't this mean that If the load state is continuously changing, at a rate
> faster than timeout_, we will never report a loadstate changed?
> 
> I think there should be a maximum reporting delay.

If we use a maximum delay, a continuously changing load state will degenerate
into sampling with a frequency of that maximum delay, which I don't think is
more useful.
> 
> To be honest I don't think the LoadState timer optimization should be included
> in this CL, would be easier to review it as a follow-up.

Done.

https://codereview.chromium.org/939503004/diff/160001/net/base/load_state_cha...
File net/base/load_state_change_coalescer.h (right):

https://codereview.chromium.org/939503004/diff/160001/net/base/load_state_cha...
net/base/load_state_change_coalescer.h:5: #ifndef
NET_BASE_LOAD_STATE_CHANGE_COALESCER_H_
On 2015/03/06 05:13:01, eroman wrote:
> please move this into the net/proxy directory. This is very specific to the
mojo
> proxy resolver, I can't imagine any other code using this functionality.

Done.

https://codereview.chromium.org/939503004/diff/160001/net/base/load_state_cha...
net/base/load_state_change_coalescer.h:19: // LoadState is reported before the
timeout has elapsed, the pending LoadState
On 2015/03/06 05:13:01, eroman wrote:
> "LoadState is discarded"

Done.

https://codereview.chromium.org/939503004/diff/160001/net/base/load_states.h
File net/base/load_states.h (right):

https://codereview.chromium.org/939503004/diff/160001/net/base/load_states.h#...
net/base/load_states.h:34: template <typename RequestHandle>
On 2015/03/06 05:13:01, eroman wrote:
> I don't feel this belongs in the generic load_states.h file since it is only
> used by the proxy code.

Done.

https://codereview.chromium.org/939503004/diff/160001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/939503004/diff/160001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.cc:740:
!parent_->on_load_state_changed_.is_null()) {
On 2015/03/06 05:13:02, eroman wrote:
> nit: can you move this check to be the first?

Done.

[Anand Mistry (off Chromium), 2015-03-10 03:36:09]

https://codereview.chromium.org/939503004/diff/260001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/939503004/diff/260001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:74: void
TestRequestClient::WaitForLoadStateChange() {
Switch to net::EventWaiter. You are, after all, waiting for a specific load
state.

[darin (slow to review), 2015-03-10 03:41:51]

darin@chromium.org changed reviewers:
+ darin@chromium.org

[darin (slow to review), 2015-03-10 03:41:52]

https://codereview.chromium.org/939503004/diff/260001/net/interfaces/proxy_re...
File net/interfaces/proxy_resolver_service.mojom (right):

https://codereview.chromium.org/939503004/diff/260001/net/interfaces/proxy_re...
net/interfaces/proxy_resolver_service.mojom:44: LoadStateChanged(int32
load_state);
note: this looks like something that would be a good fit for the
GetNextLoadState pattern. I could imagine ProxyResolver looking like this
instead:

interface ProxyResolver {
  ...

  GetProxyForURL(string url) => (int32 error, array<..>? proxy_servers);

  GetNextLoadState() => (int32 load_state);
};

This way there is no client interface. The interface is a bit simpler. You can
implement throttling for GetNextLoadState by delaying the callback.

[Sam McNally, 2015-03-10 04:00:59]

https://codereview.chromium.org/939503004/diff/260001/net/interfaces/proxy_re...
File net/interfaces/proxy_resolver_service.mojom (right):

https://codereview.chromium.org/939503004/diff/260001/net/interfaces/proxy_re...
net/interfaces/proxy_resolver_service.mojom:44: LoadStateChanged(int32
load_state);
On 2015/03/10 03:41:52, darin (slow to review) wrote:
> note: this looks like something that would be a good fit for the
> GetNextLoadState pattern. I could imagine ProxyResolver looking like this
> instead:
> 
> interface ProxyResolver {
>   ...
> 
>   GetProxyForURL(string url) => (int32 error, array<..>? proxy_servers);
> 
>   GetNextLoadState() => (int32 load_state);
> };
> 
> This way there is no client interface. The interface is a bit simpler. You can
> implement throttling for GetNextLoadState by delaying the callback.

We use a client interface so the client can cancel the request by closing its
end of the pipe. Also, the load state is per-request so we'd still need an extra
pipe to identify the associated request.

[darin (slow to review), 2015-03-10 04:10:32]

https://codereview.chromium.org/939503004/diff/260001/net/interfaces/proxy_re...
File net/interfaces/proxy_resolver_service.mojom (right):

https://codereview.chromium.org/939503004/diff/260001/net/interfaces/proxy_re...
net/interfaces/proxy_resolver_service.mojom:44: LoadStateChanged(int32
load_state);
On 2015/03/10 04:00:58, Sam McNally wrote:
> On 2015/03/10 03:41:52, darin (slow to review) wrote:
> > note: this looks like something that would be a good fit for the
> > GetNextLoadState pattern. I could imagine ProxyResolver looking like this
> > instead:
> > 
> > interface ProxyResolver {
> >   ...
> > 
> >   GetProxyForURL(string url) => (int32 error, array<..>? proxy_servers);
> > 
> >   GetNextLoadState() => (int32 load_state);
> > };
> > 
> > This way there is no client interface. The interface is a bit simpler. You
can
> > implement throttling for GetNextLoadState by delaying the callback.
> 
> We use a client interface so the client can cancel the request by closing its
> end of the pipe. Also, the load state is per-request so we'd still need an
extra
> pipe to identify the associated request.

Oh, I see. Silly me for not reading the comment above the GetProxyForUrl method
declaration :-/

[Sam McNally, 2015-03-10 06:44:39]

Patchset #6 (id:280001) has been deleted

[Sam McNally, 2015-03-10 06:47:35]

https://codereview.chromium.org/939503004/diff/260001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/939503004/diff/260001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:74: void
TestRequestClient::WaitForLoadStateChange() {
On 2015/03/10 03:36:09, Anand Mistry wrote:
> Switch to net::EventWaiter. You are, after all, waiting for a specific load
> state.

Done.

[eroman, 2015-03-11 18:21:09]

lgtm

[Anand Mistry (off Chromium), 2015-03-12 02:51:32]

lgtm

[Sam McNally, 2015-03-12 02:55:22]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2015-03-12 02:55:22]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/939503004/#ps340001
(title: "rebase")

[commit-bot: I haz the power, 2015-03-12 02:55:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/939503004/340001

[commit-bot: I haz the power, 2015-03-12 03:43:05]

Committed patchset #8 (id:340001)

[commit-bot: I haz the power, 2015-03-12 03:43:59]

Patchset 8 (id:??) landed as
https://crrev.com/a83d9e83e4a03050fec591b4375bec9a9d46bd3f
Cr-Commit-Position: refs/heads/master@{#320227}