Throttle Socket watcher RTT notifications from QUIC

Throttle Socket watcher RTT notifications from QUIC

A QUIC connection can provide at most one RTT notification per second.
This CL also refactors the code a little bit:
(1) Moves the throttling logic from TCP Socket Posix to a common place:
SocketWatcher, where it is used by both QUIC and TCP
(2) Makes it possible to configure the throttling delay using field
trial params.

BUG=692119
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2690113004
Cr-Commit-Position: refs/heads/master@{#450967}
Committed: https://chromium.googlesource.com/chromium/src/+/180587c15a36861cdaaceec045619026e55fe8c5

[tbansal1, 2017-02-14 01:39:19]

Patchset #1 (id:1) has been deleted

[tbansal1, 2017-02-14 01:39:28]

Description was changed from

==========
Throttle SPW notifications from QUIC

BUG=
==========

to

==========
Throttle SPW notifications from QUIC

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[tbansal1, 2017-02-14 01:39:57]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 01:41:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 03:54:00]

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

[commit-bot: I haz the power, 2017-02-14 03:54:01]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-02-14 17:25:34]

Description was changed from

==========
Throttle SPW notifications from QUIC

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Throttle Socket watcher RTT notifications from QUIC

A QUIC connection can provide at most one RTT notification per second.
This CL also refactors the code a little bit:
(1) Moves the throttling logic from TCP Socket Posix to a common place:
SocketWatcher, where it is used by both QUIC and TCP
(2) Makes it possible to configure the throttling delay using field
trial params.

BUG=692119
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[tbansal1, 2017-02-14 17:28:24]

tbansal@chromium.org changed reviewers:
+ ryansturm@chromium.org

[tbansal1, 2017-02-14 17:28:24]

ryansturm: ptal at //net/nqe (or everything). Thanks.

[RyanSturm, 2017-02-15 17:42:08]

https://codereview.chromium.org/2690113004/diff/20001/net/nqe/socket_watcher.cc
File net/nqe/socket_watcher.cc (right):

https://codereview.chromium.org/2690113004/diff/20001/net/nqe/socket_watcher....
net/nqe/socket_watcher.cc:35: // Do not allow incoming notifications if the last
notification was recent
nit:s/was recent than/was more recent than/

https://codereview.chromium.org/2690113004/diff/20001/net/nqe/socket_watcher_...
File net/nqe/socket_watcher_factory.cc (right):

