Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2221103003:
Compute effective connection type dynamically (Closed)
DescriptionCompute effective connection type dynamically
This CL changes the behavior of how Network Quality Estimator
(NQE) computes Effective Connection Type (ECT). Currently,
ECT is computed regularly at 15 second intervals.
This CL changes that behavior to additionally recompute the
ECT under two more conditions:
(1) Recompute the effective connection type (ECT) if the
number of RTT or bandwidth samples have more than doubled
since the last computation.
(2) Recompute the effective connection type (ECT) if the
previously computed ECT value was Unknown.
BUG=635678
Committed: https://crrev.com/dbd91c073a0b04aebd67b890a7c3cea13ac1da4d
Cr-Commit-Position: refs/heads/master@{#411803}
Patch Set 1 : PS #
Total comments: 2
Patch Set 2 : Addressed comments #
Total comments: 13
Patch Set 3 : bengr comments #Patch Set 4 : Rebased #
Messages
Total messages: 24 (13 generated)
Description was changed from ========== Adaptively compute effective connection type. BUG= ========== to ========== Compute effective connection type dynamically This CL changes the behavior of how Network Quality Estimator (NQE) computes Effective Connection Type (ECT). Currently, ECT is computed regularly at 15 second intervals. This CL changes that behavior to additionally recompute the ECT under two more conditions: (1) Recompute the effective connection type (ECT) if the number of RTT or bandwidth samples have more than doubled since the last computation. (2) Recompute the effective connection type (ECT) if the previously computed ECT value was Unknown. BUG=635678 ==========
Description was changed from ========== Compute effective connection type dynamically This CL changes the behavior of how Network Quality Estimator (NQE) computes Effective Connection Type (ECT). Currently, ECT is computed regularly at 15 second intervals. This CL changes that behavior to additionally recompute the ECT under two more conditions: (1) Recompute the effective connection type (ECT) if the number of RTT or bandwidth samples have more than doubled since the last computation. (2) Recompute the effective connection type (ECT) if the previously computed ECT value was Unknown. BUG=635678 ========== to ========== Compute effective connection type dynamically This CL changes the behavior of how Network Quality Estimator (NQE) computes Effective Connection Type (ECT). Currently, ECT is computed regularly at 15 second intervals. This CL changes that behavior to additionally recompute the ECT under two more conditions: (1) Recompute the effective connection type (ECT) if the number of RTT or bandwidth samples have more than doubled since the last computation. (2) Recompute the effective connection type (ECT) if the previously computed ECT value was Unknown. BUG=635678 ==========
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
tbansal@chromium.org changed reviewers: + ryansturm@chromium.org
ryansturm: ptal. Thanks.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm % variable names nit https://codereview.chromium.org/2221103003/diff/20001/net/nqe/network_quality... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2221103003/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:601: size_t number_of_bandwidth_samples_at_last_ect_computation_; nit: The naming of this variable is mouthful and has nothing in common with downstream_throughput_kbps_observations_, which makes it confusing. I think that the number_of_ part is unnecessary. Maybe, throughput_observations_size_at_last_ect_computation_? If you want to refactor the observations buffer name to bandwidth, that also works. If you do decide to change this, make the rtt consistent with that change.
https://codereview.chromium.org/2221103003/diff/20001/net/nqe/network_quality... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2221103003/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:601: size_t number_of_bandwidth_samples_at_last_ect_computation_; On 2016/08/08 23:57:01, RyanSturm wrote: > nit: The naming of this variable is mouthful and has nothing in common with > downstream_throughput_kbps_observations_, which makes it confusing. I think that > the number_of_ part is unnecessary. > > Maybe, throughput_observations_size_at_last_ect_computation_? If you want to > refactor the observations buffer name to bandwidth, that also works. > > If you do decide to change this, make the rtt consistent with that change. Done.
tbansal@chromium.org changed reviewers: + bengr@chromium.org
bengr: ptal. Thanks.
https://codereview.chromium.org/2221103003/diff/40001/net/nqe/network_quality... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2221103003/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:591: // Minimum duration between two consecutive computations of effective Shouldn't we also recompute if RSSI changes? https://codereview.chromium.org/2221103003/diff/40001/net/nqe/network_quality... File net/nqe/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/2221103003/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_unittest.cc:1566: // Even though there are 10 RTT samples already available, addition of one addition -> the addition https://codereview.chromium.org/2221103003/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_unittest.cc:1567: // more RTT sample should trigger recomputation of effective connection type of -> of the https://codereview.chromium.org/2221103003/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_unittest.cc:1622: // observations are more than doubled, effective connection type must be are -> is https://codereview.chromium.org/2221103003/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_unittest.cc:1623: // recomputed, and notified to observers. recomputed, -> recomputed https://codereview.chromium.org/2221103003/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_unittest.cc:1634: // Increase the number of RTT observations to more than twice the number Do you also test that not increasing the number does not trigger recomputation?
bengr: ptal. Thanks. https://codereview.chromium.org/2221103003/diff/40001/net/nqe/network_quality... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2221103003/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:591: // Minimum duration between two consecutive computations of effective On 2016/08/09 21:29:06, bengr wrote: > Shouldn't we also recompute if RSSI changes? In future CL. https://codereview.chromium.org/2221103003/diff/40001/net/nqe/network_quality... File net/nqe/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/2221103003/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_unittest.cc:1566: // Even though there are 10 RTT samples already available, addition of one On 2016/08/09 21:29:06, bengr wrote: > addition -> the addition Done. https://codereview.chromium.org/2221103003/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_unittest.cc:1567: // more RTT sample should trigger recomputation of effective connection type On 2016/08/09 21:29:06, bengr wrote: > of -> of the Done. https://codereview.chromium.org/2221103003/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_unittest.cc:1622: // observations are more than doubled, effective connection type must be On 2016/08/09 21:29:06, bengr wrote: > are -> is Done. https://codereview.chromium.org/2221103003/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_unittest.cc:1623: // recomputed, and notified to observers. On 2016/08/09 21:29:06, bengr wrote: > recomputed, -> recomputed Done. https://codereview.chromium.org/2221103003/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_unittest.cc:1634: // Increase the number of RTT observations to more than twice the number On 2016/08/09 21:29:06, bengr wrote: > Do you also test that not increasing the number does not trigger recomputation? I am not sure if I understand this comment. I am counting the observations and exactly matching that with the expected count. That should be sufficient. There could be some extraneous triggerings under some weird unknown circumstances (due to some bugs), but without knowing those circumstances, it is difficult to test that.
lgtm https://codereview.chromium.org/2221103003/diff/40001/net/nqe/network_quality... File net/nqe/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/2221103003/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_unittest.cc:1634: // Increase the number of RTT observations to more than twice the number On 2016/08/10 00:13:12, tbansal1 wrote: > On 2016/08/09 21:29:06, bengr wrote: > > Do you also test that not increasing the number does not trigger > recomputation? > > I am not sure if I understand this comment. I am counting the observations and > exactly matching that with the expected count. That should be sufficient. > > There could be some extraneous triggerings under some weird unknown > circumstances (due to some bugs), but without knowing those circumstances, it is > difficult to test that. Never mind. I see you covered the case I was worried about.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ryansturm@chromium.org, bengr@chromium.org Link to the patchset: https://codereview.chromium.org/2221103003/#ps80001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Compute effective connection type dynamically This CL changes the behavior of how Network Quality Estimator (NQE) computes Effective Connection Type (ECT). Currently, ECT is computed regularly at 15 second intervals. This CL changes that behavior to additionally recompute the ECT under two more conditions: (1) Recompute the effective connection type (ECT) if the number of RTT or bandwidth samples have more than doubled since the last computation. (2) Recompute the effective connection type (ECT) if the previously computed ECT value was Unknown. BUG=635678 ========== to ========== Compute effective connection type dynamically This CL changes the behavior of how Network Quality Estimator (NQE) computes Effective Connection Type (ECT). Currently, ECT is computed regularly at 15 second intervals. This CL changes that behavior to additionally recompute the ECT under two more conditions: (1) Recompute the effective connection type (ECT) if the number of RTT or bandwidth samples have more than doubled since the last computation. (2) Recompute the effective connection type (ECT) if the previously computed ECT value was Unknown. BUG=635678 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Compute effective connection type dynamically This CL changes the behavior of how Network Quality Estimator (NQE) computes Effective Connection Type (ECT). Currently, ECT is computed regularly at 15 second intervals. This CL changes that behavior to additionally recompute the ECT under two more conditions: (1) Recompute the effective connection type (ECT) if the number of RTT or bandwidth samples have more than doubled since the last computation. (2) Recompute the effective connection type (ECT) if the previously computed ECT value was Unknown. BUG=635678 ========== to ========== Compute effective connection type dynamically This CL changes the behavior of how Network Quality Estimator (NQE) computes Effective Connection Type (ECT). Currently, ECT is computed regularly at 15 second intervals. This CL changes that behavior to additionally recompute the ECT under two more conditions: (1) Recompute the effective connection type (ECT) if the number of RTT or bandwidth samples have more than doubled since the last computation. (2) Recompute the effective connection type (ECT) if the previously computed ECT value was Unknown. BUG=635678 Committed: https://crrev.com/dbd91c073a0b04aebd67b890a7c3cea13ac1da4d Cr-Commit-Position: refs/heads/master@{#411803} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/dbd91c073a0b04aebd67b890a7c3cea13ac1da4d Cr-Commit-Position: refs/heads/master@{#411803} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
