Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1164713004:
Store network quality samples so we can compute percentiles. (Closed)
DescriptionStore network quality samples so we can compute percentiles.
This CL stores the the samples from organic traffic (along
with the timestamp) in a deque. These would be used in a
later CL to compute the network quality at different
percentile values (e.g., 10, 50, 90).
It is ensured that RTT sample is stored only if this is the
first time the response is read for the given URL request.
Also, renamed GetEstimate() -> GetPeakEstimate(). This is
in preparation for next CL that will introduce API
GetEstimate(int percentile).
BUG=472681
Committed: https://crrev.com/6f840fc5d6482ef18945e501f729f8eb1c954f84
Cr-Commit-Position: refs/heads/master@{#334317}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Added RingBuffer class #
Total comments: 24
Patch Set 3 : Addressed mmenke comments #
Total comments: 12
Patch Set 4 : Addressed comments #
Total comments: 10
Patch Set 5 : Added tests, changed kbps to int32_t, removed confidence #
Total comments: 10
Patch Set 6 : Addressed comments. #
Total comments: 12
Patch Set 7 : Addressed bengr comments #
Total comments: 30
Patch Set 8 : rebased, addressed comments #Patch Set 9 : Using embedded server (instead of spawned) #
Total comments: 17
Patch Set 10 : Addressed comments #Patch Set 11 : Removed parenthesis #Patch Set 12 : Added test #
Total comments: 10
Patch Set 13 : Addressed comments #Patch Set 14 : Changes to make the tests work on Windows #Messages
Total messages: 51 (14 generated)
tbansal@chromium.org changed reviewers: + bengr@chromium.org
ptal. thanks.
https://codereview.chromium.org/1164713004/diff/1/net/base/network_quality.h File net/base/network_quality.h (right): https://codereview.chromium.org/1164713004/diff/1/net/base/network_quality.h#... net/base/network_quality.h:18: double rtt_confidence, Define what is meant by each of these parameters. https://codereview.chromium.org/1164713004/diff/1/net/base/network_quality.h#... net/base/network_quality.h:24: throughput_kbps_confidence(throughput_kbps_confidence) {} sanity check all parameters. https://codereview.chromium.org/1164713004/diff/1/net/base/network_quality_es... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1164713004/diff/1/net/base/network_quality_es... net/base/network_quality_estimator.cc:80: if (rtt_msec_.size() >= kMaximumSamples) Perhaps use a while loop, for better future proofing. https://codereview.chromium.org/1164713004/diff/1/net/base/network_quality_es... net/base/network_quality_estimator.cc:98: // Pop the oldest element if the buffer is already full. You should probably write a ring buffer instead of duplicating code. https://codereview.chromium.org/1164713004/diff/1/net/base/network_quality_es... net/base/network_quality_estimator.cc:99: if (kbps_.size() >= kMaximumSamples) Perhaps use a while loop, for better future proofing.
Patchset #2 (id:20001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:40001) has been deleted
ptal. thanks. https://codereview.chromium.org/1164713004/diff/1/net/base/network_quality.h File net/base/network_quality.h (right): https://codereview.chromium.org/1164713004/diff/1/net/base/network_quality.h#... net/base/network_quality.h:18: double rtt_confidence, On 2015/06/01 22:55:42, bengr wrote: > Define what is meant by each of these parameters. Done. https://codereview.chromium.org/1164713004/diff/1/net/base/network_quality.h#... net/base/network_quality.h:24: throughput_kbps_confidence(throughput_kbps_confidence) {} On 2015/06/01 22:55:42, bengr wrote: > sanity check all parameters. Done. https://codereview.chromium.org/1164713004/diff/1/net/base/network_quality_es... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1164713004/diff/1/net/base/network_quality_es... net/base/network_quality_estimator.cc:80: if (rtt_msec_.size() >= kMaximumSamples) On 2015/06/01 22:55:42, bengr wrote: > Perhaps use a while loop, for better future proofing. Added DCHECK_LE(samples_.size(), kMaximumSamples); before this if check. We should never need a while loop because the deque size should never exceed the maximum size. So, when adding a new sample, we should have to pop at most 1 existing sample. Anything else implies there is something wrong with the logic. https://codereview.chromium.org/1164713004/diff/1/net/base/network_quality_es... net/base/network_quality_estimator.cc:98: // Pop the oldest element if the buffer is already full. On 2015/06/01 22:55:42, bengr wrote: > You should probably write a ring buffer instead of duplicating code. Done. https://codereview.chromium.org/1164713004/diff/1/net/base/network_quality_es... net/base/network_quality_estimator.cc:99: if (kbps_.size() >= kMaximumSamples) On 2015/06/01 22:55:42, bengr wrote: > Perhaps use a while loop, for better future proofing. Please see the comment above.
tbansal@chromium.org changed reviewers: + mmenke@chromium.org
ptal. thanks.
Not tests? https://codereview.chromium.org/1164713004/diff/80001/net/base/network_quality.h File net/base/network_quality.h (right): https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality.h:44: DCHECK_LE(throughput_kbps_confidence, 1 + kDelta); Need base/logging.h (Which is weird to include in a header file, see next comment) https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality.h:45: } De-inline this constructor. Inline constructors shouldn't involve much code (And may be generally just a good idea to avoid them completely, anyways). https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:187: peak_kbps_since_last_connection_change_, 0.1); This seems a bit silly. Suggest just always returning 0, until we actually have a meaningful value. https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:199: DCHECK_GT(kMaximumSamples, 0U); If you think this is necessary, should use static_assert up where you declare the variable, rather than a runtime assertion. https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:61: explicit Sample(int value); Suggest taking the time here, to make things a but more explicit. Also remember someone saying something about possibly loading state from disk. https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:63: virtual ~Sample(); --virtual https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:66: const int value_; Classes aren't allowed to have public member variables. This should either be a struct, or these should be private with public accessors. Since they're in an stl container, and you need them to be copiable, I'd go with structs. https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:73: class RingBuffer { Think it's more important to name classes after what they contain, rather than the data format they use to do it, which is more of an internal implementation detail. Maybe just SampleBuffer, SampleCache, SampleSet, or even SampleRingBuffer, if you really think that's important. https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:77: virtual ~RingBuffer(); --virtual https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:83: const uint32_t kMaximumSamples = 300; static? https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:136: RingBuffer kbps_; Maybe kbps_samples_, and same for the next one?
Patchset #3 (id:100001) has been deleted
thanks for the comments. ptal. On 2015/06/03 18:31:40, mmenke wrote: > Not tests? This CL only does writes. Next CL does the percentile computation and reads. I have tests in the next CL. Does it make sense to have tests here too? (Originally I had writes, computation and reads in a single CL with all tests, but then I decided to split the CL in two). > > https://codereview.chromium.org/1164713004/diff/80001/net/base/network_quality.h > File net/base/network_quality.h (right): > > https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... > net/base/network_quality.h:44: DCHECK_LE(throughput_kbps_confidence, 1 + > kDelta); > Need base/logging.h (Which is weird to include in a header file, see next > comment) > > https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... > net/base/network_quality.h:45: } > De-inline this constructor. Inline constructors shouldn't involve much code > (And may be generally just a good idea to avoid them completely, anyways). > > https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... > File net/base/network_quality_estimator.cc (right): > > https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... > net/base/network_quality_estimator.cc:187: > peak_kbps_since_last_connection_change_, 0.1); > This seems a bit silly. Suggest just always returning 0, until we actually have > a meaningful value. > > https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... > net/base/network_quality_estimator.cc:199: DCHECK_GT(kMaximumSamples, 0U); > If you think this is necessary, should use static_assert up where you declare > the variable, rather than a runtime assertion. > > https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... > File net/base/network_quality_estimator.h (right): > > https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... > net/base/network_quality_estimator.h:61: explicit Sample(int value); > Suggest taking the time here, to make things a but more explicit. Also remember > someone saying something about possibly loading state from disk. > > https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... > net/base/network_quality_estimator.h:63: virtual ~Sample(); > --virtual > > https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... > net/base/network_quality_estimator.h:66: const int value_; > Classes aren't allowed to have public member variables. This should either be a > struct, or these should be private with public accessors. > > Since they're in an stl container, and you need them to be copiable, I'd go with > structs. > > https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... > net/base/network_quality_estimator.h:73: class RingBuffer { > Think it's more important to name classes after what they contain, rather than > the data format they use to do it, which is more of an internal implementation > detail. > > Maybe just SampleBuffer, SampleCache, SampleSet, or even SampleRingBuffer, if > you really think that's important. > > https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... > net/base/network_quality_estimator.h:77: virtual ~RingBuffer(); > --virtual > > https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... > net/base/network_quality_estimator.h:83: const uint32_t kMaximumSamples = 300; > static? > > https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... > net/base/network_quality_estimator.h:136: RingBuffer kbps_; > Maybe kbps_samples_, and same for the next one?
ptal. https://codereview.chromium.org/1164713004/diff/80001/net/base/network_quality.h File net/base/network_quality.h (right): https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality.h:44: DCHECK_LE(throughput_kbps_confidence, 1 + kDelta); On 2015/06/03 18:31:39, mmenke wrote: > Need base/logging.h (Which is weird to include in a header file, see next > comment) Done. https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality.h:45: } On 2015/06/03 18:31:39, mmenke wrote: > De-inline this constructor. Inline constructors shouldn't involve much code > (And may be generally just a good idea to avoid them completely, anyways). Done. https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:187: peak_kbps_since_last_connection_change_, 0.1); On 2015/06/03 18:31:39, mmenke wrote: > This seems a bit silly. Suggest just always returning 0, until we actually have > a meaningful value. Done. https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:199: DCHECK_GT(kMaximumSamples, 0U); On 2015/06/03 18:31:39, mmenke wrote: > If you think this is necessary, should use static_assert up where you declare > the variable, rather than a runtime assertion. Done. https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:61: explicit Sample(int value); On 2015/06/03 18:31:40, mmenke wrote: > Suggest taking the time here, to make things a but more explicit. Also remember > someone saying something about possibly loading state from disk. Done. https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:63: virtual ~Sample(); On 2015/06/03 18:31:39, mmenke wrote: > --virtual Done. https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:66: const int value_; On 2015/06/03 18:31:39, mmenke wrote: > Classes aren't allowed to have public member variables. This should either be a > struct, or these should be private with public accessors. > > Since they're in an stl container, and you need them to be copiable, I'd go with > structs. Done. https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:73: class RingBuffer { On 2015/06/03 18:31:39, mmenke wrote: > Think it's more important to name classes after what they contain, rather than > the data format they use to do it, which is more of an internal implementation > detail. > > Maybe just SampleBuffer, SampleCache, SampleSet, or even SampleRingBuffer, if > you really think that's important. Done. https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:77: virtual ~RingBuffer(); On 2015/06/03 18:31:39, mmenke wrote: > --virtual Done. https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:83: const uint32_t kMaximumSamples = 300; On 2015/06/03 18:31:40, mmenke wrote: > static? Done. https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:136: RingBuffer kbps_; On 2015/06/03 18:31:39, mmenke wrote: > Maybe kbps_samples_, and same for the next one? Done.
https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:73: class RingBuffer { On 2015/06/05 01:50:08, tbansal1 wrote: > On 2015/06/03 18:31:39, mmenke wrote: > > Think it's more important to name classes after what they contain, rather than > > the data format they use to do it, which is more of an internal implementation > > detail. > > > > Maybe just SampleBuffer, SampleCache, SampleSet, or even SampleRingBuffer, if > > you really think that's important. > > Done. I like SampleBuffer better. Also, can we use the word "Observation" instead of "Sample" everywhere? https://codereview.chromium.org/1164713004/diff/120001/net/base/network_quali... File net/base/network_quality.cc (right): https://codereview.chromium.org/1164713004/diff/120001/net/base/network_quali... net/base/network_quality.cc:39: base::TimeDelta NetworkQuality::GetRtt() const { Inline this simply as rtt() https://codereview.chromium.org/1164713004/diff/120001/net/base/network_quali... net/base/network_quality.cc:43: uint64_t NetworkQuality::GetThroughputKbps() const { Inline and rename downlink_throughput_kpps(). Rename the member variable as well. Return an int32_t. https://codereview.chromium.org/1164713004/diff/120001/net/base/network_quali... File net/base/network_quality.h (right): https://codereview.chromium.org/1164713004/diff/120001/net/base/network_quali... net/base/network_quality.h:26: uint64_t throughput_kbps, Be clear that this is downlink throuput. https://codereview.chromium.org/1164713004/diff/120001/net/base/network_quali... net/base/network_quality.h:27: double throughput_kbps_confidence); Follow the design and make this an enum, and don't separate RTT from throughput confidence. https://codereview.chromium.org/1164713004/diff/120001/net/base/network_quali... net/base/network_quality.h:35: uint64_t GetThroughputKbps() const; GetDownlinkThroughputKbps? Should this be an int32_t? I thought we were going to use int32s everywhere, and that's why the units are kbps instead of bps. https://codereview.chromium.org/1164713004/diff/120001/net/base/network_quali... net/base/network_quality.h:46: const double rtt_confidence_; I would assign one confidence to the NetworkQuality object, not one per metric. Too confusing. Also, I'd use an enum.
thanks for the comments. ptal. https://codereview.chromium.org/1164713004/diff/120001/net/base/network_quali... File net/base/network_quality.cc (right): https://codereview.chromium.org/1164713004/diff/120001/net/base/network_quali... net/base/network_quality.cc:39: base::TimeDelta NetworkQuality::GetRtt() const { On 2015/06/05 20:44:13, bengr wrote: > Inline this simply as rtt() Done. https://codereview.chromium.org/1164713004/diff/120001/net/base/network_quali... net/base/network_quality.cc:43: uint64_t NetworkQuality::GetThroughputKbps() const { On 2015/06/05 20:44:13, bengr wrote: > Inline and rename downlink_throughput_kpps(). Rename the member variable as > well. Return an int32_t. Done. Will change to int32_t in next CL. Added TODO. https://codereview.chromium.org/1164713004/diff/120001/net/base/network_quali... File net/base/network_quality.h (right): https://codereview.chromium.org/1164713004/diff/120001/net/base/network_quali... net/base/network_quality.h:26: uint64_t throughput_kbps, On 2015/06/05 20:44:13, bengr wrote: > Be clear that this is downlink throuput. Done. https://codereview.chromium.org/1164713004/diff/120001/net/base/network_quali... net/base/network_quality.h:27: double throughput_kbps_confidence); On 2015/06/05 20:44:13, bengr wrote: > Follow the design and make this an enum, Will do this in next CL, added TODO > and don't separate RTT from throughput > confidence. Done https://codereview.chromium.org/1164713004/diff/120001/net/base/network_quali... net/base/network_quality.h:35: uint64_t GetThroughputKbps() const; On 2015/06/05 20:44:13, bengr wrote: > GetDownlinkThroughputKbps? > > Should this be an int32_t? I thought we were going to use int32s everywhere, and > that's why the units are kbps instead of bps. Added TODO. https://codereview.chromium.org/1164713004/diff/120001/net/base/network_quali... net/base/network_quality.h:46: const double rtt_confidence_; On 2015/06/05 20:44:13, bengr wrote: > I would assign one confidence to the NetworkQuality object, not one per metric. > Too confusing. Done. > > Also, I'd use an enum. Added TODO.
Patchset #4 (id:140001) has been deleted
https://codereview.chromium.org/1164713004/diff/160001/net/base/network_quali... File net/base/network_quality.h (right): https://codereview.chromium.org/1164713004/diff/160001/net/base/network_quali... net/base/network_quality.h:52: const double confidence_; Actually, can we just get rid of all this until it means something? https://codereview.chromium.org/1164713004/diff/160001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1164713004/diff/160001/net/base/network_quali... net/base/network_quality_estimator.cc:45: int64_t cummulative_prefilter_bytes_read, cumulative only has one m
On 2015/06/08 16:07:04, mmenke wrote: > https://codereview.chromium.org/1164713004/diff/160001/net/base/network_quali... > File net/base/network_quality.h (right): > > https://codereview.chromium.org/1164713004/diff/160001/net/base/network_quali... > net/base/network_quality.h:52: const double confidence_; > Actually, can we just get rid of all this until it means something? > > https://codereview.chromium.org/1164713004/diff/160001/net/base/network_quali... > File net/base/network_quality_estimator.cc (right): > > https://codereview.chromium.org/1164713004/diff/160001/net/base/network_quali... > net/base/network_quality_estimator.cc:45: int64_t > cummulative_prefilter_bytes_read, > cumulative only has one m Oops...didn't mean to publish quite yet (Too...many...reviews)
This seems reasonable, but it really does seem to me like there should be some tests: NotifyDataReceived with the max int64 (Or at least something above the largest int32). Check that you're correctly removing extra observations from the list. If adding the code to create an estimate from samples in this CL makes writing tests easier, I'm fine with that. https://codereview.chromium.org/1164713004/diff/160001/net/base/network_quali... File net/base/network_quality.h (right): https://codereview.chromium.org/1164713004/diff/160001/net/base/network_quali... net/base/network_quality.h:34: uint64_t downlink_throughput_kbps() const { Believe this should be downstream (Or download) https://codereview.chromium.org/1164713004/diff/160001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1164713004/diff/160001/net/base/network_quali... net/base/network_quality_estimator.cc:86: kbps_observations_.AddObservation(kbps); AddObservation takes an int, not a uint64. Should fix that in this CL (Suggest just using ints, with a cap of max int) https://codereview.chromium.org/1164713004/diff/160001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1164713004/diff/160001/net/base/network_quali... net/base/network_quality_estimator.h:127: bool bytes_read_since_last_connection_change_; Suggest renaming this. "bytes_read_since_last_connection_change_" sounds like a count of bytes, not a bool to me. Maybe "have_received_bytes_since_..."
Addressed comments, added tests, changed kbps to int32_t, changed downlink to downstream. PTAL. Thanks for the comments. https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1164713004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.h:73: class RingBuffer { On 2015/06/05 20:44:13, bengr wrote: > On 2015/06/05 01:50:08, tbansal1 wrote: > > On 2015/06/03 18:31:39, mmenke wrote: > > > Think it's more important to name classes after what they contain, rather > than > > > the data format they use to do it, which is more of an internal > implementation > > > detail. > > > > > > Maybe just SampleBuffer, SampleCache, SampleSet, or even SampleRingBuffer, > if > > > you really think that's important. > > > > Done. > > I like SampleBuffer better. Also, can we use the word "Observation" instead of > "Sample" everywhere? Done. https://codereview.chromium.org/1164713004/diff/160001/net/base/network_quali... File net/base/network_quality.h (right): https://codereview.chromium.org/1164713004/diff/160001/net/base/network_quali... net/base/network_quality.h:34: uint64_t downlink_throughput_kbps() const { On 2015/06/08 17:30:16, mmenke wrote: > Believe this should be downstream (Or download) Done. https://codereview.chromium.org/1164713004/diff/160001/net/base/network_quali... net/base/network_quality.h:52: const double confidence_; On 2015/06/08 16:07:04, mmenke wrote: > Actually, can we just get rid of all this until it means something? Done. https://codereview.chromium.org/1164713004/diff/160001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1164713004/diff/160001/net/base/network_quali... net/base/network_quality_estimator.cc:45: int64_t cummulative_prefilter_bytes_read, On 2015/06/08 16:07:04, mmenke wrote: > cumulative only has one m Done. https://codereview.chromium.org/1164713004/diff/160001/net/base/network_quali... net/base/network_quality_estimator.cc:86: kbps_observations_.AddObservation(kbps); On 2015/06/08 17:30:16, mmenke wrote: > AddObservation takes an int, not a uint64. Should fix that in this CL (Suggest > just using ints, with a cap of max int) Changed to int32_t. Thats what histogram uses: https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo... Added check for value <0 in AddObservation() https://codereview.chromium.org/1164713004/diff/160001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1164713004/diff/160001/net/base/network_quali... net/base/network_quality_estimator.h:127: bool bytes_read_since_last_connection_change_; On 2015/06/08 17:30:16, mmenke wrote: > Suggest renaming this. "bytes_read_since_last_connection_change_" sounds like a > count of bytes, not a bool to me. > > Maybe "have_received_bytes_since_..." Done.
https://codereview.chromium.org/1164713004/diff/180001/net/base/network_quali... File net/base/network_quality.h (right): https://codereview.chromium.org/1164713004/diff/180001/net/base/network_quali... net/base/network_quality.h:27: // Returns the round trip time observed for the current connection. You could add the clause "for the current connection" to the class description, after "the current network quality". Then you could remove that clause here and below. https://codereview.chromium.org/1164713004/diff/180001/net/base/network_quali... net/base/network_quality.h:28: base::TimeDelta rtt() const { return rtt_; } Should this return a const&? https://codereview.chromium.org/1164713004/diff/180001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1164713004/diff/180001/net/base/network_quali... net/base/network_quality_estimator.cc:185: return NetworkQuality(fastest_RTT_since_last_connection_change_, 0); change the name to fastest_rtt_since_last_connection_change_ https://codereview.chromium.org/1164713004/diff/180001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1164713004/diff/180001/net/base/network_quali... net/base/network_quality_estimator.h:90: static const size_t kMaximumObservations; Why not just put this in an anonymous namespace in the .cc? https://codereview.chromium.org/1164713004/diff/180001/net/base/network_quali... net/base/network_quality_estimator.h:92: // Buffer that holds observations sorted by time. Perhaps rewrite as: Holds observations sorted by time, with the oldest observation at the front of the queue.
ptal. thanks. https://codereview.chromium.org/1164713004/diff/180001/net/base/network_quali... File net/base/network_quality.h (right): https://codereview.chromium.org/1164713004/diff/180001/net/base/network_quali... net/base/network_quality.h:27: // Returns the round trip time observed for the current connection. On 2015/06/09 20:46:33, bengr wrote: > You could add the clause "for the current connection" to the class description, > after "the current network quality". Then you could remove that clause here and > below. Actually, there is nothing that says NetworkQuality class is for "current" networks only. So, I removed "current" from the comments. Also, followed on your suggestion to simplify the comments. https://codereview.chromium.org/1164713004/diff/180001/net/base/network_quali... net/base/network_quality.h:28: base::TimeDelta rtt() const { return rtt_; } On 2015/06/09 20:46:32, bengr wrote: > Should this return a const&? Done. https://codereview.chromium.org/1164713004/diff/180001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1164713004/diff/180001/net/base/network_quali... net/base/network_quality_estimator.cc:185: return NetworkQuality(fastest_RTT_since_last_connection_change_, 0); On 2015/06/09 20:46:33, bengr wrote: > change the name to fastest_rtt_since_last_connection_change_ Done. https://codereview.chromium.org/1164713004/diff/180001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1164713004/diff/180001/net/base/network_quali... net/base/network_quality_estimator.h:90: static const size_t kMaximumObservations; On 2015/06/09 20:46:33, bengr wrote: > Why not just put this in an anonymous namespace in the .cc? Now, this is exposed to tests. I believe in that case, putting in .h is a better design than putting in anonymous namespace and then exposing it. https://codereview.chromium.org/1164713004/diff/180001/net/base/network_quali... net/base/network_quality_estimator.h:92: // Buffer that holds observations sorted by time. On 2015/06/09 20:46:33, bengr wrote: > Perhaps rewrite as: > > Holds observations sorted by time, with the oldest observation at the front of > the queue. Done.
lgtm with nits https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... File net/base/network_quality.h (right): https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... net/base/network_quality.h:29: // Returns the estimate of the downstream throughput in Kbps. Is this bytes or bits? Maybe KB/s? https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... net/base/network_quality_estimator.h:60: // Observation is used to store RTT and Kbps observation in buffers. An // Records the round trip time or throughput observation, along with the time the observation was made. https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... net/base/network_quality_estimator.h:74: // ObservationBuffer is used to store observations sorted by time. // Stores observations sorted by time. https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... net/base/network_quality_estimator.h:82: // will be popped out and discarded if the buffer is already full. popped out and discarded -> evicted to make room https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... net/base/network_quality_estimator.h:146: // Buffer that holds Kbps observations. KB/s everywhere in comments https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:107: TestDelegate d; d -> test_delegate
On 2015/06/11 00:14:56, bengr wrote: > lgtm with nits > > https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... > File net/base/network_quality.h (right): > > https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... > net/base/network_quality.h:29: // Returns the estimate of the downstream > throughput in Kbps. > Is this bytes or bits? Maybe KB/s? > > https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... > File net/base/network_quality_estimator.h (right): > > https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... > net/base/network_quality_estimator.h:60: // Observation is used to store RTT and > Kbps observation in buffers. An > // Records the round trip time or throughput observation, along with the time > the observation was made. > > https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... > net/base/network_quality_estimator.h:74: // ObservationBuffer is used to store > observations sorted by time. > // Stores observations sorted by time. > > https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... > net/base/network_quality_estimator.h:82: // will be popped out and discarded if > the buffer is already full. > popped out and discarded -> evicted to make room > > https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... > net/base/network_quality_estimator.h:146: // Buffer that holds Kbps > observations. > KB/s everywhere in comments > > https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... > File net/base/network_quality_estimator_unittest.cc (right): > > https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... > net/base/network_quality_estimator_unittest.cc:107: TestDelegate d; > d -> test_delegate Ignore my comments about Kbps. You meant bits but I thought you meant bytes but wrote a lower case b. My mistake.
Patchset #7 (id:220001) has been deleted
ptal. thanks. https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... File net/base/network_quality.h (right): https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... net/base/network_quality.h:29: // Returns the estimate of the downstream throughput in Kbps. On 2015/06/11 00:14:56, bengr wrote: > Is this bytes or bits? Maybe KB/s? Kilo bits per second. Added comment to make it unambiguous. Done. https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... net/base/network_quality_estimator.h:60: // Observation is used to store RTT and Kbps observation in buffers. An On 2015/06/11 00:14:56, bengr wrote: > // Records the round trip time or throughput observation, along with the time > the observation was made. Done. https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... net/base/network_quality_estimator.h:74: // ObservationBuffer is used to store observations sorted by time. On 2015/06/11 00:14:56, bengr wrote: > // Stores observations sorted by time. Done. https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... net/base/network_quality_estimator.h:82: // will be popped out and discarded if the buffer is already full. On 2015/06/11 00:14:56, bengr wrote: > popped out and discarded -> evicted to make room Done. https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... net/base/network_quality_estimator.h:146: // Buffer that holds Kbps observations. On 2015/06/11 00:14:56, bengr wrote: > KB/s everywhere in comments Currently, it is Kbps (kilo bits per second). As discussed offline, this is correct. https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1164713004/diff/200001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:107: TestDelegate d; On 2015/06/11 00:14:56, bengr wrote: > d -> test_delegate Done.
https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator.cc:63: DCHECK_GE(request_duration, base::TimeDelta()); If this is 0, we're in trouble, aren't we? Should we exit early in that case? https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator.cc:69: fastest_rtt_since_last_connection_change_ = request_duration; Suggest only doing this if cumulative_prefilter_bytes_read != prefiltered_bytes_read. Also note that if there are redirects, this can give very inaccurate numbers. Could use request.GetLoadTimeInfo and just find the difference between now and send_start. Those changes don't really belong in this CL, but should give better estimates. Note that I'm not sure if we currently set send_start in the case of QUIC. https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator.cc:82: request_duration.InMicroseconds()); Should handle overflow here. Unlikely, even for local transfers, but best to be careful. Alternatively, could expose a double instead of an int32 https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator.cc:176: current_connection_type_ = type; Should we throw out our observations here? If so, should also have a test for that. https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator.cc:183: return NetworkQuality(base::TimeDelta(), 0); Returning 0 as the default peak transfer rate seems reasonable. Returning 0 as the default observed rtt seems questionable. https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator.cc:201: "Minimum size of observation buffer must be > 0"); This should go just under the line where you define kMaximumObservations. It doesn't map to code, it's a compile-time assertion. https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator.cc:209: if (value < 0) When can this happen? https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator.cc:215: observations_.push_back(Observation(value, base::TimeTicks::Now())); Suggest passing base::TimeTicks::Now() in as an argument - OSes *probably* all cache it, but best to be safe. Could optionally just pass in an Observation to this method. https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator.cc:219: size_t NetworkQualityEstimator::ObservationBuffer::size() const { If this isn't inlined in the header, it should be named Size(). https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator.h:144: int32_t peak_kbps_since_last_connection_change_; Should we really provide kilobits per second? Seems like the first thing everyone will do with it to actually use it is multiple it by 8. For that matter, is there some reason not to just give bytes per second? Seems like that's the most directly usable by consumers. https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:98: // SpawnedTestServer not supported on iOS (see http://crbug.com/148666). Can we just use the embedded test server?
Patchset #8 (id:260001) has been deleted
ptal. Had to rebase. @mmenke: Please look at net/* thanks. https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator.cc:63: DCHECK_GE(request_duration, base::TimeDelta()); On 2015/06/11 15:11:01, mmenke wrote: > If this is 0, we're in trouble, aren't we? Should we exit early in that case? We may not be in trouble. It will cause peak_rtt to be 0. It won't affect kbps computation (that requires the time duration to be greater than |kMinRequestDurationMicroseconds|. I am assuming this will happen rarely enough that median RTT computation won't be affected. Also, this being zero is not really any different from this being very low (say 1 microsecond). https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator.cc:69: fastest_rtt_since_last_connection_change_ = request_duration; On 2015/06/11 15:11:01, mmenke wrote: > Suggest only doing this if cumulative_prefilter_bytes_read != > prefiltered_bytes_read. > > Also note that if there are redirects, this can give very inaccurate numbers. > Could use request.GetLoadTimeInfo and just find the difference between now and > send_start. > > Those changes don't really belong in this CL, but should give better estimates. > Note that I'm not sure if we currently set send_start in the case of QUIC. Can you please explain why this should be done only when cumulative_prefilter_bytes_read != prefiltered_bytes_read. Also, I am already working on using LoadTimingInfo instead of the creation_time() https://codereview.chromium.org/1162293004/ https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator.cc:82: request_duration.InMicroseconds()); On 2015/06/11 15:11:01, mmenke wrote: > Should handle overflow here. Unlikely, even for local transfers, but best to be > careful. Alternatively, could expose a double instead of an int32 I was handling the overflow in AddObservation() but it seems better to handle it here itself. Done. https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator.cc:176: current_connection_type_ = type; On 2015/06/11 15:11:01, mmenke wrote: > Should we throw out our observations here? If so, should also have a test for > that. Yes, we should. This was part of the bigger CL (that also computes the percentiles). Forgot to move it here from the bigger CL. Added now, also added test. https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator.cc:183: return NetworkQuality(base::TimeDelta(), 0); On 2015/06/11 15:11:01, mmenke wrote: > Returning 0 as the default peak transfer rate seems reasonable. Returning 0 as > the default observed rtt seems questionable. Done. Default rtt is now base::TimeDelta::Max(). This simplified the code. thanks. https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator.cc:201: "Minimum size of observation buffer must be > 0"); On 2015/06/11 15:11:01, mmenke wrote: > This should go just under the line where you define kMaximumObservations. It > doesn't map to code, it's a compile-time assertion. Problem there is that it does not compile because ObservationBuffer and kMaximumObservations are private. Here, it compiles and also gets triggered if the condition is not true. https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator.cc:209: if (value < 0) On 2015/06/11 15:11:01, mmenke wrote: > When can this happen? This is for the case if there is an overflow on the kbps computation. Overflow may cause |value| to be negative. Moved this check up in the NotifyDataReceived() https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator.cc:215: observations_.push_back(Observation(value, base::TimeTicks::Now())); On 2015/06/11 15:11:01, mmenke wrote: > Suggest passing base::TimeTicks::Now() in as an argument - OSes *probably* all > cache it, but best to be safe. Could optionally just pass in an Observation to > this method. Done. https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator.cc:219: size_t NetworkQualityEstimator::ObservationBuffer::size() const { On 2015/06/11 15:11:01, mmenke wrote: > If this isn't inlined in the header, it should be named Size(). Done. https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator.h:144: int32_t peak_kbps_since_last_connection_change_; On 2015/06/11 15:11:01, mmenke wrote: > Should we really provide kilobits per second? Seems like the first thing > everyone will do with it to actually use it is multiple it by 8. > > For that matter, is there some reason not to just give bytes per second? Seems > like that's the most directly usable by consumers. It seems that (kilo|mega) bits per second is used everywhere in NCN. I tried to be consistent with that. e.g., this is mbps: https://code.google.com/p/chromium/codesearch#chromium/src/net/base/network_c... this is kbps: https://code.google.com/p/chromium/codesearch#chromium/src/net/base/network_c... I wanted to operate in int32 regime (instead of doule), so used kbps. WDYT? https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:98: // SpawnedTestServer not supported on iOS (see http://crbug.com/148666). On 2015/06/11 15:11:01, mmenke wrote: > Can we just use the embedded test server? Is it okay if I add a TODO with crbug against myself and do that in a separate CL?
Quick responses. https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator.cc:69: fastest_rtt_since_last_connection_change_ = request_duration; On 2015/06/11 19:27:50, tbansal1 wrote: > On 2015/06/11 15:11:01, mmenke wrote: > > Suggest only doing this if cumulative_prefilter_bytes_read != > > prefiltered_bytes_read. > > > > Also note that if there are redirects, this can give very inaccurate numbers. > > Could use request.GetLoadTimeInfo and just find the difference between now and > > send_start. > > > > Those changes don't really belong in this CL, but should give better > estimates. > > Note that I'm not sure if we currently set send_start in the case of QUIC. > > Can you please explain why this should be done only when > cumulative_prefilter_bytes_read != > prefiltered_bytes_read. > > Also, I am already working on using LoadTimingInfo instead of the > creation_time() > https://codereview.chromium.org/1162293004/ RTT should be from when we send the headers to when we get the first part of the response, not from when the request is created to when we receive a random byte from it. Consider: t=0: Create request. t=1: Send request. t=2: Get first bytes, call this with, say "NotifyDataReceived(_, 10, 10)"; t=3: Get more bytes, call this with, say "NotifyDataReceived(_, 20, 10)"; t=4: Get more bytes, call this with, say "NotifyDataReceived(_, 30, 10)"; t=5: ... ... We should only record an rtt of 1 (t=3 - t=2), but with this code, we'd record rtts of 2, 3, 4, 5, 6, 7, 8, .... https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:98: // SpawnedTestServer not supported on iOS (see http://crbug.com/148666). On 2015/06/11 19:27:50, tbansal1 wrote: > On 2015/06/11 15:11:01, mmenke wrote: > > Can we just use the embedded test server? > > Is it okay if I add a TODO with crbug against myself and do that in a separate > CL? The embedded test server has a ServeFilesFromDirectory method, so this should be a trivial change (Unless it has trouble finding the files on Android). You may also need to add a slash before echo.html. https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:99: #if !defined(OS_IOS) Rather than if-def-ing out the test, please use: #if ... #define MAYBE_BLAH DISABLED_BLAH #else #define MAYBE_BLAH BLAH #endif Those are much easier to find when one is searching for disabled tests. Should fix the previous test, too, if we don't fix this as part of this CL. https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:126: base::PlatformThread::Sleep(request_duration); Do you even need to run the requests here?
thanks for the comments. ptal. https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator.cc:69: fastest_rtt_since_last_connection_change_ = request_duration; On 2015/06/11 19:51:18, mmenke wrote: > On 2015/06/11 19:27:50, tbansal1 wrote: > > On 2015/06/11 15:11:01, mmenke wrote: > > > Suggest only doing this if cumulative_prefilter_bytes_read != > > > prefiltered_bytes_read. > > > > > > Also note that if there are redirects, this can give very inaccurate > numbers. > > > Could use request.GetLoadTimeInfo and just find the difference between now > and > > > send_start. > > > > > > Those changes don't really belong in this CL, but should give better > > estimates. > > > Note that I'm not sure if we currently set send_start in the case of QUIC. > > > > Can you please explain why this should be done only when > > cumulative_prefilter_bytes_read != > > prefiltered_bytes_read. > > > > Also, I am already working on using LoadTimingInfo instead of the > > creation_time() > > https://codereview.chromium.org/1162293004/ > > RTT should be from when we send the headers to when we get the first part of the > response, not from when the request is created to when we receive a random byte > from it. > > Consider: > > t=0: Create request. > t=1: Send request. > t=2: Get first bytes, call this with, say "NotifyDataReceived(_, 10, 10)"; > t=3: Get more bytes, call this with, say "NotifyDataReceived(_, 20, 10)"; > t=4: Get more bytes, call this with, say "NotifyDataReceived(_, 30, 10)"; > t=5: ... > ... > > We should only record an rtt of 1 (t=3 - t=2), but with this code, we'd record > rtts of 2, 3, 4, 5, 6, 7, 8, .... Right, it makes sense to do RTT only when cumulative_prefilter_bytes_read == prefiltered_bytes_read. I did that only for RTT samples and not for fastest_rtt. Now, doing this for fastest_rtt also. I think I got confused because initially you said we should do this when cumulative_prefilter_bytes_read != prefiltered_bytes_read (!= instead of ==) https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:98: // SpawnedTestServer not supported on iOS (see http://crbug.com/148666). On 2015/06/11 19:51:18, mmenke wrote: > On 2015/06/11 19:27:50, tbansal1 wrote: > > On 2015/06/11 15:11:01, mmenke wrote: > > > Can we just use the embedded test server? > > > > Is it okay if I add a TODO with crbug against myself and do that in a separate > > CL? > > The embedded test server has a ServeFilesFromDirectory method, so this should be > a trivial change (Unless it has trouble finding the files on Android). You may > also need to add a slash before echo.html. Done. https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:99: #if !defined(OS_IOS) On 2015/06/11 19:51:18, mmenke wrote: > Rather than if-def-ing out the test, please use: > > #if ... > #define MAYBE_BLAH DISABLED_BLAH > #else > #define MAYBE_BLAH BLAH > #endif > > Those are much easier to find when one is searching for disabled tests. Should > fix the previous test, too, if we don't fix this as part of this CL. I think the problem was that SpawnedTestServer is not even defined on iOS. So, it gives compilation error when using this DISABLED_*. (Apparently DISABLED_* only disables the execution of test. What I was doing was preventing the compilation also). https://codereview.chromium.org/1164713004/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:126: base::PlatformThread::Sleep(request_duration); On 2015/06/11 19:51:18, mmenke wrote: > Do you even need to run the requests here? Yes, otherwise the observation will be discarded due to shorter duration.
Want to do another pass on the tests, and see if we can make the kbps math prettier, but I'm pretty happy with this.
Not sure how I can make it prettier. Suggestions welcome. Thanks for taking time to review this.
https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... net/base/network_quality_estimator.cc:18: // Maximum number of observations to hold in the ObservationBuffer. Description should go with the variable declaration (If it's already there, no need to have it in both places) https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... net/base/network_quality_estimator.cc:20: 500; Your other constants are inlined in the header. Can we do that with this one, too, or does the fact it's a size_t cause issues? https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... net/base/network_quality_estimator.cc:83: if (kbps > 0) { This doesn't cover all overflow cases. Consider that 0x70000001 * 8000 will give a value of 8000. Moreover, if overflow seems unlikely, consider that this overflows at a rate of merely 2gigabits per microsecond, which is actually just 268 kBps. Suggest just using InSecondsF() and explicitly comparing to max int. Also suggest a test for the overflow case. https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:115: 10U; Starting at i=0 and using < is far more common. https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:124: base::PlatformThread::Sleep(request_duration); On 2015/06/11 21:16:29, tbansal1 wrote: > On 2015/06/11 19:51:18, mmenke wrote: > > Do you even need to run the requests here? > > Yes, otherwise the observation will be discarded due to shorter duration. I'm not following. You're using creation time, not start time.
https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... net/base/network_quality_estimator.cc:87: kbps_observations_.AddObservation(Observation(kbps, now)); Also, I wonder about repeatedly adding observations for the same download. One download from a slow server completely swamps everything else. Wonder if it would be better to more of a cumulative bandwidth thing - add up all new bytes received over a unit of time, and use that for the estimate. Admittedly, doesn't handle the case where we aren't fully utilizing out bandwidth. Anyhow, just a thought for another time - not suggesting you rework this CL.
ptal. thanks. https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... net/base/network_quality_estimator.cc:18: // Maximum number of observations to hold in the ObservationBuffer. On 2015/06/12 17:34:42, mmenke wrote: > Description should go with the variable declaration (If it's already there, no > need to have it in both places) Done. https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... net/base/network_quality_estimator.cc:20: 500; On 2015/06/12 17:34:42, mmenke wrote: > Your other constants are inlined in the header. Can we do that with this one, > too, or does the fact it's a size_t cause issues? Yes, I tried that before: size_t causes issues. https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... net/base/network_quality_estimator.cc:83: if (kbps > 0) { On 2015/06/12 17:34:42, mmenke wrote: > This doesn't cover all overflow cases. Consider that 0x70000001 * 8000 will > give a value of 8000. > > Moreover, if overflow seems unlikely, consider that this overflows at a rate of > merely 2gigabits per microsecond, which is actually just 268 kBps. > > Suggest just using InSecondsF() and explicitly comparing to max int. > > Also suggest a test for the overflow case. Fixed but how is 2 gigabits per microsecond == 268 kBps? Also, not sure how to write a test that is not flaky. I can pass INT_MAX bytes to this function, but that does not guarantee it will trigger overflow detection and correction (depending on how long machine sleeps etc.). https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... net/base/network_quality_estimator.cc:87: kbps_observations_.AddObservation(Observation(kbps, now)); On 2015/06/12 18:08:18, mmenke wrote: > Also, I wonder about repeatedly adding observations for the same download. One > download from a slow server completely swamps everything else. Wonder if it > would be better to more of a cumulative bandwidth thing - add up all new bytes > received over a unit of time, and use that for the estimate. Admittedly, > doesn't handle the case where we aren't fully utilizing out bandwidth. > > Anyhow, just a thought for another time - not suggesting you rework this CL. Yes, I have been playing around with it. General observation is that bandwidth estimate is more accurate when there is more data. So, we plan to do bandwidth sampling at the end of the request. https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:115: 10U; On 2015/06/12 17:34:42, mmenke wrote: > Starting at i=0 and using < is far more common. Done. https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:124: base::PlatformThread::Sleep(request_duration); On 2015/06/12 17:34:42, mmenke wrote: > On 2015/06/11 21:16:29, tbansal1 wrote: > > On 2015/06/11 19:51:18, mmenke wrote: > > > Do you even need to run the requests here? > > > > Yes, otherwise the observation will be discarded due to shorter duration. > > I'm not following. You're using creation time, not start time. Yes, the Kbps computation proceeds only if the duration between |now | and |creation time| is more than |kMinRequestDurationMicroseconds|. So, I simply sleep here to keep that duration longer than the threshold. (creation_time is a const that I can't change, and probably won't be a good design either).
https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... net/base/network_quality_estimator.cc:83: if (kbps > 0) { On 2015/06/12 19:14:46, tbansal1 wrote: > On 2015/06/12 17:34:42, mmenke wrote: > > This doesn't cover all overflow cases. Consider that 0x70000001 * 8000 will > > give a value of 8000. > > > > Moreover, if overflow seems unlikely, consider that this overflows at a rate > of > > merely 2gigabits per microsecond, which is actually just 268 kBps. > > > > Suggest just using InSecondsF() and explicitly comparing to max int. > > > > Also suggest a test for the overflow case. > > Fixed but how is 2 gigabits per microsecond == 268 kBps? Sorry, my mistake. What I meant was this overflows when cumulative_prefilter_bytes_read is 268k, which is gets you 8000 * 268,000 = 2,000,000,000, and I was thinking we divide by microseconds, so...it must by 268 kbps! (Oops) > Also, not sure how to write a test that is not flaky. I can pass INT_MAX bytes > to this function, but that does not guarantee it will trigger overflow detection > and correction (depending on how long machine sleeps etc.). The bytes read we pass in is an int64. If we pass in 2.21 petabytes, regardless of any skew in the time, that should be enough, no? https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... net/base/network_quality_estimator.cc:87: kbps_observations_.AddObservation(Observation(kbps, now)); On 2015/06/12 19:14:46, tbansal1 wrote: > On 2015/06/12 18:08:18, mmenke wrote: > > Also, I wonder about repeatedly adding observations for the same download. > One > > download from a slow server completely swamps everything else. Wonder if it > > would be better to more of a cumulative bandwidth thing - add up all new bytes > > received over a unit of time, and use that for the estimate. Admittedly, > > doesn't handle the case where we aren't fully utilizing out bandwidth. > > > > Anyhow, just a thought for another time - not suggesting you rework this CL. > > Yes, I have been playing around with it. General observation is that > bandwidth estimate is more accurate when there is more data. So, we plan to do > bandwidth sampling at the end of the request. You could also try subtracting out 1/2 of rtt from the duration, and see if that helps (Once you switch to using LoadTimingInfo).
ptal. https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... net/base/network_quality_estimator.cc:83: if (kbps > 0) { On 2015/06/12 19:36:06, mmenke wrote: > On 2015/06/12 19:14:46, tbansal1 wrote: > > On 2015/06/12 17:34:42, mmenke wrote: > > > This doesn't cover all overflow cases. Consider that 0x70000001 * 8000 will > > > give a value of 8000. > > > > > > Moreover, if overflow seems unlikely, consider that this overflows at a rate > > of > > > merely 2gigabits per microsecond, which is actually just 268 kBps. > > > > > > Suggest just using InSecondsF() and explicitly comparing to max int. > > > > > > Also suggest a test for the overflow case. > > > > Fixed but how is 2 gigabits per microsecond == 268 kBps? > > Sorry, my mistake. What I meant was this overflows when > cumulative_prefilter_bytes_read is 268k, which is gets you 8000 * 268,000 = > 2,000,000,000, and I was thinking we divide by microseconds, so...it must by 268 > kbps! (Oops) > Not sure. Because |cumulative_prefilter_bytes_read| is int64_t, the product has to hit 2,000,000,000 * 2,000,000,000 (not just 2,000,000,000). And, InMicroseconds() is int64 too. Btw, I totally agree that this was probably confusing. The updated method is cleaner. > > Also, not sure how to write a test that is not flaky. I can pass INT_MAX bytes > > to this function, but that does not guarantee it will trigger overflow > detection > > and correction (depending on how long machine sleeps etc.). > > The bytes read we pass in is an int64. If we pass in 2.21 petabytes, regardless > of any skew in the time, that should be enough, no? If 2.21 petabytes are received in 1 petaseconds, then there won;t be an overflow. But yeah, 1 peta second is obviously impossible. So, added the test. https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... net/base/network_quality_estimator.cc:87: kbps_observations_.AddObservation(Observation(kbps, now)); On 2015/06/12 19:36:06, mmenke wrote: > On 2015/06/12 19:14:46, tbansal1 wrote: > > On 2015/06/12 18:08:18, mmenke wrote: > > > Also, I wonder about repeatedly adding observations for the same download. > > One > > > download from a slow server completely swamps everything else. Wonder if it > > > would be better to more of a cumulative bandwidth thing - add up all new > bytes > > > received over a unit of time, and use that for the estimate. Admittedly, > > > doesn't handle the case where we aren't fully utilizing out bandwidth. > > > > > > Anyhow, just a thought for another time - not suggesting you rework this CL. > > > > Yes, I have been playing around with it. General observation is that > > bandwidth estimate is more accurate when there is more data. So, we plan to do > > bandwidth sampling at the end of the request. > > You could also try subtracting out 1/2 of rtt from the duration, and see if that > helps (Once you switch to using LoadTimingInfo). Good suggestion. Will do that.
LGTM https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1164713004/diff/300001/net/base/network_quali... net/base/network_quality_estimator.cc:83: if (kbps > 0) { On 2015/06/12 19:56:26, tbansal1 wrote: > On 2015/06/12 19:36:06, mmenke wrote: > > On 2015/06/12 19:14:46, tbansal1 wrote: > > > On 2015/06/12 17:34:42, mmenke wrote: > > > > This doesn't cover all overflow cases. Consider that 0x70000001 * 8000 > will > > > > give a value of 8000. > > > > > > > > Moreover, if overflow seems unlikely, consider that this overflows at a > rate > > > of > > > > merely 2gigabits per microsecond, which is actually just 268 kBps. > > > > > > > > Suggest just using InSecondsF() and explicitly comparing to max int. > > > > > > > > Also suggest a test for the overflow case. > > > > > > Fixed but how is 2 gigabits per microsecond == 268 kBps? > > > > Sorry, my mistake. What I meant was this overflows when > > cumulative_prefilter_bytes_read is 268k, which is gets you 8000 * 268,000 = > > 2,000,000,000, and I was thinking we divide by microseconds, so...it must by > 268 > > kbps! (Oops) > > > Not sure. Because |cumulative_prefilter_bytes_read| is int64_t, the product has > to hit 2,000,000,000 * 2,000,000,000 (not just 2,000,000,000). And, > InMicroseconds() is int64 too. Ah, right. I realized that yesterday, but today is Friday (which changes everything). :) > Btw, I totally agree that this was probably confusing. The updated method is > cleaner. > > > > Also, not sure how to write a test that is not flaky. I can pass INT_MAX > bytes > > > to this function, but that does not guarantee it will trigger overflow > > detection > > > and correction (depending on how long machine sleeps etc.). > > > > The bytes read we pass in is an int64. If we pass in 2.21 petabytes, > regardless > > of any skew in the time, that should be enough, no? > If 2.21 petabytes are received in 1 petaseconds, then there won;t be an > overflow. But yeah, 1 peta second is obviously impossible. So, added the test. > https://codereview.chromium.org/1164713004/diff/360001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1164713004/diff/360001/net/base/network_quali... net/base/network_quality_estimator.cc:78: double kbpsF = (cumulative_prefilter_bytes_read * 8.0) / nit: kbps_f https://codereview.chromium.org/1164713004/diff/360001/net/base/network_quali... net/base/network_quality_estimator.cc:79: (1000.0 * request_duration.InSecondsF()); nit: I Think .... / 1000.0 / request_duration.InSecondsF() is clearer. - the current grouping implies they're related somehow https://codereview.chromium.org/1164713004/diff/360001/net/base/network_quali... net/base/network_quality_estimator.cc:84: if (kbpsF >= INT32_MAX) I think "std::numeric_limits<int32_t>::max()" is preferred (And include <limits>) https://codereview.chromium.org/1164713004/diff/360001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1164713004/diff/360001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:147: estimator.NotifyDataReceived(*(request.get()), INT64_MAX, INT64_MAX); Here and below, again, I think numer_limits is preferred - at least it seems to be used more often in net/. https://codereview.chromium.org/1164713004/diff/360001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:147: estimator.NotifyDataReceived(*(request.get()), INT64_MAX, INT64_MAX); *(request.get()) -> *request
thanks, submitting now. https://codereview.chromium.org/1164713004/diff/360001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1164713004/diff/360001/net/base/network_quali... net/base/network_quality_estimator.cc:78: double kbpsF = (cumulative_prefilter_bytes_read * 8.0) / On 2015/06/12 20:31:41, mmenke wrote: > nit: kbps_f Done. https://codereview.chromium.org/1164713004/diff/360001/net/base/network_quali... net/base/network_quality_estimator.cc:79: (1000.0 * request_duration.InSecondsF()); On 2015/06/12 20:31:42, mmenke wrote: > nit: I Think .... / 1000.0 / request_duration.InSecondsF() is clearer. - the > current grouping implies they're related somehow Done. https://codereview.chromium.org/1164713004/diff/360001/net/base/network_quali... net/base/network_quality_estimator.cc:84: if (kbpsF >= INT32_MAX) On 2015/06/12 20:31:41, mmenke wrote: > I think "std::numeric_limits<int32_t>::max()" is preferred (And include > <limits>) Done. https://codereview.chromium.org/1164713004/diff/360001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1164713004/diff/360001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:147: estimator.NotifyDataReceived(*(request.get()), INT64_MAX, INT64_MAX); On 2015/06/12 20:31:42, mmenke wrote: > *(request.get()) -> *request Done. https://codereview.chromium.org/1164713004/diff/360001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:147: estimator.NotifyDataReceived(*(request.get()), INT64_MAX, INT64_MAX); On 2015/06/12 20:31:42, mmenke wrote: > Here and below, again, I think numer_limits is preferred - at least it seems to > be used more often in net/. Done. I thought stdint was the new hotness, so I used INT64_MAX directly from stdint.
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, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1164713004/#ps380001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1164713004/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
@mmenke: Seems like the linker on Windows does not like the inner classes. http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp... I made a few minor changes to expose member functions/objects of the inner class to the tests by plumbing the call through the outer class. Please let me know what you think. I might submit it with my changes (just to make sure that calling functions of inner class is indeed the cause of failure) but obviously will make any fixes that you might suggest.
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, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1164713004/#ps400001 (title: "Changes to make the tests work on Windows")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1164713004/400001
Message was sent while issue was closed.
Committed patchset #14 (id:400001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/6f840fc5d6482ef18945e501f729f8eb1c954f84 Cr-Commit-Position: refs/heads/master@{#334317} |