https://codereview.chromium.org/2690113004/diff/20001/net/nqe/socket_watcher_...
net/nqe/socket_watcher_factory.cc:28: return
std::unique_ptr<SocketPerformanceWatcher>(
change this to make_unique while you're here.

https://codereview.chromium.org/2690113004/diff/20001/net/nqe/socket_watcher_...
File net/nqe/socket_watcher_unittest.cc (right):

https://codereview.chromium.org/2690113004/diff/20001/net/nqe/socket_watcher_...
net/nqe/socket_watcher_unittest.cc:40:
socket_watcher.SetTickClockForTesting(std::move(tick_clock));
nit: I'd prefer this passed a constructor param, and the factory can pass in the
default tick clock. wdyt?

[tbansal1, 2017-02-15 20:37:28]

Patchset #2 (id:40001) has been deleted

[tbansal1, 2017-02-15 20:37:34]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[tbansal1, 2017-02-15 20:37:42]

ryansturm: ptal. Thanks.

https://codereview.chromium.org/2690113004/diff/20001/net/nqe/socket_watcher.cc
File net/nqe/socket_watcher.cc (right):

https://codereview.chromium.org/2690113004/diff/20001/net/nqe/socket_watcher....
net/nqe/socket_watcher.cc:35: // Do not allow incoming notifications if the last
notification was recent
On 2017/02/15 17:42:08, Ryan Sturm wrote:
> nit:s/was recent than/was more recent than/

Done.

https://codereview.chromium.org/2690113004/diff/20001/net/nqe/socket_watcher_...
File net/nqe/socket_watcher_factory.cc (right):

https://codereview.chromium.org/2690113004/diff/20001/net/nqe/socket_watcher_...
net/nqe/socket_watcher_factory.cc:28: return
std::unique_ptr<SocketPerformanceWatcher>(
On 2017/02/15 17:42:08, Ryan Sturm wrote:
> change this to make_unique while you're here.

Done.

https://codereview.chromium.org/2690113004/diff/20001/net/nqe/socket_watcher_...
File net/nqe/socket_watcher_unittest.cc (right):

https://codereview.chromium.org/2690113004/diff/20001/net/nqe/socket_watcher_...
net/nqe/socket_watcher_unittest.cc:40:
socket_watcher.SetTickClockForTesting(std::move(tick_clock));
On 2017/02/15 17:42:08, Ryan Sturm wrote:
> nit: I'd prefer this passed a constructor param, and the factory can pass in
the
> default tick clock. wdyt?

Done.

[commit-bot: I haz the power, 2017-02-15 20:39:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2017-02-15 20:40:25]

net/nqe: lgtm

[tbansal1, 2017-02-15 20:46:26]

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

[tbansal1, 2017-02-15 20:46:27]

rch: ptal at //net/quic, //net/BUILD.gn and //net/socket. Thanks.

[commit-bot: I haz the power, 2017-02-15 22:11:49]

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

[commit-bot: I haz the power, 2017-02-15 22:11:50]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Ryan Hamilton, 2017-02-15 23:08:17]

Looks good.

https://codereview.chromium.org/2690113004/diff/60001/net/quic/chromium/quic_...
File net/quic/chromium/quic_connection_logger.cc (right):

https://codereview.chromium.org/2690113004/diff/60001/net/quic/chromium/quic_...
net/quic/chromium/quic_connection_logger.cc:691:
socket_performance_watcher_->OnUpdatedRTTAvailable(
I guess this is fine, but any reason not to make the impl of
OnUpdatedRTTAvailable do:

OnUpdatedRTTAvailable() {
 if (!ShouldNotifyUpdatedRTT())
   return;
  ...
}

And leave callers unchanged?

https://codereview.chromium.org/2690113004/diff/60001/net/quic/chromium/quic_...
File net/quic/chromium/quic_network_transaction_unittest.cc (right):

https://codereview.chromium.org/2690113004/diff/60001/net/quic/chromium/quic_...
net/quic/chromium/quic_network_transaction_unittest.cc:156: explicit
TestSocketPerformanceWatcher(bool* should_notify_updated_rtt,
nit: no need for explicit now that there are 2 arguments.

https://codereview.chromium.org/2690113004/diff/60001/net/socket/tcp_socket_p...
File net/socket/tcp_socket_posix.cc (left):

https://codereview.chromium.org/2690113004/diff/60001/net/socket/tcp_socket_p...
net/socket/tcp_socket_posix.cc:732: 
This is a nice cleanup.

Should we do something similar for TCPSocketWin?

https://codereview.chromium.org/2690113004/diff/60001/net/socket/tcp_socket_p...
File net/socket/tcp_socket_posix.cc (right):

https://codereview.chromium.org/2690113004/diff/60001/net/socket/tcp_socket_p...
net/socket/tcp_socket_posix.cc:722:
!socket_performance_watcher_->ShouldNotifyUpdatedRTT()) {
Ah! I think I understand your API choice. Getting this RTT estimate in not
necessairly cheap, so it's handy for callers to not do the Update if the update
is not needed?

[tbansal1, 2017-02-15 23:16:12]

rch: ptal. Thanks.

https://codereview.chromium.org/2690113004/diff/60001/net/quic/chromium/quic_...
File net/quic/chromium/quic_connection_logger.cc (right):

https://codereview.chromium.org/2690113004/diff/60001/net/quic/chromium/quic_...
net/quic/chromium/quic_connection_logger.cc:691:
socket_performance_watcher_->OnUpdatedRTTAvailable(
On 2017/02/15 23:08:16, Ryan Hamilton wrote:
> I guess this is fine, but any reason not to make the impl of
> OnUpdatedRTTAvailable do:
> 
> OnUpdatedRTTAvailable() {
>  if (!ShouldNotifyUpdatedRTT())
>    return;
>   ...
> }
> 
> And leave callers unchanged?

The reason is that we do not want TCP Socket Posix to make a syscall unless the
Socket Watcher is interested in receiving a reading. So, the socket first checks
if the Watcher is interested in the reading before making any effort to generate
one.

https://codereview.chromium.org/2690113004/diff/60001/net/socket/tcp_socket_p...
File net/socket/tcp_socket_posix.cc (left):

https://codereview.chromium.org/2690113004/diff/60001/net/socket/tcp_socket_p...
net/socket/tcp_socket_posix.cc:732: 
On 2017/02/15 23:08:17, Ryan Hamilton wrote:
> This is a nice cleanup.
> 
> Should we do something similar for TCPSocketWin?

TCPSocketWin currently does not push any RTT readings. I guess I need to read up
on Windows APIs to write that code. There is also a TODO from bmcquade here:
https://cs.chromium.org/chromium/src/net/socket/tcp_socket_win.cc?rcl=32e074e...
So, I may be I can copy his code once he implements that :)

https://codereview.chromium.org/2690113004/diff/60001/net/socket/tcp_socket_p...
File net/socket/tcp_socket_posix.cc (right):

https://codereview.chromium.org/2690113004/diff/60001/net/socket/tcp_socket_p...
net/socket/tcp_socket_posix.cc:722:
!socket_performance_watcher_->ShouldNotifyUpdatedRTT()) {
On 2017/02/15 23:08:17, Ryan Hamilton wrote:
> Ah! I think I understand your API choice. Getting this RTT estimate in not
> necessairly cheap, so it's handy for callers to not do the Update if the
update
> is not needed?

Right. (see the other comment)

[tbansal1, 2017-02-15 23:16:32]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 23:17:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ryan Hamilton, 2017-02-16 00:01:42]

lgtm

https://codereview.chromium.org/2690113004/diff/60001/net/socket/tcp_socket_p...
File net/socket/tcp_socket_posix.cc (left):

https://codereview.chromium.org/2690113004/diff/60001/net/socket/tcp_socket_p...
net/socket/tcp_socket_posix.cc:732: 
On 2017/02/15 23:16:12, tbansal1 wrote:
> On 2017/02/15 23:08:17, Ryan Hamilton wrote:
> > This is a nice cleanup.
> > 
> > Should we do something similar for TCPSocketWin?
> 
> TCPSocketWin currently does not push any RTT readings. I guess I need to read
up
> on Windows APIs to write that code. There is also a TODO from bmcquade here:
>
https://cs.chromium.org/chromium/src/net/socket/tcp_socket_win.cc?rcl=32e074e...
> So, I may be I can copy his code once he implements that :)

Since windows is huge part of Chrome's user base, it would be good to make sure
we do this. Can you follow up with bcmquade to ensure that this is done by
someone?

[tbansal1, 2017-02-16 00:10:58]

https://codereview.chromium.org/2690113004/diff/60001/net/socket/tcp_socket_p...
File net/socket/tcp_socket_posix.cc (left):

https://codereview.chromium.org/2690113004/diff/60001/net/socket/tcp_socket_p...
net/socket/tcp_socket_posix.cc:732: 
On 2017/02/16 00:01:42, Ryan Hamilton wrote:
> On 2017/02/15 23:16:12, tbansal1 wrote:
> > On 2017/02/15 23:08:17, Ryan Hamilton wrote:
> > > This is a nice cleanup.
> > > 
> > > Should we do something similar for TCPSocketWin?
> > 
> > TCPSocketWin currently does not push any RTT readings. I guess I need to
read
> up
> > on Windows APIs to write that code. There is also a TODO from bmcquade here:
> >
>
https://cs.chromium.org/chromium/src/net/socket/tcp_socket_win.cc?rcl=32e074e...
> > So, I may be I can copy his code once he implements that :)
> 
> Since windows is huge part of Chrome's user base, it would be good to make
sure
> we do this. Can you follow up with bcmquade to ensure that this is done by
> someone?

