Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2032443003:
NQE: Allow algorithm to be set using variation params (Closed)
DescriptionNQE: Allow algorithm to be set using variation params
This CL provides the ability to use different algorithms for
computing effective connection type. The algorithm to use
can be set using field trial params. If field trial params
are unavailable (e.g., at first run), then a default value
is used.
The existing algorithm was also modified to be exactly same
as what data reduction proxy is currently is using.
Once this CL lands, the server side CL needs to land to set
the variation param. Then, GetEffectiveConnectionType() API
will be ready for use by DRP and custom fonts.
BUG=615551, 569497
Committed: https://crrev.com/a1d1c6a86c81451dabc92c3450348b329ff1290a
Cr-Commit-Position: refs/heads/master@{#398753}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : bengr comments #
Total comments: 2
Patch Set 3 : Rebased #Patch Set 4 : bengr comments #Patch Set 5 : Rebased #
Total comments: 8
Patch Set 6 : Moved the map initialization to constructor #Patch Set 7 : Addressed naming comments #Patch Set 8 : Minor fix in DCHECK evaluation #
Messages
Total messages: 28 (16 generated)
Description was changed from ========== Allow algorithm to be set using variation params BUG= ========== to ========== NQE: Allow algorithm to be set using variation params This CL provides the ability to use different algorithms for computing effective connection type. The algorithm to use can be set using field trial params. If field trial params are unavailable (e.g., at first run), then a default value is used. The existing algorithm was also modified to be exactly same as what data reduction proxy is currently is using. Once this CL lands, the server side CL needs to land to set the variation param. Then, GetEffectiveConnectionType() API will be ready for use by DRP and custom fonts. BUG= ==========
Description was changed from ========== NQE: Allow algorithm to be set using variation params This CL provides the ability to use different algorithms for computing effective connection type. The algorithm to use can be set using field trial params. If field trial params are unavailable (e.g., at first run), then a default value is used. The existing algorithm was also modified to be exactly same as what data reduction proxy is currently is using. Once this CL lands, the server side CL needs to land to set the variation param. Then, GetEffectiveConnectionType() API will be ready for use by DRP and custom fonts. BUG= ========== to ========== NQE: Allow algorithm to be set using variation params This CL provides the ability to use different algorithms for computing effective connection type. The algorithm to use can be set using field trial params. If field trial params are unavailable (e.g., at first run), then a default value is used. The existing algorithm was also modified to be exactly same as what data reduction proxy is currently is using. Once this CL lands, the server side CL needs to land to set the variation param. Then, GetEffectiveConnectionType() API will be ready for use by DRP and custom fonts. BUG= 615551,569497 ==========
Patchset #1 (id:1) has been deleted
tbansal@chromium.org changed reviewers: + bengr@chromium.org
bengr: ptal. Thanks.
https://codereview.chromium.org/2032443003/diff/20001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2032443003/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:270: algorithm_(GetValueForVariationParamWithDefaultValue(variation_params, You should add a comment somewhere that you default to algorithm 0 if you do not have a valid variations param. https://codereview.chromium.org/2032443003/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:293: DCHECK_GE(algorithm_, 0); Shouldn't this be DCHECK_EQ? algorithm_ should be greater than or equal to zero, but no greater than the maximum supported algorithm number. You might also want to introduce a constant that contains the maximum supported algorithm number. https://codereview.chromium.org/2032443003/diff/20001/net/nqe/network_quality... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2032443003/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:485: const int32_t algorithm_; Letters are more intuitive than numbers for experiments, at least to me, probably because of the existence of A/B testing.
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
bengr: ptal. Thanks. https://codereview.chromium.org/2032443003/diff/20001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2032443003/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:270: algorithm_(GetValueForVariationParamWithDefaultValue(variation_params, On 2016/06/02 22:59:43, bengr wrote: > You should add a comment somewhere that you default to algorithm 0 if you do not > have a valid variations param. Done. https://codereview.chromium.org/2032443003/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:293: DCHECK_GE(algorithm_, 0); On 2016/06/02 22:59:43, bengr wrote: > Shouldn't this be DCHECK_EQ? algorithm_ should be greater than or equal to zero, > but no greater than the maximum supported algorithm number. You might also want > to introduce a constant that contains the maximum supported algorithm number. I removed it since with strings it does not really work. Also, I added NOTREACHED() in GetRecentEffectiveConnectionType, so this is covered there. https://codereview.chromium.org/2032443003/diff/20001/net/nqe/network_quality... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2032443003/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:485: const int32_t algorithm_; On 2016/06/02 22:59:43, bengr wrote: > Letters are more intuitive than numbers for experiments, at least to me, > probably because of the existence of A/B testing. Done.
https://codereview.chromium.org/2032443003/diff/120001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2032443003/diff/120001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:38: // The algorithm that uses HTTP RTT and downstream throughput to determine the Hmm. This will be hard to extend. I think instead you should do something like this: enum class Algorithm { UNKNOWN = 0, HTTP_RTT_AND_DOWNSTREAM_THROUGHOUT = 1 }; // If needed for UMA: std::unordered_map<Algorithm, std::string> algorithm_name( { { HTTP_RTT_AND_DOWNSTREAM_THROUGHPUT, "HttpRTTAndDownstreamThroughput" } }); EffectiveConnectionType GetRecentEffectiveConnectionType( EffectiveConnectionTypeAlgorithm algorithm) { EffectiveConnectionType ect = UNKNOWN; // ? switch (algorithm) { case HTTP_RTT_AND_DOWNSTREAM_THROUGHOUT: // fall-through: default: RunHttpRttAndDownstreamThroughputAlgorithm(&ect); break: } return ect; }
Patchset #4 (id:160001) has been deleted
Patchset #4 (id:180001) has been deleted
Patchset #4 (id:200001) has been deleted
bengr: ptal. Thanks. https://codereview.chromium.org/2032443003/diff/120001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2032443003/diff/120001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:38: // The algorithm that uses HTTP RTT and downstream throughput to determine the On 2016/06/06 20:14:54, bengr wrote: > Hmm. This will be hard to extend. I think instead you should do something like > this: > > enum class Algorithm { > UNKNOWN = 0, > HTTP_RTT_AND_DOWNSTREAM_THROUGHOUT = 1 > }; > > // If needed for UMA: > std::unordered_map<Algorithm, std::string> algorithm_name( > { { HTTP_RTT_AND_DOWNSTREAM_THROUGHPUT, > "HttpRTTAndDownstreamThroughput" } }); > > EffectiveConnectionType > GetRecentEffectiveConnectionType( > EffectiveConnectionTypeAlgorithm algorithm) { > EffectiveConnectionType ect = UNKNOWN; // ? > switch (algorithm) { > case HTTP_RTT_AND_DOWNSTREAM_THROUGHOUT: > // fall-through: > default: > RunHttpRttAndDownstreamThroughputAlgorithm(&ect); > break: > } > return ect; > } I wanted to keep |algorithm_| as a const variable. So, I did something similar.
Patchset #5 (id:240001) has been deleted
ping.
https://codereview.chromium.org/2032443003/diff/260001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2032443003/diff/260001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:238: DCHECK(stringToAlgorithm_.end() == stringToAlgorithm_.find(std::string())); Construction of the map should go write before this. https://codereview.chromium.org/2032443003/diff/260001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2032443003/diff/260001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:359: const std::map<std::string, EffectiveConnectionTypeAlgorithm> This probably shouldn't be in the .h. https://codereview.chromium.org/2032443003/diff/260001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:360: stringToAlgorithm_ = {{"HttpRTTAndDownstreamThroughput", Why does this need to be a member? Also how about algorithm_name_to_enum_? https://codereview.chromium.org/2032443003/diff/260001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:509: const EffectiveConnectionTypeAlgorithm algorithm_; To be clear this should probably be called effective_connection_type_algorithm_.
lgtm modulo the naming suggestions.
https://codereview.chromium.org/2032443003/diff/260001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2032443003/diff/260001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:238: DCHECK(stringToAlgorithm_.end() == stringToAlgorithm_.find(std::string())); On 2016/06/08 23:33:11, bengr wrote: > Construction of the map should go write before this. Not doing this as discussed offline. https://codereview.chromium.org/2032443003/diff/260001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2032443003/diff/260001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:359: const std::map<std::string, EffectiveConnectionTypeAlgorithm> On 2016/06/08 23:33:11, bengr wrote: > This probably shouldn't be in the .h. Done. https://codereview.chromium.org/2032443003/diff/260001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:360: stringToAlgorithm_ = {{"HttpRTTAndDownstreamThroughput", On 2016/06/08 23:33:11, bengr wrote: > Why does this need to be a member? Also how about algorithm_name_to_enum_? Done. https://codereview.chromium.org/2032443003/diff/260001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:509: const EffectiveConnectionTypeAlgorithm algorithm_; On 2016/06/08 23:33:11, bengr wrote: > To be clear this should probably be called effective_connection_type_algorithm_. Done.
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 Link to the patchset: https://codereview.chromium.org/2032443003/#ps320001 (title: "Minor fix in DCHECK evaluation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2032443003/320001
Message was sent while issue was closed.
Description was changed from ========== NQE: Allow algorithm to be set using variation params This CL provides the ability to use different algorithms for computing effective connection type. The algorithm to use can be set using field trial params. If field trial params are unavailable (e.g., at first run), then a default value is used. The existing algorithm was also modified to be exactly same as what data reduction proxy is currently is using. Once this CL lands, the server side CL needs to land to set the variation param. Then, GetEffectiveConnectionType() API will be ready for use by DRP and custom fonts. BUG= 615551,569497 ========== to ========== NQE: Allow algorithm to be set using variation params This CL provides the ability to use different algorithms for computing effective connection type. The algorithm to use can be set using field trial params. If field trial params are unavailable (e.g., at first run), then a default value is used. The existing algorithm was also modified to be exactly same as what data reduction proxy is currently is using. Once this CL lands, the server side CL needs to land to set the variation param. Then, GetEffectiveConnectionType() API will be ready for use by DRP and custom fonts. BUG= 615551,569497 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== NQE: Allow algorithm to be set using variation params This CL provides the ability to use different algorithms for computing effective connection type. The algorithm to use can be set using field trial params. If field trial params are unavailable (e.g., at first run), then a default value is used. The existing algorithm was also modified to be exactly same as what data reduction proxy is currently is using. Once this CL lands, the server side CL needs to land to set the variation param. Then, GetEffectiveConnectionType() API will be ready for use by DRP and custom fonts. BUG= 615551,569497 ========== to ========== NQE: Allow algorithm to be set using variation params This CL provides the ability to use different algorithms for computing effective connection type. The algorithm to use can be set using field trial params. If field trial params are unavailable (e.g., at first run), then a default value is used. The existing algorithm was also modified to be exactly same as what data reduction proxy is currently is using. Once this CL lands, the server side CL needs to land to set the variation param. Then, GetEffectiveConnectionType() API will be ready for use by DRP and custom fonts. BUG= 615551,569497 Committed: https://crrev.com/a1d1c6a86c81451dabc92c3450348b329ff1290a Cr-Commit-Position: refs/heads/master@{#398753} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/a1d1c6a86c81451dabc92c3450348b329ff1290a Cr-Commit-Position: refs/heads/master@{#398753} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
