Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2411833003:
NQE: Add more statistics to ObservationBuffer class (Closed)
DescriptionNQE: Add more statistics to ObservationBuffer class
Add APIs to compute weighted average and unweighted average
to the ObservationBuffer class.
These statistics would be used to compute the network
quality, and then compare their accuracy with the weighted
median statistic (which is currently in use).
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet
BUG=649887
Committed: https://crrev.com/d53a4ba712f3941e9c7f9f70dbc75fd2136ff14e
Cr-Commit-Position: refs/heads/master@{#426095}
Patch Set 1 : ps #
Total comments: 2
Patch Set 2 : Addressed kundaji's comments #
Total comments: 30
Patch Set 3 : Addressed bengr comments #
Total comments: 8
Patch Set 4 : Addressed bengr comments #Messages
Total messages: 73 (63 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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Description was changed from ========== w w w w w BUG= ========== to ========== Add more statistics BUG= ==========
Description was changed from ========== Add more statistics BUG= ========== to ========== NQE: Add more statistics BUG= ==========
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
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 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...
Description was changed from ========== NQE: Add more statistics BUG= ========== to ========== NQE: Add more statistics to ObservationBuffer class Add APIs to compute weighted average and unweighted average to the ObservationBuffer class. These would be used to compute the network quality, and then compare their accuracy with the weighted median (which is currently in use). BUG=649887 ==========
Patchset #1 (id:20001) has been deleted
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 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 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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
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 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...
Patchset #5 (id:120001) has been deleted
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #1 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 checked by tbansal@chromium.org to run a CQ dry run
Patchset #2 (id:160001) has been deleted
Patchset #1 (id:140001) has been deleted
Description was changed from ========== NQE: Add more statistics to ObservationBuffer class Add APIs to compute weighted average and unweighted average to the ObservationBuffer class. These would be used to compute the network quality, and then compare their accuracy with the weighted median (which is currently in use). BUG=649887 ========== to ========== NQE: Add more statistics to ObservationBuffer class Add APIs to compute weighted average and unweighted average to the ObservationBuffer class. These would be used to compute the network quality, and then compare their accuracy with the weighted median statistic (which is currently in use). BUG=649887 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== NQE: Add more statistics to ObservationBuffer class Add APIs to compute weighted average and unweighted average to the ObservationBuffer class. These would be used to compute the network quality, and then compare their accuracy with the weighted median statistic (which is currently in use). BUG=649887 ========== to ========== NQE: Add more statistics to ObservationBuffer class Add APIs to compute weighted average and unweighted average to the ObservationBuffer class. These statistics would be used to compute the network quality, and then compare their accuracy with the weighted median statistic (which is currently in use). BUG=649887 ==========
tbansal@chromium.org changed reviewers: + kundaji@chromium.org
kunaji: ptal. thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
lgtm Minor nit. https://codereview.chromium.org/2411833003/diff/180001/net/nqe/observation_bu... File net/nqe/observation_buffer.h (right): https://codereview.chromium.org/2411833003/diff/180001/net/nqe/observation_bu... net/nqe/observation_buffer.h:125: // Returns true iff the weighted average of the observations in this Can you clarify why weighted average might not be available. I am guessing that: // Returns false if all of the |observations_| are in |disallowed_observation_sources|. Same for below.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
tbansal@chromium.org changed reviewers: + bengr@chromium.org
bengr: ptal. Thanks. https://codereview.chromium.org/2411833003/diff/180001/net/nqe/observation_bu... File net/nqe/observation_buffer.h (right): https://codereview.chromium.org/2411833003/diff/180001/net/nqe/observation_bu... net/nqe/observation_buffer.h:125: // Returns true iff the weighted average of the observations in this On 2016/10/12 22:09:52, kundaji wrote: > Can you clarify why weighted average might not be available. I am guessing that: > // Returns false if all of the |observations_| are in > |disallowed_observation_sources|. > > Same for below. Done.
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.
Description was changed from ========== NQE: Add more statistics to ObservationBuffer class Add APIs to compute weighted average and unweighted average to the ObservationBuffer class. These statistics would be used to compute the network quality, and then compare their accuracy with the weighted median statistic (which is currently in use). BUG=649887 ========== to ========== NQE: Add more statistics to ObservationBuffer class Add APIs to compute weighted average and unweighted average to the ObservationBuffer class. These statistics would be used to compute the network quality, and then compare their accuracy with the weighted median statistic (which is currently in use). CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=649887 ==========
https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... File net/nqe/observation_buffer.h (right): https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:127: // among all observations since |begin_timestamp|. If the value is among -> of? since -> made on or after https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:131: bool GetWeightedAverage(const base::TimeTicks begin_timestamp, Did you mean for this parameter to be a const reference? https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:134: ValueType* result) const { Why is this not a base::TimeDelta? https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:135: DCHECK(result); No need for this DCHECK. https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:138: // Stores WeightedObservation in increasing order of value. I would say: //Stores weighted observations in increasing order by value. https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:149: DCHECK_LT(0.0, total_weight); Why can't a weight be 0.0? https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:157: // weight, divided by the sum of weights of all observations. weight -> weights of weights -> of the weights https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:173: // is unavailable if all the values in observation buffer are older than all the values in -> all values in the https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:175: bool GetUnweightedAverage(const base::TimeTicks begin_timestamp, This method seems very similar to the previous one. Is it possible to extract the common parts into an anonymous function that they share? https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:179: DCHECK(result); Remove this DCHECK. https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:182: // Stores WeightedObservation in increasing order of value. See previous comment on wording. https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:198: // Unweighted average is the sum of all observations divided by the number Unweighted -> The unweighted https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:217: // Convert different ValueTypes to double to make it possible to perform It would be cleaner to not have to use any of these converters. I'm not sure why they're necessary. https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... File net/nqe/observation_buffer_unittest.cc (right): https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer_unittest.cc:248: // TODO(tbansal): Enable these tests on Windows once these functions are called File a bug for this and reference it here. https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer_unittest.cc:268: // First 50 samples have very old timestamp. Turn these all into complete sentences. E.g.: // The first 50 samples have very old timestamps.
Patchset #3 (id:220001) has been deleted
Patchset #3 (id:240001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
bengr: ptal. thanks. https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... File net/nqe/observation_buffer.h (right): https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:127: // among all observations since |begin_timestamp|. If the value is On 2016/10/14 21:15:58, bengr wrote: > among -> of? > since -> made on or after Done. https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:131: bool GetWeightedAverage(const base::TimeTicks begin_timestamp, On 2016/10/14 21:15:58, bengr wrote: > Did you mean for this parameter to be a const reference? Done. https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:134: ValueType* result) const { On 2016/10/14 21:15:58, bengr wrote: > Why is this not a base::TimeDelta? It is a template class. Caller may set it to base::TimeDelta (for RTT) or int32 (for throughput). https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:135: DCHECK(result); On 2016/10/14 21:15:58, bengr wrote: > No need for this DCHECK. Done. https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:138: // Stores WeightedObservation in increasing order of value. On 2016/10/14 21:15:58, bengr wrote: > I would say: > //Stores weighted observations in increasing order by value. Done. https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:149: DCHECK_LT(0.0, total_weight); On 2016/10/14 21:15:58, bengr wrote: > Why can't a weight be 0.0? Because there is a divide by |total_weight| later on line 165, and that may cause divide-by-zero. This DCHECK is more for documentation. It is not expected to be zero because |(weighted_observations| has at least one entry. https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:157: // weight, divided by the sum of weights of all observations. On 2016/10/14 21:15:58, bengr wrote: > weight -> weights > of weights -> of the weights Done. https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:173: // is unavailable if all the values in observation buffer are older than On 2016/10/14 21:15:58, bengr wrote: > all the values in -> all values in the Done. https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:175: bool GetUnweightedAverage(const base::TimeTicks begin_timestamp, On 2016/10/14 21:15:58, bengr wrote: > This method seems very similar to the previous one. Is it possible to extract > the common parts into an anonymous function that they share? I have moved some of the common code to ComputeWeightedObservations(). https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:179: DCHECK(result); On 2016/10/14 21:15:58, bengr wrote: > Remove this DCHECK. Done. https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:182: // Stores WeightedObservation in increasing order of value. On 2016/10/14 21:15:58, bengr wrote: > See previous comment on wording. Done. https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:198: // Unweighted average is the sum of all observations divided by the number On 2016/10/14 21:15:58, bengr wrote: > Unweighted -> The unweighted Done. https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer.h:217: // Convert different ValueTypes to double to make it possible to perform On 2016/10/14 21:15:58, bengr wrote: > It would be cleaner to not have to use any of these converters. I'm not sure why > they're necessary. They are necessary for doing arithmetic operations on base::TimeDelta. Without these, when computing the weighted average, the code will need to multiple ValueType (which is base::TimeDelta) with double. That operation is not supported. So, we have to temporarily convert base::TimeDelta to double. https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... File net/nqe/observation_buffer_unittest.cc (right): https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer_unittest.cc:248: // TODO(tbansal): Enable these tests on Windows once these functions are called On 2016/10/14 21:15:59, bengr wrote: > File a bug for this and reference it here. Done. https://codereview.chromium.org/2411833003/diff/200001/net/nqe/observation_bu... net/nqe/observation_buffer_unittest.cc:268: // First 50 samples have very old timestamp. On 2016/10/14 21:15:59, bengr wrote: > Turn these all into complete sentences. E.g.: > > // The first 50 samples have very old timestamps. Done.
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 with nits. https://codereview.chromium.org/2411833003/diff/260001/net/nqe/observation_bu... File net/nqe/observation_buffer_unittest.cc (right): https://codereview.chromium.org/2411833003/diff/260001/net/nqe/observation_bu... net/nqe/observation_buffer_unittest.cc:268: // The first 50 samples have very old timestamp. timestamp -> timestamps. https://codereview.chromium.org/2411833003/diff/260001/net/nqe/observation_bu... net/nqe/observation_buffer_unittest.cc:277: // The next 50 (i.e., from 51 to 100) samples have recent timestamp. timestamp -> timestamps https://codereview.chromium.org/2411833003/diff/260001/net/nqe/observation_bu... net/nqe/observation_buffer_unittest.cc:322: // The first 50 samples have very old timestamp. timestamp -> timestamps https://codereview.chromium.org/2411833003/diff/260001/net/nqe/observation_bu... net/nqe/observation_buffer_unittest.cc:331: // The next 50 (i.e., from 51 to 100) samples have recent timestamp. timestamp -> timestamps
https://codereview.chromium.org/2411833003/diff/260001/net/nqe/observation_bu... File net/nqe/observation_buffer_unittest.cc (right): https://codereview.chromium.org/2411833003/diff/260001/net/nqe/observation_bu... net/nqe/observation_buffer_unittest.cc:268: // The first 50 samples have very old timestamp. On 2016/10/18 21:32:35, bengr wrote: > timestamp -> timestamps. Done. https://codereview.chromium.org/2411833003/diff/260001/net/nqe/observation_bu... net/nqe/observation_buffer_unittest.cc:277: // The next 50 (i.e., from 51 to 100) samples have recent timestamp. On 2016/10/18 21:32:35, bengr wrote: > timestamp -> timestamps Done. https://codereview.chromium.org/2411833003/diff/260001/net/nqe/observation_bu... net/nqe/observation_buffer_unittest.cc:322: // The first 50 samples have very old timestamp. On 2016/10/18 21:32:35, bengr wrote: > timestamp -> timestamps Done. https://codereview.chromium.org/2411833003/diff/260001/net/nqe/observation_bu... net/nqe/observation_buffer_unittest.cc:331: // The next 50 (i.e., from 51 to 100) samples have recent timestamp. On 2016/10/18 21:32:35, bengr wrote: > timestamp -> timestamps Done.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
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/2411833003/#ps280001 (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: Add more statistics to ObservationBuffer class Add APIs to compute weighted average and unweighted average to the ObservationBuffer class. These statistics would be used to compute the network quality, and then compare their accuracy with the weighted median statistic (which is currently in use). CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=649887 ========== to ========== NQE: Add more statistics to ObservationBuffer class Add APIs to compute weighted average and unweighted average to the ObservationBuffer class. These statistics would be used to compute the network quality, and then compare their accuracy with the weighted median statistic (which is currently in use). CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=649887 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== NQE: Add more statistics to ObservationBuffer class Add APIs to compute weighted average and unweighted average to the ObservationBuffer class. These statistics would be used to compute the network quality, and then compare their accuracy with the weighted median statistic (which is currently in use). CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=649887 ========== to ========== NQE: Add more statistics to ObservationBuffer class Add APIs to compute weighted average and unweighted average to the ObservationBuffer class. These statistics would be used to compute the network quality, and then compare their accuracy with the weighted median statistic (which is currently in use). CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=649887 Committed: https://crrev.com/d53a4ba712f3941e9c7f9f70dbc75fd2136ff14e Cr-Commit-Position: refs/heads/master@{#426095} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d53a4ba712f3941e9c7f9f70dbc75fd2136ff14e Cr-Commit-Position: refs/heads/master@{#426095} |
