Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2064193002:
Use NQE's GetECT() API for data reduction proxy (Closed)
DescriptionUse NQE's GetECT() API for data reduction proxy
BUG=615551
Committed: https://crrev.com/810e048bfe701353a90d5a496b64c8f3701b4d8e
Cr-Commit-Position: refs/heads/master@{#400842}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Rebased #Patch Set 3 : bengr comments #Patch Set 4 : default params #Patch Set 5 : Rebased #
Messages
Total messages: 24 (11 generated)
Description was changed from ========== Use NQE'sECT API for data reduction proxy BUG= ========== to ========== Use NQE's GetECT() API for data reduction proxy BUG=615551 ==========
tbansal@chromium.org changed reviewers: + kundaji@chromium.org
kundaji: PTAL. Thanks.
https://codereview.chromium.org/2064193002/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2064193002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:471: IsEffectiveConnectionTypeSlowerThanThreshold(effective_connection_type); Do you think it makes sense to move method IsEffectiveConnectionTypeSlowerThanThreshold to network_quality_estimator? Since it already knows effective_connection_type, you would only have to send it the threshold. https://codereview.chromium.org/2064193002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:788: if (recent_effective_connection_type == It seems odd that accuracy is not being recorded when recent effective connection type is not known. Whereas the prediction itself uses GetEffectiveConnectionType(). Are you measuring hypothetical accuracy of a new method?
https://codereview.chromium.org/2064193002/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2064193002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:471: IsEffectiveConnectionTypeSlowerThanThreshold(effective_connection_type); On 2016/06/14 19:05:05, kundaji wrote: > Do you think it makes sense to move method > IsEffectiveConnectionTypeSlowerThanThreshold to network_quality_estimator? Since > it already knows effective_connection_type, you would only have to send it the > threshold. The effective connection type can also be recent effective connection type, which NQE does not know because the value of recent ECT depends on the time duration. https://codereview.chromium.org/2064193002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:788: if (recent_effective_connection_type == On 2016/06/14 19:05:05, kundaji wrote: > It seems odd that accuracy is not being recorded when recent effective > connection type is not known. Whereas the prediction itself uses > GetEffectiveConnectionType(). Are you measuring hypothetical accuracy of a new > method? If the recent effective connection type is unknown, then we do not know what is the ground truth. That makes it difficult to record accuracy.
lgtm https://codereview.chromium.org/2064193002/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/2064193002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:471: IsEffectiveConnectionTypeSlowerThanThreshold(effective_connection_type); On 2016/06/14 20:35:48, tbansal1 wrote: > On 2016/06/14 19:05:05, kundaji wrote: > > Do you think it makes sense to move method > > IsEffectiveConnectionTypeSlowerThanThreshold to network_quality_estimator? > Since > > it already knows effective_connection_type, you would only have to send it the > > threshold. > > The effective connection type can also be recent effective connection type, > which NQE does not know because the value of recent ECT depends on the time > duration. Yes, does not make sense if you are going to have recent effective connection type. Was based on assumption that you would get rid of it. https://codereview.chromium.org/2064193002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:788: if (recent_effective_connection_type == On 2016/06/14 20:35:48, tbansal1 wrote: > On 2016/06/14 19:05:05, kundaji wrote: > > It seems odd that accuracy is not being recorded when recent effective > > connection type is not known. Whereas the prediction itself uses > > GetEffectiveConnectionType(). Are you measuring hypothetical accuracy of a new > > method? > > If the recent effective connection type is unknown, then we do not know what is > the ground truth. That makes it difficult to record accuracy. Acknowledged.
tbansal@chromium.org changed reviewers: + bengr@chromium.org
bengr: ptal. Thanks.
lgtm. nits. https://codereview.chromium.org/2064193002/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/2064193002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:291: // Returns true if the network quality is at least as slow as the one The use of the word "slow" doesn't seem quite right. Can network quality be "slow?" Or should it be "poor?" You can leave it as is, but know it bothers me. ;) https://codereview.chromium.org/2064193002/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc (right): https://codereview.chromium.org/2064193002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:913: variation_params["effective_connection_type"] = "Slow2G"; Here also, I thought the name was "2G Poor." Why the name change?
https://codereview.chromium.org/2064193002/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc (right): https://codereview.chromium.org/2064193002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:913: variation_params["effective_connection_type"] = "Slow2G"; On 2016/06/20 15:25:03, bengr wrote: > Here also, I thought the name was "2G Poor." Why the name change? Nope, the name was Slow2G https://cs.chromium.org/chromium/src/net/nqe/network_quality_estimator.h?rcl=... I can change it, but it will be better to do in a separate CL.
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org, kundaji@chromium.org Link to the patchset: https://codereview.chromium.org/2064193002/#ps80001 (title: "default params")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2064193002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org, kundaji@chromium.org Link to the patchset: https://codereview.chromium.org/2064193002/#ps100001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2064193002/100001
Message was sent while issue was closed.
Description was changed from ========== Use NQE's GetECT() API for data reduction proxy BUG=615551 ========== to ========== Use NQE's GetECT() API for data reduction proxy BUG=615551 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Use NQE's GetECT() API for data reduction proxy BUG=615551 ========== to ========== Use NQE's GetECT() API for data reduction proxy BUG=615551 Committed: https://crrev.com/810e048bfe701353a90d5a496b64c8f3701b4d8e Cr-Commit-Position: refs/heads/master@{#400842} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/810e048bfe701353a90d5a496b64c8f3701b4d8e Cr-Commit-Position: refs/heads/master@{#400842} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
