QUIC - histograms to measure the time spent to read QUIC sever

QUIC - histograms to measure the time spent to read QUIC sever
information from disk cache.

R=rch@chromium.org

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

[Ryan Hamilton, 2014-03-07 04:41:04]

Thanks for doing this!!

https://codereview.chromium.org/189963002/diff/1/net/quic/quic_crypto_client_...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/189963002/diff/1/net/quic/quic_crypto_client_...
net/quic/quic_crypto_client_stream.cc:427:
quic_server_info->set_read_start_time(base::TimeTicks::Now());
I would have expected that this start time would be saved on the crypto stream,
perhaps in a member. Is there a performance or correctness reason for putting
this in the quic_server_info.

https://codereview.chromium.org/189963002/diff/1/net/quic/quic_crypto_client_...
net/quic/quic_crypto_client_stream.cc:431: // TODO(rtenneti): If multiple tabs
load the same URL, all requests except for
I think this TODO is not correct.  We will only have a single
QuicStreamFactory::Job for a given origin.  We will not have multiple jobs for
the same origin, and hence, we will not have more than one session at a time. 
Does that sound right to you?

[ramant (doing other things), 2014-03-07 06:09:28]

Hi Ryan,
  Many many thanks for your comments. Would like to go over the code tomorrow
with you if you have few minutes.

thanks
raman

https://codereview.chromium.org/189963002/diff/1/net/quic/quic_crypto_client_...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/189963002/diff/1/net/quic/quic_crypto_client_...
net/quic/quic_crypto_client_stream.cc:427:
quic_server_info->set_read_start_time(base::TimeTicks::Now());
On 2014/03/07 04:41:04, Ryan Hamilton wrote:
> I would have expected that this start time would be saved on the crypto
stream,
> perhaps in a member. Is there a performance or correctness reason for putting
> this in the quic_server_info.

We share the same quic_server_info (server config) for URLs to the same server
but different ports (www.google.com:80 and www.google.com:443) and thus thought
it is better to save the time in quic_server_info. 

I think there are two different quic sessions one for 80 and another for 443 but
they share the same QuicCrytoClientConfig:CachedState (QuicServerInfo). 

If you don't mind, could we go over the QuicStreamFactory's code tomorrow.

thanks much.

https://codereview.chromium.org/189963002/diff/1/net/quic/quic_crypto_client_...
net/quic/quic_crypto_client_stream.cc:431: // TODO(rtenneti): If multiple tabs
load the same URL, all requests except for
On 2014/03/07 04:41:04, Ryan Hamilton wrote:
> I think this TODO is not correct.  We will only have a single
> QuicStreamFactory::Job for a given origin.  We will not have multiple jobs for
> the same origin, and hence, we will not have more than one session at a time. 
> Does that sound right to you?

I think comment shouldn't have said same URL. I meant to say same server
hostname but different ports (different host_port_pairs for the same host).

[ramant (doing other things), 2014-03-07 16:23:57]

Hi Ryan,
  Made the changes you have suggested. PTAL.

thanks for your comments,
raman

https://codereview.chromium.org/189963002/diff/1/net/quic/quic_crypto_client_...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/189963002/diff/1/net/quic/quic_crypto_client_...
net/quic/quic_crypto_client_stream.cc:427:
quic_server_info->set_read_start_time(base::TimeTicks::Now());
On 2014/03/07 04:41:04, Ryan Hamilton wrote:
> I would have expected that this start time would be saved on the crypto
stream,
> perhaps in a member. Is there a performance or correctness reason for putting
> this in the quic_server_info.

We go to disk only once for each hostname. After the initial access, we always
get data from memory.

We can see with this histogram what is the worst case performace (or the
performance hit for outliers).

Done.