Filed https://bugs.chromium.org/p/chromium/issues/detail?id=692825.

[Ryan Hamilton, 2017-02-16 00:13:01]

On 2017/02/16 00:10:58, tbansal1 wrote:
>
https://codereview.chromium.org/2690113004/diff/60001/net/socket/tcp_socket_p...
> File net/socket/tcp_socket_posix.cc (left):
> 
>
https://codereview.chromium.org/2690113004/diff/60001/net/socket/tcp_socket_p...
> net/socket/tcp_socket_posix.cc:732: 
> On 2017/02/16 00:01:42, Ryan Hamilton wrote:
> > On 2017/02/15 23:16:12, tbansal1 wrote:
> > > On 2017/02/15 23:08:17, Ryan Hamilton wrote:
> > > > This is a nice cleanup.
> > > > 
> > > > Should we do something similar for TCPSocketWin?
> > > 
> > > TCPSocketWin currently does not push any RTT readings. I guess I need to
> read
> > up
> > > on Windows APIs to write that code. There is also a TODO from bmcquade
here:
> > >
> >
>
https://cs.chromium.org/chromium/src/net/socket/tcp_socket_win.cc?rcl=32e074e...
> > > So, I may be I can copy his code once he implements that :)
> > 
> > Since windows is huge part of Chrome's user base, it would be good to make
> sure
> > we do this. Can you follow up with bcmquade to ensure that this is done by
> > someone?
> 
> Filed https://bugs.chromium.org/p/chromium/issues/detail?id=692825.

