QUIC - Cache the connection type and connection description. Make the

QUIC - Cache the connection type and connection description. Make the
expensive calls if connection type has changed or if connection description was null.

Defined a new class to cache the connection
description and this class is owned by QuicStreamFactory.

This fixes the Jank on IO thread and avoids call to GetWifiPHYLayerProtocol
(which takes around 40ms).

BUG=422516
R=rch@chromium.org

Committed: https://crrev.com/041b299d0a6476be730bfbc100b33f3184279339
Cr-Commit-Position: refs/heads/master@{#317674}

[ramant (doing other things), 2015-02-20 21:57:42]

Patchset #1 (id:1) has been deleted

[ramant (doing other things), 2015-02-20 22:03:04]

Patchset #1 (id:20001) has been deleted

[ramant (doing other things), 2015-02-20 22:26:35]

rtenneti@chromium.org changed reviewers:
+ rch@chromium.org

[ramant (doing other things), 2015-02-20 22:34:58]

Hi Ryan,
  QuicConnectionLogger GetConnectionDescriptionString is taking around 30ms only
on windows (whenever we create a QuicClientSession).

  Cached the connection type and description in QuicStreamFactory and returned
that description if connection type hasn't changed. Whenever network has
changed, cleared the cached data.

What do you think of this change?

thanks
raman

[Ryan Hamilton, 2015-02-21 04:02:19]

As discussed offline, it's probably best to move this description caching into a
distinct class, an instance of which can be owned by the factory and passed into
the logger.

Or this logic could move into the NCN directly.

[ramant (doing other things), 2015-02-22 23:12:34]

Patchset #3 (id:80001) has been deleted

[ramant (doing other things), 2015-02-22 23:20:29]

Patchset #3 (id:100001) has been deleted

[ramant (doing other things), 2015-02-22 23:46:14]

Patchset #3 (id:120001) has been deleted

[ramant (doing other things), 2015-02-23 03:04:52]

Patchset #3 (id:140001) has been deleted

[ramant (doing other things), 2015-02-23 03:08:56]

Hi Ryan,
  Implemented a new class to cache the connection description. 

  Will move this code into net/base after talking with owners of
NetworkChangeNotifier.

PTAL. thanks.

[Ryan Hamilton, 2015-02-23 18:43:00]

Looking great. Two small suggestions.

https://codereview.chromium.org/944883003/diff/160001/net/quic/network_connec...
File net/quic/network_connection.h (right):

https://codereview.chromium.org/944883003/diff/160001/net/quic/network_connec...
net/quic/network_connection.h:34: const char* GetDescription();
Please comment on the lifetime/ownership of this value. namely, do client need
to copy this string in case the underlying network type changes?

https://codereview.chromium.org/944883003/diff/160001/net/quic/quic_connectio...
File net/quic/quic_connection_logger.cc (right):

https://codereview.chromium.org/944883003/diff/160001/net/quic/quic_connectio...
net/quic/quic_connection_logger.cc:302:
connection_description_(network_connection->GetDescription()) {
nit: if you don't need the actual NetworkConnection object and just need the
description,  I recommend you just pass in the network description directly.

[ramant (doing other things), 2015-02-23 19:43:07]

Patchset #4 (id:180001) has been deleted

[ramant (doing other things), 2015-02-23 19:50:19]

PTAL.

https://codereview.chromium.org/944883003/diff/160001/net/quic/network_connec...
File net/quic/network_connection.h (right):

https://codereview.chromium.org/944883003/diff/160001/net/quic/network_connec...
net/quic/network_connection.h:34: const char* GetDescription();
On 2015/02/23 18:43:00, Ryan Hamilton wrote:
> Please comment on the lifetime/ownership of this value. namely, do client need
> to copy this string in case the underlying network type changes?

Done.

https://codereview.chromium.org/944883003/diff/160001/net/quic/quic_connectio...
File net/quic/quic_connection_logger.cc (right):

https://codereview.chromium.org/944883003/diff/160001/net/quic/quic_connectio...
net/quic/quic_connection_logger.cc:302:
connection_description_(network_connection->GetDescription()) {
On 2015/02/23 18:43:00, Ryan Hamilton wrote:
> nit: if you don't need the actual NetworkConnection object and just need the
> description,  I recommend you just pass in the network description directly.

+1 and thanks for the suggestion.

Done.

[ramant (doing other things), 2015-02-23 19:57:38]

Patchset #4 (id:200001) has been deleted

[Ryan Hamilton, 2015-02-23 20:42:19]

LGTM. a few nits, but feel free to land once fixed.

https://codereview.chromium.org/944883003/diff/240001/net/quic/network_connec...
File net/quic/network_connection.h (right):

https://codereview.chromium.org/944883003/diff/240001/net/quic/network_connec...
net/quic/network_connection.h:35: //
NetworkChangeNotifier::ConnectionTypeToString. Callers don't need to make a
nit: please move the ownership comment to be the second sentence.

https://codereview.chromium.org/944883003/diff/240001/net/quic/quic_client_se...
File net/quic/quic_client_session_test.cc (right):

https://codereview.chromium.org/944883003/diff/240001/net/quic/quic_client_se...
net/quic/quic_client_session_test.cc:54: network_connection_.GetDescription(),
I think you can just pass in any string you want here. No need to create an
actual NetworkConnection.

https://codereview.chromium.org/944883003/diff/240001/net/quic/quic_connectio...
File net/quic/quic_connection_logger_unittest.cc (right):

https://codereview.chromium.org/944883003/diff/240001/net/quic/quic_connectio...
net/quic/quic_connection_logger_unittest.cc:27: logger_(&session_,
network_connection_.GetDescription(), net_log_) {}
same comment here about just using "some_description" instead of actually
creating a real NetworkConnection

https://codereview.chromium.org/944883003/diff/240001/net/quic/quic_http_stre...
File net/quic/quic_http_stream_test.cc (right):

https://codereview.chromium.org/944883003/diff/240001/net/quic/quic_http_stre...
net/quic/quic_http_stream_test.cc:213: DefaultQuicConfig(),
network_connection_.GetDescription(),
ditto

[ramant (doing other things), 2015-02-23 21:05:30]

Thanks much Ryan for quick review.

https://codereview.chromium.org/944883003/diff/240001/net/quic/network_connec...
File net/quic/network_connection.h (right):

https://codereview.chromium.org/944883003/diff/240001/net/quic/network_connec...
net/quic/network_connection.h:35: //
NetworkChangeNotifier::ConnectionTypeToString. Callers don't need to make a
On 2015/02/23 20:42:19, Ryan Hamilton wrote:
> nit: please move the ownership comment to be the second sentence.

Done.

https://codereview.chromium.org/944883003/diff/240001/net/quic/quic_client_se...
File net/quic/quic_client_session_test.cc (right):

https://codereview.chromium.org/944883003/diff/240001/net/quic/quic_client_se...
net/quic/quic_client_session_test.cc:54: network_connection_.GetDescription(),
On 2015/02/23 20:42:19, Ryan Hamilton wrote:
> I think you can just pass in any string you want here. No need to create an
> actual NetworkConnection.

Done.

https://codereview.chromium.org/944883003/diff/240001/net/quic/quic_connectio...
File net/quic/quic_connection_logger_unittest.cc (right):

https://codereview.chromium.org/944883003/diff/240001/net/quic/quic_connectio...
net/quic/quic_connection_logger_unittest.cc:27: logger_(&session_,
network_connection_.GetDescription(), net_log_) {}
On 2015/02/23 20:42:19, Ryan Hamilton wrote:
> same comment here about just using "some_description" instead of actually
> creating a real NetworkConnection

Done.

https://codereview.chromium.org/944883003/diff/240001/net/quic/quic_http_stre...
File net/quic/quic_http_stream_test.cc (right):

https://codereview.chromium.org/944883003/diff/240001/net/quic/quic_http_stre...
net/quic/quic_http_stream_test.cc:213: DefaultQuicConfig(),
network_connection_.GetDescription(),
On 2015/02/23 20:42:19, Ryan Hamilton wrote:
> ditto

Done.

[ramant (doing other things), 2015-02-23 21:05:37]

The CQ bit was checked by rtenneti@chromium.org

[ramant (doing other things), 2015-02-23 21:05:37]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org
Link to the patchset: https://codereview.chromium.org/944883003/#ps260001
(title: "Fix comments to Patch Set 5")

[commit-bot: I haz the power, 2015-02-23 21:06:29]

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

[commit-bot: I haz the power, 2015-02-23 23:03:38]

Committed patchset #6 (id:260001)

[commit-bot: I haz the power, 2015-02-23 23:24:11]

Patchset 6 (id:??) landed as
https://crrev.com/041b299d0a6476be730bfbc100b33f3184279339
Cr-Commit-Position: refs/heads/master@{#317674}