New issue
Advanced search Search tips

Issue 692119 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

RTT notifications from QUIC connection to Socket watchers should be throttled

Project Member Reported by tbansal@chromium.org, Feb 14 2017

Issue description

Currently, RTT notifications from TCP sockets to socket watchers (used by NQE for computing transport RTT estimates) is throttled. A single TCP socket provides at most one notification per second. On the other hand, RTT notifications from QUIC connection to socket watchers are not throttled. So, a single QUIC connection may provide multiple notifications to Socket watcher, and to NQE.

This is problematic because a small number of QUIC connections may end up providing more RTT samples to NQE than TCP sockets. So, RTT notifications from QUIC connection to Socket watchers should be throttled, just like they are from TCP sockets.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/180587c15a36861cdaaceec045619026e55fe8c5

commit 180587c15a36861cdaaceec045619026e55fe8c5
Author: tbansal <tbansal@chromium.org>
Date: Thu Feb 16 15:13:23 2017

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}

[modify] https://crrev.com/180587c15a36861cdaaceec045619026e55fe8c5/net/BUILD.gn
[modify] https://crrev.com/180587c15a36861cdaaceec045619026e55fe8c5/net/nqe/network_quality_estimator.cc
[modify] https://crrev.com/180587c15a36861cdaaceec045619026e55fe8c5/net/nqe/network_quality_estimator_params.cc
[modify] https://crrev.com/180587c15a36861cdaaceec045619026e55fe8c5/net/nqe/network_quality_estimator_params.h
[modify] https://crrev.com/180587c15a36861cdaaceec045619026e55fe8c5/net/nqe/socket_watcher.cc
[modify] https://crrev.com/180587c15a36861cdaaceec045619026e55fe8c5/net/nqe/socket_watcher.h
[modify] https://crrev.com/180587c15a36861cdaaceec045619026e55fe8c5/net/nqe/socket_watcher_factory.cc
[modify] https://crrev.com/180587c15a36861cdaaceec045619026e55fe8c5/net/nqe/socket_watcher_factory.h
[add] https://crrev.com/180587c15a36861cdaaceec045619026e55fe8c5/net/nqe/socket_watcher_unittest.cc
[modify] https://crrev.com/180587c15a36861cdaaceec045619026e55fe8c5/net/quic/chromium/quic_connection_logger.cc
[modify] https://crrev.com/180587c15a36861cdaaceec045619026e55fe8c5/net/quic/chromium/quic_network_transaction_unittest.cc
[modify] https://crrev.com/180587c15a36861cdaaceec045619026e55fe8c5/net/socket/tcp_socket_posix.cc
[modify] https://crrev.com/180587c15a36861cdaaceec045619026e55fe8c5/net/socket/tcp_socket_posix.h
[modify] https://crrev.com/180587c15a36861cdaaceec045619026e55fe8c5/net/socket/tcp_socket_unittest.cc

Labels: Merge-Request-57
Project Member

Comment 3 by sheriffbot@chromium.org, Feb 21 2017

Labels: -Merge-Request-57 Hotlist-Merge-Approved Merge-Approved-57
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 4 by bugdroid1@chromium.org, Feb 21 2017

Labels: -merge-approved-57 merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a7161971f69be0c7f27b1756a4ccfbd9896a98cc

commit a7161971f69be0c7f27b1756a4ccfbd9896a98cc
Author: Tarun Bansal <tbansal@google.com>
Date: Tue Feb 21 17:42:48 2017

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}
(cherry picked from commit 180587c15a36861cdaaceec045619026e55fe8c5)

Review-Url: https://codereview.chromium.org/2705273002 .
Cr-Commit-Position: refs/branch-heads/2987@{#609}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/a7161971f69be0c7f27b1756a4ccfbd9896a98cc/net/net.gypi
[modify] https://crrev.com/a7161971f69be0c7f27b1756a4ccfbd9896a98cc/net/nqe/network_quality_estimator.cc
[modify] https://crrev.com/a7161971f69be0c7f27b1756a4ccfbd9896a98cc/net/nqe/network_quality_estimator_params.cc
[modify] https://crrev.com/a7161971f69be0c7f27b1756a4ccfbd9896a98cc/net/nqe/network_quality_estimator_params.h
[modify] https://crrev.com/a7161971f69be0c7f27b1756a4ccfbd9896a98cc/net/nqe/socket_watcher.cc
[modify] https://crrev.com/a7161971f69be0c7f27b1756a4ccfbd9896a98cc/net/nqe/socket_watcher.h
[modify] https://crrev.com/a7161971f69be0c7f27b1756a4ccfbd9896a98cc/net/nqe/socket_watcher_factory.cc
[modify] https://crrev.com/a7161971f69be0c7f27b1756a4ccfbd9896a98cc/net/nqe/socket_watcher_factory.h
[add] https://crrev.com/a7161971f69be0c7f27b1756a4ccfbd9896a98cc/net/nqe/socket_watcher_unittest.cc
[modify] https://crrev.com/a7161971f69be0c7f27b1756a4ccfbd9896a98cc/net/quic/chromium/quic_connection_logger.cc
[modify] https://crrev.com/a7161971f69be0c7f27b1756a4ccfbd9896a98cc/net/quic/chromium/quic_network_transaction_unittest.cc
[modify] https://crrev.com/a7161971f69be0c7f27b1756a4ccfbd9896a98cc/net/socket/tcp_socket_posix.cc
[modify] https://crrev.com/a7161971f69be0c7f27b1756a4ccfbd9896a98cc/net/socket/tcp_socket_posix.h
[modify] https://crrev.com/a7161971f69be0c7f27b1756a4ccfbd9896a98cc/net/socket/tcp_socket_unittest.cc

Labels: M-57
Status: Fixed (was: Started)

Sign in to add a comment