Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2266663002:
NQE: Change GetEffectiveConnectionType to return last ECT (Closed)
DescriptionNQE: Change GetEffectiveConnectionType to return last ECT
This change decouples the GetECT() API exposed by NQE, and the
logic that dictates when NQE recomputes the ECT. It also makes
GetECT() API light-weight so it can be called more frequently
by consumers (e.g., once per request) without worrying about
the computation overhead.
BUG=639869
Committed: https://crrev.com/dc48857294abdcc3222dad7464c2f50b36b9b4ca
Cr-Commit-Position: refs/heads/master@{#417133}
Patch Set 1 : PS #
Total comments: 2
Patch Set 2 : Addressed Ryan's comments #
Total comments: 10
Patch Set 3 : Rebased #Patch Set 4 : Addressed bengr comments #
Messages
Total messages: 29 (19 generated)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
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: Exceeded global retry quota
Description was changed from ========== NQE: Change GetEffectiveConnectionType to return last ECT BUG= ========== to ========== NQE: Change GetEffectiveConnectionType to return last ECT This change decouples the GetECT() API exposed by NQE, and the logic that dictates when NQE recomputes the ECT. It also makes GetECT() API light-weight so it can be called more frequently by consumers (e.g., once per request) without worrying about the computation overhead. BUG= ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== NQE: Change GetEffectiveConnectionType to return last ECT This change decouples the GetECT() API exposed by NQE, and the logic that dictates when NQE recomputes the ECT. It also makes GetECT() API light-weight so it can be called more frequently by consumers (e.g., once per request) without worrying about the computation overhead. BUG= ========== to ========== NQE: Change GetEffectiveConnectionType to return last ECT This change decouples the GetECT() API exposed by NQE, and the logic that dictates when NQE recomputes the ECT. It also makes GetECT() API light-weight so it can be called more frequently by consumers (e.g., once per request) without worrying about the computation overhead. BUG=639869 ==========
tbansal@chromium.org changed reviewers: + ryansturm@chromium.org
ryansturm: ptal. thanks!
This is putting more trust in effective_connection_type_recomputation_interval_ than there previously was. I'm worried 15 seconds is too long for a service that might want to force a recompute more often than that. You know the use cases, so consider if 15 seconds is too long for some of the cronet consumers (where there may not be MAIN_FRAME requests and they might also not hit the doubling of the observations in 15 seconds). If you share any of that concern, you can either have a shorter polling time interval or a RecomputeAndGetEffectiveConnectionType method exposed. Otherwise, this CL lgtm % 1 nit. https://codereview.chromium.org/2266663002/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2266663002/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:291: // Forces recomputtion of effective connection type, and notifies observers if s/recomputtion/recomputation
On 2016/08/22 18:01:52, RyanSturm wrote: > This is putting more trust in effective_connection_type_recomputation_interval_ > than there previously was. I'm worried 15 seconds is too long for a service that > might want to force a recompute more often than that. You know the use cases, so > consider if 15 seconds is too long for some of the cronet consumers (where there > may not be MAIN_FRAME requests and they might also not hit the doubling of the > observations in 15 seconds). If you share any of that concern, you can either > have a shorter polling time interval or a RecomputeAndGetEffectiveConnectionType > method exposed. I have changed the constants to a lower value. Hopefully, once the signal strength code is plumbed in, I can trigger the recomputation based on that too. > > Otherwise, this CL lgtm % 1 nit. > > https://codereview.chromium.org/2266663002/diff/60001/net/nqe/network_quality... > File net/nqe/network_quality_estimator.h (right): > > https://codereview.chromium.org/2266663002/diff/60001/net/nqe/network_quality... > net/nqe/network_quality_estimator.h:291: // Forces recomputtion of effective > connection type, and notifies observers if > s/recomputtion/recomputation
tbansal@chromium.org changed reviewers: + bengr@chromium.org
bengr: ptal. thanks. https://codereview.chromium.org/2266663002/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2266663002/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:291: // Forces recomputtion of effective connection type, and notifies observers if On 2016/08/22 18:01:52, RyanSturm wrote: > s/recomputtion/recomputation Done.
https://codereview.chromium.org/2266663002/diff/80001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2266663002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1649: // available now are 50% more in count than the number of samples that remove "in count" https://codereview.chromium.org/2266663002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1651: rtt_observations_size_at_last_ect_computation_ * 1.5 >= Does this mean that there's an exponential backoff on recomputation? If so, I don't think that's a great idea, since we'll want to be responsive to sudden changes. https://codereview.chromium.org/2266663002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1658: RecomputeEffectiveConnectionType(); Can you just call this "ComputeEffectiveConnectionType()?" https://codereview.chromium.org/2266663002/diff/80001/net/nqe/network_quality... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2266663002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:152: // effective connection type is computed by network quality estimator at by -> by the https://codereview.chromium.org/2266663002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:293: void RecomputeEffectiveConnectionType(); Why is this not private?
bengr: ptal. Thanks. https://codereview.chromium.org/2266663002/diff/80001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2266663002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1649: // available now are 50% more in count than the number of samples that On 2016/08/25 22:51:24, bengr wrote: > remove "in count" Done. https://codereview.chromium.org/2266663002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1651: rtt_observations_size_at_last_ect_computation_ * 1.5 >= On 2016/08/25 22:51:24, bengr wrote: > Does this mean that there's an exponential backoff on recomputation? If so, I > don't think that's a great idea, since we'll want to be responsive to sudden > changes. There is exponential backoff. Additionally, ECT is recomputed every 10 seconds (provided there is network traffic), and on every main frame request. And, also (in future) we may recompute if there is a significant change in the signal strength. The goal here is that NQE should detect "sudden changes" and respond to them instead of callers detecting these changes, and forcing NQE to recompute ECT. https://codereview.chromium.org/2266663002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1658: RecomputeEffectiveConnectionType(); On 2016/08/25 22:51:24, bengr wrote: > Can you just call this "ComputeEffectiveConnectionType()?" Done. https://codereview.chromium.org/2266663002/diff/80001/net/nqe/network_quality... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2266663002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:152: // effective connection type is computed by network quality estimator at On 2016/08/25 22:51:24, bengr wrote: > by -> by the Done. https://codereview.chromium.org/2266663002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:293: void RecomputeEffectiveConnectionType(); On 2016/08/25 22:51:24, bengr wrote: > Why is this not private? Done.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
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: This issue passed the CQ dry run.
lgtm
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 Link to the patchset: https://codereview.chromium.org/2266663002/#ps120001 (title: "Addressed bengr comments")
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 ========== NQE: Change GetEffectiveConnectionType to return last ECT This change decouples the GetECT() API exposed by NQE, and the logic that dictates when NQE recomputes the ECT. It also makes GetECT() API light-weight so it can be called more frequently by consumers (e.g., once per request) without worrying about the computation overhead. BUG=639869 ========== to ========== NQE: Change GetEffectiveConnectionType to return last ECT This change decouples the GetECT() API exposed by NQE, and the logic that dictates when NQE recomputes the ECT. It also makes GetECT() API light-weight so it can be called more frequently by consumers (e.g., once per request) without worrying about the computation overhead. BUG=639869 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== NQE: Change GetEffectiveConnectionType to return last ECT This change decouples the GetECT() API exposed by NQE, and the logic that dictates when NQE recomputes the ECT. It also makes GetECT() API light-weight so it can be called more frequently by consumers (e.g., once per request) without worrying about the computation overhead. BUG=639869 ========== to ========== NQE: Change GetEffectiveConnectionType to return last ECT This change decouples the GetECT() API exposed by NQE, and the logic that dictates when NQE recomputes the ECT. It also makes GetECT() API light-weight so it can be called more frequently by consumers (e.g., once per request) without worrying about the computation overhead. BUG=639869 Committed: https://crrev.com/dc48857294abdcc3222dad7464c2f50b36b9b4ca Cr-Commit-Position: refs/heads/master@{#417133} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/dc48857294abdcc3222dad7464c2f50b36b9b4ca Cr-Commit-Position: refs/heads/master@{#417133} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
