|
|
DescriptionNQE: Add Transport RTT based GetECT algorithm
In the next CL, GetECT API which computes Effective
Connection Type (ECT) based on transport RTT and downlink
throughput will be exposed to Cronet.
BUG=625305
Committed: https://crrev.com/b7046a46eea6d3938ad2fc136d650003260cd47f
Cr-Commit-Position: refs/heads/master@{#404497}
Patch Set 1 #
Total comments: 21
Patch Set 2 : Addressed kundaji comments #Patch Set 3 : kundaji comments #Patch Set 4 : Add TODO #
Total comments: 2
Patch Set 5 : Rebased #
Messages
Total messages: 31 (16 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Add Transport RTT based GetECT algorithm BUG= ========== to ========== Add Transport RTT based GetECT algorithm In the next CL, GetECT based on transport RTT algorithm will be exposed to Cronet. BUG=625305 ==========
Description was changed from ========== Add Transport RTT based GetECT algorithm In the next CL, GetECT based on transport RTT algorithm will be exposed to Cronet. BUG=625305 ========== to ========== Add Transport RTT based GetECT algorithm In the next CL, GetECT API which computes ECT based on transport RTT and downlink throughput will be exposed to Cronet. BUG=625305 ==========
Description was changed from ========== Add Transport RTT based GetECT algorithm In the next CL, GetECT API which computes ECT based on transport RTT and downlink throughput will be exposed to Cronet. BUG=625305 ========== to ========== NQE: Add Transport RTT based GetECT algorithm In the next CL, GetECT API which computes ECT based on transport RTT and downlink throughput will be exposed to Cronet. BUG=625305 ==========
tbansal@chromium.org changed reviewers: + kundaji@chromium.org
kundaji: ptal. Thanks.
https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:238: {"TransportRTTOrDownstreamThroughput", This is brittle because it allows adding an entry to enum without updating this hashmap. Also because copy pasting wrong values will also fail silently. Same issue if you manually maintain a switch statement that maintains mappings. Not a good idea to have a critical dependency that fails silently. Instead of manually initializing string-enum pairs, suggest using macros to autogenerate the boiler plate. Lots of examples in code base: https://cs.chromium.org/search/?q=%5C%23define.*LIST&sq=package:chromium&type=cs Macro doc: https://gcc.gnu.org/onlinedocs/cpp/Concatenation.html Specifically suggest something like: #define RTT_ALGO_LIST \ RTT_ALGO_ENUM(HTTP_AND_DOWNSTREAM),\ RTT_ALGO_ENUM(TRANSPORT_OR_DOWNSTREAM),\ ... Then you can autogenerate the boilerplate by defining RTT_ALGO_ENUM. .h file: #define RTT_ALGO_ENUM(name) RTT_##name##_THROUGHPUT, typedef enum { CLIENT_ENUMS_LIST } EffectiveConnectionTypeAlgorithm; #undef RTT_ALGO_ENUM .cc file: #define RTT_ALGO_ENUM(name) \ case name: return name; const EffectiveConnectionTypeAlgorithm GetAlgoForString(char* algo_name) { switch (client) { CLIENT_ENUMS_LIST } NOTREACHED(); return ""; } #undef RTT_ALGO_ENUM https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:995: // If the device is currently offline, then return FWIW code is very readable, so this comment seems unnecessary to me. https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1002: transport_rtt = nqe::internal::InvalidRTT(); Can we have GetRecentTransportRTTMedian(..) set transport_rtt to InvalidRTT on failure so that we can avoid this set, test, and reset? Actually, couldn't it simply return TimeDelta instead of a bool? https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1006: kbps = nqe::internal::kInvalidThroughput; Similar to above comment: Can GetRecentMedianDownlinkThroughputKbps(..) just return the kbps?
kundaji: ptal. Thanks. https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:238: {"TransportRTTOrDownstreamThroughput", On 2016/07/06 19:04:07, kundaji wrote: > This is brittle because it allows adding an entry to enum without updating this > hashmap. That error would be caught by DCHECK below. See line 278 below. "DCHECK_EQ(algorithm_name_to_enum_.size()..." > Also because copy pasting wrong values will also fail silently. That's the problem with using macros too...right? I am thinking of adding functionality to give a warning (or add UMA metric) if a variation param is not recognized, but that will take some time. > Same > issue if you manually maintain a switch statement that maintains mappings. Not a > good idea to have a critical dependency that fails silently. > > Instead of manually initializing string-enum pairs, suggest using macros to > autogenerate the boiler plate. Lots of examples in code base: > https://cs.chromium.org/search/?q=%5C%23define.*LIST&sq=package:chromium&type=cs > > Macro doc: https://gcc.gnu.org/onlinedocs/cpp/Concatenation.html > > Specifically suggest something like: > > #define RTT_ALGO_LIST \ > RTT_ALGO_ENUM(HTTP_AND_DOWNSTREAM),\ > RTT_ALGO_ENUM(TRANSPORT_OR_DOWNSTREAM),\ ... > > Then you can autogenerate the boilerplate by defining RTT_ALGO_ENUM. > > .h file: > #define RTT_ALGO_ENUM(name) RTT_##name##_THROUGHPUT, > typedef enum { > CLIENT_ENUMS_LIST > } EffectiveConnectionTypeAlgorithm; > #undef RTT_ALGO_ENUM > > > .cc file: > #define RTT_ALGO_ENUM(name) \ > case name: return name; > > const EffectiveConnectionTypeAlgorithm GetAlgoForString(char* algo_name) { > switch (client) { > CLIENT_ENUMS_LIST > } > NOTREACHED(); > return ""; > } > #undef RTT_ALGO_ENUM https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:995: // If the device is currently offline, then return On 2016/07/06 19:04:07, kundaji wrote: > FWIW code is very readable, so this comment seems unnecessary to me. Done. https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1002: transport_rtt = nqe::internal::InvalidRTT(); On 2016/07/06 19:04:07, kundaji wrote: > Can we have GetRecentTransportRTTMedian(..) set transport_rtt to InvalidRTT on > failure so that we can avoid this set, test, and reset? > > Actually, couldn't it simply return TimeDelta instead of a bool? Yeah, I need to fix that. It is easier now since GetRecentTransportRTTMedian is a private API. I would prefer to do that in a separate CL. https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1006: kbps = nqe::internal::kInvalidThroughput; On 2016/07/06 19:04:07, kundaji wrote: > Similar to above comment: Can GetRecentMedianDownlinkThroughputKbps(..) just > return the kbps? same as above.
lgtm https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:238: {"TransportRTTOrDownstreamThroughput", On 2016/07/06 23:17:32, tbansal1 wrote: > On 2016/07/06 19:04:07, kundaji wrote: > > This is brittle because it allows adding an entry to enum without updating > this > > hashmap. > That error would be caught by DCHECK below. See line 278 below. > "DCHECK_EQ(algorithm_name_to_enum_.size()..." > > Also because copy pasting wrong values will also fail silently. > That's the problem with using macros too...right? I am thinking of adding > functionality to give a warning (or add UMA metric) if a variation param is not > recognized, but that will take some time. > > Same > > issue if you manually maintain a switch statement that maintains mappings. Not > a > > good idea to have a critical dependency that fails silently. > > > > Instead of manually initializing string-enum pairs, suggest using macros to > > autogenerate the boiler plate. Lots of examples in code base: > > > https://cs.chromium.org/search/?q=%5C%23define.*LIST&sq=package:chromium&type=cs > > > > Macro doc: https://gcc.gnu.org/onlinedocs/cpp/Concatenation.html > > > > Specifically suggest something like: > > > > #define RTT_ALGO_LIST \ > > RTT_ALGO_ENUM(HTTP_AND_DOWNSTREAM),\ > > RTT_ALGO_ENUM(TRANSPORT_OR_DOWNSTREAM),\ ... > > > > Then you can autogenerate the boilerplate by defining RTT_ALGO_ENUM. > > > > .h file: > > #define RTT_ALGO_ENUM(name) RTT_##name##_THROUGHPUT, > > typedef enum { > > CLIENT_ENUMS_LIST > > } EffectiveConnectionTypeAlgorithm; > > #undef RTT_ALGO_ENUM > > > > > > .cc file: > > #define RTT_ALGO_ENUM(name) \ > > case name: return name; > > > > const EffectiveConnectionTypeAlgorithm GetAlgoForString(char* algo_name) { > > switch (client) { > > CLIENT_ENUMS_LIST > > } > > NOTREACHED(); > > return ""; > > } > > #undef RTT_ALGO_ENUM > Macro does not have this issue. You will get a compile time warning if 2 entries are the same. Issue of mismatching string-enum will never occur since you are only entering one; the other is auto-generated. > I am thinking of adding functionality to give a warning (or add UMA metric) if a variation param is not recognized, but that will take some time. Is there a good way to catch the error where I accidentally add a new entry, but forget to change the enum: { "SomeNewRTTAlgorithm", HTTP_RTT_AND_DOWNSTREAM_THROUGHOUT /*existing enum*/} FWIW, I have seen this exact pattern cause a copy paste bug that reached production. https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:399: variations_value = kMinimumRTTVariationParameterMsec - 1; This block is extremely similar to the block above and below. I see similar repition in RecordAccuracyAfterMainFrame below. Any way to eliminate the duplication? One option I could think of is for NetworkQuality to be loosely structured (for example HashMap with key value pairs) instead of having dedicated fields. It will make this loop more generic instead of having a block for each field. However, don't love this approach. Cannot think of a better way, but thought I'd mention it. https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:407: connection_thresholds_[i] = nqe::internal::NetworkQuality( Any reason to create new instance of NetworkQuality rather than setting field on the existing instance? https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1002: transport_rtt = nqe::internal::InvalidRTT(); On 2016/07/06 23:17:32, tbansal1 wrote: > On 2016/07/06 19:04:07, kundaji wrote: > > Can we have GetRecentTransportRTTMedian(..) set transport_rtt to InvalidRTT on > > failure so that we can avoid this set, test, and reset? > > > > Actually, couldn't it simply return TimeDelta instead of a bool? > > Yeah, I need to fix that. It is easier now since GetRecentTransportRTTMedian is > a private API. I would prefer to do that in a separate CL. Add TODO? https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1006: kbps = nqe::internal::kInvalidThroughput; On 2016/07/06 23:17:32, tbansal1 wrote: > On 2016/07/06 19:04:07, kundaji wrote: > > Similar to above comment: Can GetRecentMedianDownlinkThroughputKbps(..) just > > return the kbps? > > same as above. Acknowledged.
Patchset #3 (id:60001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:120001) has been deleted
tbansal@chromium.org changed reviewers: + bengr@chromium.org
bengr: ptal. Thanks. https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:238: {"TransportRTTOrDownstreamThroughput", On 2016/07/07 00:03:04, kundaji wrote: > On 2016/07/06 23:17:32, tbansal1 wrote: > > On 2016/07/06 19:04:07, kundaji wrote: > > > This is brittle because it allows adding an entry to enum without updating > > this > > > hashmap. > > That error would be caught by DCHECK below. See line 278 below. > > "DCHECK_EQ(algorithm_name_to_enum_.size()..." > > > Also because copy pasting wrong values will also fail silently. > > That's the problem with using macros too...right? I am thinking of adding > > functionality to give a warning (or add UMA metric) if a variation param is > not > > recognized, but that will take some time. > > > Same > > > issue if you manually maintain a switch statement that maintains mappings. > Not > > a > > > good idea to have a critical dependency that fails silently. > > > > > > Instead of manually initializing string-enum pairs, suggest using macros to > > > autogenerate the boiler plate. Lots of examples in code base: > > > > > > https://cs.chromium.org/search/?q=%5C%23define.*LIST&sq=package:chromium&type=cs > > > > > > Macro doc: https://gcc.gnu.org/onlinedocs/cpp/Concatenation.html > > > > > > Specifically suggest something like: > > > > > > #define RTT_ALGO_LIST \ > > > RTT_ALGO_ENUM(HTTP_AND_DOWNSTREAM),\ > > > RTT_ALGO_ENUM(TRANSPORT_OR_DOWNSTREAM),\ ... > > > > > > Then you can autogenerate the boilerplate by defining RTT_ALGO_ENUM. > > > > > > .h file: > > > #define RTT_ALGO_ENUM(name) RTT_##name##_THROUGHPUT, > > > typedef enum { > > > CLIENT_ENUMS_LIST > > > } EffectiveConnectionTypeAlgorithm; > > > #undef RTT_ALGO_ENUM > > > > > > > > > .cc file: > > > #define RTT_ALGO_ENUM(name) \ > > > case name: return name; > > > > > > const EffectiveConnectionTypeAlgorithm GetAlgoForString(char* algo_name) { > > > switch (client) { > > > CLIENT_ENUMS_LIST > > > } > > > NOTREACHED(); > > > return ""; > > > } > > > #undef RTT_ALGO_ENUM > > > > Macro does not have this issue. You will get a compile time warning if 2 entries > are the same. Issue of mismatching string-enum will never occur since you are > only entering one; the other is auto-generated. Agreed. Right now this error is captured using NQE tests. Another problem with using macro is that I need to map strings to enums (not enums to strings). Since switch-case does not support strings, it is not clear if I can use that framework. > > > I am thinking of adding functionality to give a warning (or add UMA metric) if > a variation param is not recognized, but that will take some time. > Is there a good way to catch the error where I accidentally add a new entry, but > forget to change the enum: > { "SomeNewRTTAlgorithm", HTTP_RTT_AND_DOWNSTREAM_THROUGHOUT /*existing enum*/} I added some more test code in NetworkQualityEstimatorTest.ObtainAlgorithmToUseFromParams which tests this. > > FWIW, I have seen this exact pattern cause a copy paste bug that reached > production. https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:399: variations_value = kMinimumRTTVariationParameterMsec - 1; On 2016/07/07 00:03:04, kundaji wrote: > This block is extremely similar to the block above and below. I see similar > repition in RecordAccuracyAfterMainFrame below. Any way to eliminate the > duplication? > > One option I could think of is for NetworkQuality to be loosely structured (for > example HashMap with key value pairs) instead of having dedicated fields. It > will make this loop more generic instead of having a block for each field. > However, don't love this approach. > > Cannot think of a better way, but thought I'd mention it. I am not sure of a better way to combine them. https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:407: connection_thresholds_[i] = nqe::internal::NetworkQuality( On 2016/07/07 00:03:04, kundaji wrote: > Any reason to create new instance of NetworkQuality rather than setting field on > the existing instance? Done. https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1002: transport_rtt = nqe::internal::InvalidRTT(); On 2016/07/07 00:03:04, kundaji wrote: > On 2016/07/06 23:17:32, tbansal1 wrote: > > On 2016/07/06 19:04:07, kundaji wrote: > > > Can we have GetRecentTransportRTTMedian(..) set transport_rtt to InvalidRTT > on > > > failure so that we can avoid this set, test, and reset? > > > > > > Actually, couldn't it simply return TimeDelta instead of a bool? > > > > Yeah, I need to fix that. It is easier now since GetRecentTransportRTTMedian > is > > a private API. I would prefer to do that in a separate CL. > > Add TODO? Done in .h file.
https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:238: {"TransportRTTOrDownstreamThroughput", On 2016/07/07 16:47:32, tbansal1 wrote: > On 2016/07/07 00:03:04, kundaji wrote: > > On 2016/07/06 23:17:32, tbansal1 wrote: > > > On 2016/07/06 19:04:07, kundaji wrote: > > > > This is brittle because it allows adding an entry to enum without updating > > > this > > > > hashmap. > > > That error would be caught by DCHECK below. See line 278 below. > > > "DCHECK_EQ(algorithm_name_to_enum_.size()..." > > > > Also because copy pasting wrong values will also fail silently. > > > That's the problem with using macros too...right? I am thinking of adding > > > functionality to give a warning (or add UMA metric) if a variation param is > > not > > > recognized, but that will take some time. > > > > Same > > > > issue if you manually maintain a switch statement that maintains mappings. > > Not > > > a > > > > good idea to have a critical dependency that fails silently. > > > > > > > > Instead of manually initializing string-enum pairs, suggest using macros > to > > > > autogenerate the boiler plate. Lots of examples in code base: > > > > > > > > > > https://cs.chromium.org/search/?q=%5C%23define.*LIST&sq=package:chromium&type=cs > > > > > > > > Macro doc: https://gcc.gnu.org/onlinedocs/cpp/Concatenation.html > > > > > > > > Specifically suggest something like: > > > > > > > > #define RTT_ALGO_LIST \ > > > > RTT_ALGO_ENUM(HTTP_AND_DOWNSTREAM),\ > > > > RTT_ALGO_ENUM(TRANSPORT_OR_DOWNSTREAM),\ ... > > > > > > > > Then you can autogenerate the boilerplate by defining RTT_ALGO_ENUM. > > > > > > > > .h file: > > > > #define RTT_ALGO_ENUM(name) RTT_##name##_THROUGHPUT, > > > > typedef enum { > > > > CLIENT_ENUMS_LIST > > > > } EffectiveConnectionTypeAlgorithm; > > > > #undef RTT_ALGO_ENUM > > > > > > > > > > > > .cc file: > > > > #define RTT_ALGO_ENUM(name) \ > > > > case name: return name; > > > > > > > > const EffectiveConnectionTypeAlgorithm GetAlgoForString(char* algo_name) { > > > > switch (client) { > > > > CLIENT_ENUMS_LIST > > > > } > > > > NOTREACHED(); > > > > return ""; > > > > } > > > > #undef RTT_ALGO_ENUM > > > > > > > Macro does not have this issue. You will get a compile time warning if 2 > entries > > are the same. Issue of mismatching string-enum will never occur since you are > > only entering one; the other is auto-generated. > Agreed. Right now this error is captured using NQE tests. Another problem with > using macro is that I need to map strings to enums (not enums to strings). Since > switch-case does not support strings, it is not clear if I can use that > framework. > > > > > I am thinking of adding functionality to give a warning (or add UMA metric) > if > > a variation param is not recognized, but that will take some time. > > Is there a good way to catch the error where I accidentally add a new entry, > but > > forget to change the enum: > > { "SomeNewRTTAlgorithm", HTTP_RTT_AND_DOWNSTREAM_THROUGHOUT /*existing enum*/} > I added some more test code in > NetworkQualityEstimatorTest.ObtainAlgorithmToUseFromParams > which tests this. > > > > FWIW, I have seen this exact pattern cause a copy paste bug that reached > > production. > It is easy to map strings to enums. You just use an if statement instead of the case statement: #define RTT_ALGO_ENUM(name) \ if (name == algo_name) return RTT_##name##_THROUGHPUT;
https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:238: {"TransportRTTOrDownstreamThroughput", On 2016/07/07 17:19:17, kundaji wrote: > On 2016/07/07 16:47:32, tbansal1 wrote: > > On 2016/07/07 00:03:04, kundaji wrote: > > > On 2016/07/06 23:17:32, tbansal1 wrote: > > > > On 2016/07/06 19:04:07, kundaji wrote: > > > > > This is brittle because it allows adding an entry to enum without > updating > > > > this > > > > > hashmap. > > > > That error would be caught by DCHECK below. See line 278 below. > > > > "DCHECK_EQ(algorithm_name_to_enum_.size()..." > > > > > Also because copy pasting wrong values will also fail silently. > > > > That's the problem with using macros too...right? I am thinking of adding > > > > functionality to give a warning (or add UMA metric) if a variation param > is > > > not > > > > recognized, but that will take some time. > > > > > Same > > > > > issue if you manually maintain a switch statement that maintains > mappings. > > > Not > > > > a > > > > > good idea to have a critical dependency that fails silently. > > > > > > > > > > Instead of manually initializing string-enum pairs, suggest using macros > > to > > > > > autogenerate the boiler plate. Lots of examples in code base: > > > > > > > > > > > > > > > https://cs.chromium.org/search/?q=%5C%23define.*LIST&sq=package:chromium&type=cs > > > > > > > > > > Macro doc: https://gcc.gnu.org/onlinedocs/cpp/Concatenation.html > > > > > > > > > > Specifically suggest something like: > > > > > > > > > > #define RTT_ALGO_LIST \ > > > > > RTT_ALGO_ENUM(HTTP_AND_DOWNSTREAM),\ > > > > > RTT_ALGO_ENUM(TRANSPORT_OR_DOWNSTREAM),\ ... > > > > > > > > > > Then you can autogenerate the boilerplate by defining RTT_ALGO_ENUM. > > > > > > > > > > .h file: > > > > > #define RTT_ALGO_ENUM(name) RTT_##name##_THROUGHPUT, > > > > > typedef enum { > > > > > CLIENT_ENUMS_LIST > > > > > } EffectiveConnectionTypeAlgorithm; > > > > > #undef RTT_ALGO_ENUM > > > > > > > > > > > > > > > .cc file: > > > > > #define RTT_ALGO_ENUM(name) \ > > > > > case name: return name; > > > > > > > > > > const EffectiveConnectionTypeAlgorithm GetAlgoForString(char* algo_name) > { > > > > > switch (client) { > > > > > CLIENT_ENUMS_LIST > > > > > } > > > > > NOTREACHED(); > > > > > return ""; > > > > > } > > > > > #undef RTT_ALGO_ENUM > > > > > > > > > > Macro does not have this issue. You will get a compile time warning if 2 > > entries > > > are the same. Issue of mismatching string-enum will never occur since you > are > > > only entering one; the other is auto-generated. > > Agreed. Right now this error is captured using NQE tests. Another problem with > > using macro is that I need to map strings to enums (not enums to strings). > Since > > switch-case does not support strings, it is not clear if I can use that > > framework. > > > > > > > I am thinking of adding functionality to give a warning (or add UMA > metric) > > if > > > a variation param is not recognized, but that will take some time. > > > Is there a good way to catch the error where I accidentally add a new entry, > > but > > > forget to change the enum: > > > { "SomeNewRTTAlgorithm", HTTP_RTT_AND_DOWNSTREAM_THROUGHOUT /*existing > enum*/} > > I added some more test code in > > NetworkQualityEstimatorTest.ObtainAlgorithmToUseFromParams > > which tests this. > > > > > > FWIW, I have seen this exact pattern cause a copy paste bug that reached > > > production. > > > > It is easy to map strings to enums. You just use an if statement instead of the > case statement: > #define RTT_ALGO_ENUM(name) \ > if (name == algo_name) return RTT_##name##_THROUGHPUT; > > "if" condtional does not catch duplicates in value (the switch does). See example here: https://codereview.chromium.org/2129723004/
https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:238: {"TransportRTTOrDownstreamThroughput", On 2016/07/07 18:01:32, tbansal1 wrote: > On 2016/07/07 17:19:17, kundaji wrote: > > On 2016/07/07 16:47:32, tbansal1 wrote: > > > On 2016/07/07 00:03:04, kundaji wrote: > > > > On 2016/07/06 23:17:32, tbansal1 wrote: > > > > > On 2016/07/06 19:04:07, kundaji wrote: > > > > > > This is brittle because it allows adding an entry to enum without > > updating > > > > > this > > > > > > hashmap. > > > > > That error would be caught by DCHECK below. See line 278 below. > > > > > "DCHECK_EQ(algorithm_name_to_enum_.size()..." > > > > > > Also because copy pasting wrong values will also fail silently. > > > > > That's the problem with using macros too...right? I am thinking of > adding > > > > > functionality to give a warning (or add UMA metric) if a variation param > > is > > > > not > > > > > recognized, but that will take some time. > > > > > > Same > > > > > > issue if you manually maintain a switch statement that maintains > > mappings. > > > > Not > > > > > a > > > > > > good idea to have a critical dependency that fails silently. > > > > > > > > > > > > Instead of manually initializing string-enum pairs, suggest using > macros > > > to > > > > > > autogenerate the boiler plate. Lots of examples in code base: > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/search/?q=%5C%23define.*LIST&sq=package:chromium&type=cs > > > > > > > > > > > > Macro doc: https://gcc.gnu.org/onlinedocs/cpp/Concatenation.html > > > > > > > > > > > > Specifically suggest something like: > > > > > > > > > > > > #define RTT_ALGO_LIST \ > > > > > > RTT_ALGO_ENUM(HTTP_AND_DOWNSTREAM),\ > > > > > > RTT_ALGO_ENUM(TRANSPORT_OR_DOWNSTREAM),\ ... > > > > > > > > > > > > Then you can autogenerate the boilerplate by defining RTT_ALGO_ENUM. > > > > > > > > > > > > .h file: > > > > > > #define RTT_ALGO_ENUM(name) RTT_##name##_THROUGHPUT, > > > > > > typedef enum { > > > > > > CLIENT_ENUMS_LIST > > > > > > } EffectiveConnectionTypeAlgorithm; > > > > > > #undef RTT_ALGO_ENUM > > > > > > > > > > > > > > > > > > .cc file: > > > > > > #define RTT_ALGO_ENUM(name) \ > > > > > > case name: return name; > > > > > > > > > > > > const EffectiveConnectionTypeAlgorithm GetAlgoForString(char* > algo_name) > > { > > > > > > switch (client) { > > > > > > CLIENT_ENUMS_LIST > > > > > > } > > > > > > NOTREACHED(); > > > > > > return ""; > > > > > > } > > > > > > #undef RTT_ALGO_ENUM > > > > > > > > > > > > > Macro does not have this issue. You will get a compile time warning if 2 > > > entries > > > > are the same. Issue of mismatching string-enum will never occur since you > > are > > > > only entering one; the other is auto-generated. > > > Agreed. Right now this error is captured using NQE tests. Another problem > with > > > using macro is that I need to map strings to enums (not enums to strings). > > Since > > > switch-case does not support strings, it is not clear if I can use that > > > framework. > > > > > > > > > I am thinking of adding functionality to give a warning (or add UMA > > metric) > > > if > > > > a variation param is not recognized, but that will take some time. > > > > Is there a good way to catch the error where I accidentally add a new > entry, > > > but > > > > forget to change the enum: > > > > { "SomeNewRTTAlgorithm", HTTP_RTT_AND_DOWNSTREAM_THROUGHOUT /*existing > > enum*/} > > > I added some more test code in > > > NetworkQualityEstimatorTest.ObtainAlgorithmToUseFromParams > > > which tests this. > > > > > > > > FWIW, I have seen this exact pattern cause a copy paste bug that reached > > > > production. > > > > > > > It is easy to map strings to enums. You just use an if statement instead of > the > > case statement: > > #define RTT_ALGO_ENUM(name) \ > > if (name == algo_name) return RTT_##name##_THROUGHPUT; > > > > > > "if" condtional does not catch duplicates in value (the switch does). See > example here: https://codereview.chromium.org/2129723004/ We don't rely on the switch to catch duplicates. The enum definition will catch duplicates. The if statement is autogenerated from the enum definition, so will automatically not have duplicates.
bengr: ptal. Thanks. https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2113363002/diff/20001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:238: {"TransportRTTOrDownstreamThroughput", On 2016/07/07 19:10:49, kundaji wrote: > On 2016/07/07 18:01:32, tbansal1 wrote: > > On 2016/07/07 17:19:17, kundaji wrote: > > > On 2016/07/07 16:47:32, tbansal1 wrote: > > > > On 2016/07/07 00:03:04, kundaji wrote: > > > > > On 2016/07/06 23:17:32, tbansal1 wrote: > > > > > > On 2016/07/06 19:04:07, kundaji wrote: > > > > > > > This is brittle because it allows adding an entry to enum without > > > updating > > > > > > this > > > > > > > hashmap. > > > > > > That error would be caught by DCHECK below. See line 278 below. > > > > > > "DCHECK_EQ(algorithm_name_to_enum_.size()..." > > > > > > > Also because copy pasting wrong values will also fail silently. > > > > > > That's the problem with using macros too...right? I am thinking of > > adding > > > > > > functionality to give a warning (or add UMA metric) if a variation > param > > > is > > > > > not > > > > > > recognized, but that will take some time. > > > > > > > Same > > > > > > > issue if you manually maintain a switch statement that maintains > > > mappings. > > > > > Not > > > > > > a > > > > > > > good idea to have a critical dependency that fails silently. > > > > > > > > > > > > > > Instead of manually initializing string-enum pairs, suggest using > > macros > > > > to > > > > > > > autogenerate the boiler plate. Lots of examples in code base: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/search/?q=%5C%23define.*LIST&sq=package:chromium&type=cs > > > > > > > > > > > > > > Macro doc: https://gcc.gnu.org/onlinedocs/cpp/Concatenation.html > > > > > > > > > > > > > > Specifically suggest something like: > > > > > > > > > > > > > > #define RTT_ALGO_LIST \ > > > > > > > RTT_ALGO_ENUM(HTTP_AND_DOWNSTREAM),\ > > > > > > > RTT_ALGO_ENUM(TRANSPORT_OR_DOWNSTREAM),\ ... > > > > > > > > > > > > > > Then you can autogenerate the boilerplate by defining RTT_ALGO_ENUM. > > > > > > > > > > > > > > .h file: > > > > > > > #define RTT_ALGO_ENUM(name) RTT_##name##_THROUGHPUT, > > > > > > > typedef enum { > > > > > > > CLIENT_ENUMS_LIST > > > > > > > } EffectiveConnectionTypeAlgorithm; > > > > > > > #undef RTT_ALGO_ENUM > > > > > > > > > > > > > > > > > > > > > .cc file: > > > > > > > #define RTT_ALGO_ENUM(name) \ > > > > > > > case name: return name; > > > > > > > > > > > > > > const EffectiveConnectionTypeAlgorithm GetAlgoForString(char* > > algo_name) > > > { > > > > > > > switch (client) { > > > > > > > CLIENT_ENUMS_LIST > > > > > > > } > > > > > > > NOTREACHED(); > > > > > > > return ""; > > > > > > > } > > > > > > > #undef RTT_ALGO_ENUM > > > > > > > > > > > > > > > > Macro does not have this issue. You will get a compile time warning if 2 > > > > entries > > > > > are the same. Issue of mismatching string-enum will never occur since > you > > > are > > > > > only entering one; the other is auto-generated. > > > > Agreed. Right now this error is captured using NQE tests. Another problem > > with > > > > using macro is that I need to map strings to enums (not enums to strings). > > > Since > > > > switch-case does not support strings, it is not clear if I can use that > > > > framework. > > > > > > > > > > > I am thinking of adding functionality to give a warning (or add UMA > > > metric) > > > > if > > > > > a variation param is not recognized, but that will take some time. > > > > > Is there a good way to catch the error where I accidentally add a new > > entry, > > > > but > > > > > forget to change the enum: > > > > > { "SomeNewRTTAlgorithm", HTTP_RTT_AND_DOWNSTREAM_THROUGHOUT /*existing > > > enum*/} > > > > I added some more test code in > > > > NetworkQualityEstimatorTest.ObtainAlgorithmToUseFromParams > > > > which tests this. > > > > > > > > > > FWIW, I have seen this exact pattern cause a copy paste bug that reached > > > > > production. > > > > > > > > > > It is easy to map strings to enums. You just use an if statement instead of > > the > > > case statement: > > > #define RTT_ALGO_ENUM(name) \ > > > if (name == algo_name) return RTT_##name##_THROUGHPUT; > > > > > > > > > > "if" condtional does not catch duplicates in value (the switch does). See > > example here: https://codereview.chromium.org/2129723004/ > > We don't rely on the switch to catch duplicates. The enum definition will catch > duplicates. The if statement is autogenerated from the enum definition, so will > automatically not have duplicates. Added TODO. For now the duplicates are catched using tests.
lgtm. Please update the description to spell out "effective connection type" so that readers know what you mean by GetECT. One nit. https://codereview.chromium.org/2113363002/diff/160001/net/nqe/network_qualit... File net/nqe/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/2113363002/diff/160001/net/nqe/network_qualit... net/nqe/network_quality_estimator_unittest.cc:662: for (Algorithms::const_iterator it_second = Can't you initialize it_second to one after it_first? Then you wouldn't need the check that it_first != it_second, and you'd do less work.
Description was changed from ========== NQE: Add Transport RTT based GetECT algorithm In the next CL, GetECT API which computes ECT based on transport RTT and downlink throughput will be exposed to Cronet. BUG=625305 ========== to ========== NQE: Add Transport RTT based GetECT algorithm In the next CL, GetECT API which computes Effective Connection Type (ECT) based on transport RTT and downlink throughput will be exposed to Cronet. BUG=625305 ==========
Patchset #6 (id:200001) has been deleted
https://codereview.chromium.org/2113363002/diff/160001/net/nqe/network_qualit... File net/nqe/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/2113363002/diff/160001/net/nqe/network_qualit... net/nqe/network_quality_estimator_unittest.cc:662: for (Algorithms::const_iterator it_second = On 2016/07/08 17:10:50, bengr wrote: > Can't you initialize it_second to one after it_first? Then you wouldn't need the > check that it_first != it_second, and you'd do less work. I think only the random access iterators (e.g., a vector iterator) implement addition. In general, iterators are only required to implement increment operator (++). And, I can't use increment operator here because that will modify the first iterator too.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kundaji@chromium.org, bengr@chromium.org Link to the patchset: https://codereview.chromium.org/2113363002/#ps180001 (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 ========== NQE: Add Transport RTT based GetECT algorithm In the next CL, GetECT API which computes Effective Connection Type (ECT) based on transport RTT and downlink throughput will be exposed to Cronet. BUG=625305 ========== to ========== NQE: Add Transport RTT based GetECT algorithm In the next CL, GetECT API which computes Effective Connection Type (ECT) based on transport RTT and downlink throughput will be exposed to Cronet. BUG=625305 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:180001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== NQE: Add Transport RTT based GetECT algorithm In the next CL, GetECT API which computes Effective Connection Type (ECT) based on transport RTT and downlink throughput will be exposed to Cronet. BUG=625305 ========== to ========== NQE: Add Transport RTT based GetECT algorithm In the next CL, GetECT API which computes Effective Connection Type (ECT) based on transport RTT and downlink throughput will be exposed to Cronet. BUG=625305 Committed: https://crrev.com/b7046a46eea6d3938ad2fc136d650003260cd47f Cr-Commit-Position: refs/heads/master@{#404497} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b7046a46eea6d3938ad2fc136d650003260cd47f Cr-Commit-Position: refs/heads/master@{#404497} |