https://codereview.chromium.org/189963002/diff/1/net/quic/quic_crypto_client_...
net/quic/quic_crypto_client_stream.cc:431: // TODO(rtenneti): If multiple tabs
load the same URL, all requests except for
On 2014/03/07 04:41:04, Ryan Hamilton wrote:
> I think this TODO is not correct.  We will only have a single
> QuicStreamFactory::Job for a given origin.  We will not have multiple jobs for
> the same origin, and hence, we will not have more than one session at a time. 
> Does that sound right to you?

Done.

[ramant (doing other things), 2014-03-07 21:57:15]

Hi Ryan,
  Would appreciate if you could take another look at this change.

thanks
raman

https://codereview.chromium.org/189963002/diff/1/net/quic/quic_crypto_client_...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/189963002/diff/1/net/quic/quic_crypto_client_...
net/quic/quic_crypto_client_stream.cc:427:
quic_server_info->set_read_start_time(base::TimeTicks::Now());
On 2014/03/07 16:23:57, ramant wrote:
> On 2014/03/07 04:41:04, Ryan Hamilton wrote:
> > I would have expected that this start time would be saved on the crypto
> stream,
> > perhaps in a member. Is there a performance or correctness reason for
putting
> > this in the quic_server_info.
> 
> We go to disk only once for each hostname. After the initial access, we always
> get data from memory.
> 
> We can see with this histogram what is the worst case performace (or the
> performance hit for outliers).
> 
> Done.

Changed the code to collect stats when we load server config from disk cache (as
we had discussed this morning).

[Ryan Hamilton, 2014-03-07 22:24:43]

lgtm

https://codereview.chromium.org/189963002/diff/40001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/189963002/diff/40001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:430: // TODO(rtenneti): If multiple tabs
load URLs with same hostname but different
Please add a TODO to use the host:port and not just the host in the cache.

[ramant (doing other things), 2014-03-07 22:34:01]

https://codereview.chromium.org/189963002/diff/40001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/189963002/diff/40001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:430: // TODO(rtenneti): If multiple tabs
load URLs with same hostname but different
On 2014/03/07 22:24:43, Ryan Hamilton wrote:
> Please add a TODO to use the host:port and not just the host in the cache.

Done.

[ramant (doing other things), 2014-03-07 22:34:05]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-03-07 22:37:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/189963002/10003

[commit-bot: I haz the power, 2014-03-08 11:07:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/189963002/10003

[commit-bot: I haz the power, 2014-03-08 13:49:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-08 13:50:00]

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

[ramant (doing other things), 2014-03-08 14:33:17]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-03-08 14:35:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/189963002/10003

[commit-bot: I haz the power, 2014-03-08 15:32:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-08 15:32:13]

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

[ramant (doing other things), 2014-03-09 22:10:46]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-03-09 22:10:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/189963002/10003

[commit-bot: I haz the power, 2014-03-10 06:37:05]

Change committed as 255887

[wtc, 2014-03-10 17:29:27]

Patch set 4 LGTM. Just a suggestion for renaming the member "read_start_time_".
Thanks.

https://codereview.chromium.org/189963002/diff/10003/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.h (right):

https://codereview.chromium.org/189963002/diff/10003/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.h:141: base::TimeTicks read_start_time_;

Nit: this variable is poorly named because "read" can also mean "reading from
the network".

I suggest renaming it "disk_cache_load_start_time_" (see
"disk_cache_load_result_" above) or "disk_cache_read_start_time_".

[ramant (doing other things), 2014-03-10 20:42:19]

https://codereview.chromium.org/189963002/diff/10003/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.h (right):

https://codereview.chromium.org/189963002/diff/10003/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.h:141: base::TimeTicks read_start_time_;
On 2014/03/10 17:29:28, wtc wrote:
> 
> Nit: this variable is poorly named because "read" can also mean "reading from
> the network".
> 
> I suggest renaming it "disk_cache_load_start_time_" (see
> "disk_cache_load_result_" above) or "disk_cache_read_start_time_".

Renamed the variable in https://codereview.chromium.org/192583005/

Thanks Wan-Teh.
Done.