Thanks!

[commit-bot: I haz the power, 2017-02-16 00:27:04]

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

[commit-bot: I haz the power, 2017-02-16 00:27:05]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tbansal1, 2017-02-16 04:29:16]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2017-02-16 04:29:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org
Link to the patchset: https://codereview.chromium.org/2690113004/#ps80001
(title: "Rebased, rch comments")

[commit-bot: I haz the power, 2017-02-16 04:30:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 06:33:39]

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

[commit-bot: I haz the power, 2017-02-16 06:33:41]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[tbansal1, 2017-02-16 15:07:29]

The CQ bit was checked by tbansal@chromium.org

[commit-bot: I haz the power, 2017-02-16 15:07:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 15:13:27]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487257649812970,
"parent_rev": "cafece6e34023641054a86d9d145cab53ef0b5b3", "commit_rev":
"180587c15a36861cdaaceec045619026e55fe8c5"}

[commit-bot: I haz the power, 2017-02-16 15:14:08]

Description was changed from

==========
Throttle Socket watcher RTT notifications from QUIC

A QUIC connection can provide at most one RTT notification per second.
This CL also refactors the code a little bit:
(1) Moves the throttling logic from TCP Socket Posix to a common place:
SocketWatcher, where it is used by both QUIC and TCP
(2) Makes it possible to configure the throttling delay using field
trial params.

BUG=692119
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Throttle Socket watcher RTT notifications from QUIC

A QUIC connection can provide at most one RTT notification per second.
This CL also refactors the code a little bit:
(1) Moves the throttling logic from TCP Socket Posix to a common place:
SocketWatcher, where it is used by both QUIC and TCP
(2) Makes it possible to configure the throttling delay using field
trial params.

BUG=692119
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2690113004
Cr-Commit-Position: refs/heads/master@{#450967}
Committed:
https://chromium.googlesource.com/chromium/src/+/180587c15a36861cdaaceec04561...
==========

[commit-bot: I haz the power, 2017-02-16 15:14:10]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/180587c15a36861cdaaceec04561...