New issue
Advanced search Search tips

Issue 786074 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Change socket watcher throttler to be global across all sockets

Project Member Reported by tbansal@chromium.org, Nov 16 2017

Issue description

Currently, in network quality estimator, a socket watcher notifies NQE of a new RTT observation if its last notification was more than a second ago. This was done in order to reduce the overhead of polling the kernet to get the RTT estimates.

However, some network embedders may create very few sockets (say only 1-2). In those cases, the number of RTT observations received may be very few. To ensure that there are enough RTT observations (regardless of number of sockets created), we should instead throttle the notifications across all socket watchers. e.g., a socket watcher should be able to push a RTT notification if no socket watcher (across all socket watchers) has pushed a notification in last 200 msec.
 

Comment 1 by bengr@chromium.org, Nov 20 2017

So what's the solution approach? Is it to change the logic to report whenever 200ms goes by? Will that create a lot more load when there are a lot of sockets?
Right now, each socket can push an observation once per second. With very few sockets open (say 1), this results in few RTT observations.

The proposed solution is to let sockets push another observation if no socket (across all sockets) has pushed an observation in last 200 ms.

If there are many sockets open (> = 5), then this proposal would not create additional load since the 200 msec check would likely fail. However, if there are few sockets (< 5), then it would let the few sockets push additional data. This may result in the load increase.
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 20 2017

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

commit 58a0f880f57b5ad1011ac1260c492da9f9d952d6
Author: Tarun Bansal <tbansal@chromium.org>
Date: Mon Nov 20 18:48:17 2017

Use a global throttler for Socket Watcher in the Network Quality Estimator (NQE)

Bug:  786074 
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I63330a3ababd79f58fb19faa3531411d179122a4
Reviewed-on: https://chromium-review.googlesource.com/768525
Reviewed-by: Ben Greenstein <bengr@chromium.org>
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517873}
[modify] https://crrev.com/58a0f880f57b5ad1011ac1260c492da9f9d952d6/net/nqe/network_quality_estimator.cc
[modify] https://crrev.com/58a0f880f57b5ad1011ac1260c492da9f9d952d6/net/nqe/network_quality_estimator.h
[modify] https://crrev.com/58a0f880f57b5ad1011ac1260c492da9f9d952d6/net/nqe/network_quality_estimator_params.cc
[modify] https://crrev.com/58a0f880f57b5ad1011ac1260c492da9f9d952d6/net/nqe/network_quality_estimator_params.h
[modify] https://crrev.com/58a0f880f57b5ad1011ac1260c492da9f9d952d6/net/nqe/network_quality_estimator_unittest.cc
[modify] https://crrev.com/58a0f880f57b5ad1011ac1260c492da9f9d952d6/net/nqe/socket_watcher.cc
[modify] https://crrev.com/58a0f880f57b5ad1011ac1260c492da9f9d952d6/net/nqe/socket_watcher.h
[modify] https://crrev.com/58a0f880f57b5ad1011ac1260c492da9f9d952d6/net/nqe/socket_watcher_factory.cc
[modify] https://crrev.com/58a0f880f57b5ad1011ac1260c492da9f9d952d6/net/nqe/socket_watcher_factory.h
[modify] https://crrev.com/58a0f880f57b5ad1011ac1260c492da9f9d952d6/net/nqe/socket_watcher_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment