Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1162293004:
Use request start time for estimating network quality. (Closed)
DescriptionUse request start time for estimating network quality.
Instead of using request creation time, use request start
time from load timing info. The start time is slightly
later than request creation time and is more accurate.
BUG=472681
Committed: https://crrev.com/afc2c1cb34daef6366349037a878be7a554b9dc6
Cr-Commit-Position: refs/heads/master@{#334499}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use response_time instead of now() #
Total comments: 8
Patch Set 3 : Addressed comments #
Total comments: 4
Patch Set 4 : Addressed bengr comments #Patch Set 5 : rebased #
Total comments: 4
Patch Set 6 : Return if load timing info is missing #Patch Set 7 : Fix flaky NQE tests #Patch Set 8 : More fixes to flaky tests #
Messages
Total messages: 34 (11 generated)
tbansal@chromium.org changed reviewers: + bengr@chromium.org
ptal.
https://codereview.chromium.org/1162293004/diff/1/net/base/network_quality_es... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1162293004/diff/1/net/base/network_quality_es... net/base/network_quality_estimator.cc:56: DCHECK_NE(load_timing_info.request_start, base::TimeTicks()); Do all requests have load timing info?
https://codereview.chromium.org/1162293004/diff/1/net/base/network_quality_es... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1162293004/diff/1/net/base/network_quality_es... net/base/network_quality_estimator.cc:56: DCHECK_NE(load_timing_info.request_start, base::TimeTicks()); On 2015/06/01 23:23:34, bengr wrote: > Do all requests have load timing info? Seems like yes. request_start is populated in URLRequest::Start() https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur... Also, we are reading request_start for only those requests that are HTTP and we have received the complete response header. So, request_start must be populated by this time. Also, URLRequest::ConvertRealLoadTimesToBlockingTimes() DCHECKS if request_start is populated. This is also called after the response headers are received.
ptal, now using timings stored in URLRequest instead of Load Timings (makes it a bit simpler). Also, using response start time, instead of base::Time::Now(). thanks.
how do i unsubscribe from this mailing list please help!!!! i am no longer part of this project! On Mon, Jun 1, 2015 at 6:32 PM, <tbansal@chromium.org> wrote: > Reviewers: bengr, > > Message: > ptal. > > Description: > Use request start time for estimating network quality. > > Instead of using request creation time, use request start > time from load timing info. The start time is slightly > later than request creation time and is more accurate. > > BUG=472681 > > Please review this at https://codereview.chromium.org/1162293004/ > > Base URL: https://chromium.googlesource.com/chromium/src.git@master > > Affected files (+7, -1 lines): > M net/base/network_quality_estimator.cc > > > Index: net/base/network_quality_estimator.cc > diff --git a/net/base/network_quality_estimator.cc > b/net/base/network_quality_estimator.cc > index > e012a99d72fdcc752cf4e9406f8e61651315598a..710703e11a2cf1cba2367c3927b5cf0a27e46a5b > 100644 > --- a/net/base/network_quality_estimator.cc > +++ b/net/base/network_quality_estimator.cc > @@ -7,6 +7,7 @@ > #include <string> > > #include "base/metrics/histogram.h" > +#include "net/base/load_timing_info.h" > #include "net/base/net_util.h" > #include "net/base/network_quality.h" > #include "net/url_request/url_request.h" > @@ -49,7 +50,12 @@ void NetworkQualityEstimator::NotifyDataReceived(const > URLRequest& request, > return; > > base::TimeTicks now = base::TimeTicks::Now(); > - base::TimeDelta request_duration = now - request.creation_time(); > + > + LoadTimingInfo load_timing_info; > + request.GetLoadTimingInfo(&load_timing_info); > + DCHECK_NE(load_timing_info.request_start, base::TimeTicks()); > + > + base::TimeDelta request_duration = now - load_timing_info.request_start; > DCHECK_GE(request_duration, base::TimeDelta()); > if (!bytes_read_since_last_connection_change_) > fastest_RTT_since_last_connection_change_ = request_duration; > > > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/06/05 19:53:13, carpediemishere wrote: > how do i unsubscribe from this mailing list please help!!!! > i am no longer part of this project! Did you try sending mail to chromium-reviews+unsubscribe@chromium.org from your chromium account? > > On Mon, Jun 1, 2015 at 6:32 PM, <mailto:tbansal@chromium.org> wrote: > > > Reviewers: bengr, > > > > Message: > > ptal. > > > > Description: > > Use request start time for estimating network quality. > > > > Instead of using request creation time, use request start > > time from load timing info. The start time is slightly > > later than request creation time and is more accurate. > > > > BUG=472681 > > > > Please review this at https://codereview.chromium.org/1162293004/ > > > > Base URL: https://chromium.googlesource.com/chromium/src.git@master > > > > Affected files (+7, -1 lines): > > M net/base/network_quality_estimator.cc > > > > > > Index: net/base/network_quality_estimator.cc > > diff --git a/net/base/network_quality_estimator.cc > > b/net/base/network_quality_estimator.cc > > index > > > e012a99d72fdcc752cf4e9406f8e61651315598a..710703e11a2cf1cba2367c3927b5cf0a27e46a5b > > 100644 > > --- a/net/base/network_quality_estimator.cc > > +++ b/net/base/network_quality_estimator.cc > > @@ -7,6 +7,7 @@ > > #include <string> > > > > #include "base/metrics/histogram.h" > > +#include "net/base/load_timing_info.h" > > #include "net/base/net_util.h" > > #include "net/base/network_quality.h" > > #include "net/url_request/url_request.h" > > @@ -49,7 +50,12 @@ void NetworkQualityEstimator::NotifyDataReceived(const > > URLRequest& request, > > return; > > > > base::TimeTicks now = base::TimeTicks::Now(); > > - base::TimeDelta request_duration = now - request.creation_time(); > > + > > + LoadTimingInfo load_timing_info; > > + request.GetLoadTimingInfo(&load_timing_info); > > + DCHECK_NE(load_timing_info.request_start, base::TimeTicks()); > > + > > + base::TimeDelta request_duration = now - load_timing_info.request_start; > > DCHECK_GE(request_duration, base::TimeDelta()); > > if (!bytes_read_since_last_connection_change_) > > fastest_RTT_since_last_connection_change_ = request_duration; > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > > email to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
tbansal@chromium.org changed reviewers: - carpediemishere@gmail.com
This no longer conforms to design. Chatted offline and it will be rewritten. https://codereview.chromium.org/1162293004/diff/20001/net/base/network_qualit... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1162293004/diff/20001/net/base/network_qualit... net/base/network_quality_estimator.cc:56: // Time when the resource was requested. Please add a comment to explain this logic. https://codereview.chromium.org/1162293004/diff/20001/net/base/network_qualit... net/base/network_quality_estimator.cc:61: // Difference of when the response headers were received and when the resource // Duration between when the resource was requested and when response headers were received. https://codereview.chromium.org/1162293004/diff/20001/net/base/network_qualit... net/base/network_quality_estimator.cc:63: base::TimeDelta estimated_rtt = Maybe rename this as observed_rtt? It would be good if our samples were called observations, and the estimates we report were called estimates. https://codereview.chromium.org/1162293004/diff/20001/net/base/network_qualit... net/base/network_quality_estimator.cc:64: request.response_info().response_time - request_start_time; Can response_time be null?
ptal. This does not confirm to our final design which specifies that the bandwidth should be computed after the request has finished. However, this CL still improves on the RTT estimation by using load timing information. In a forthcoming CL, I will put the calls so that NQE is notified after the request has finished. In the meantime, we should still go ahead with this CL. https://codereview.chromium.org/1162293004/diff/20001/net/base/network_qualit... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1162293004/diff/20001/net/base/network_qualit... net/base/network_quality_estimator.cc:56: // Time when the resource was requested. On 2015/06/09 00:18:49, bengr wrote: > Please add a comment to explain this logic. Done. https://codereview.chromium.org/1162293004/diff/20001/net/base/network_qualit... net/base/network_quality_estimator.cc:61: // Difference of when the response headers were received and when the resource On 2015/06/09 00:18:49, bengr wrote: > // Duration between when the resource was requested and when response headers > were received. Done. https://codereview.chromium.org/1162293004/diff/20001/net/base/network_qualit... net/base/network_quality_estimator.cc:63: base::TimeDelta estimated_rtt = On 2015/06/09 00:18:49, bengr wrote: > Maybe rename this as observed_rtt? It would be good if our samples were called > observations, and the estimates we report were called estimates. Done. https://codereview.chromium.org/1162293004/diff/20001/net/base/network_qualit... net/base/network_quality_estimator.cc:64: request.response_info().response_time - request_start_time; On 2015/06/09 00:18:49, bengr wrote: > Can response_time be null? No, there is a check above in the "if" condition: request.response_info().response_time.is_null() then, return
https://codereview.chromium.org/1162293004/diff/40001/net/base/network_qualit... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1162293004/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.cc:61: // if it is available since it is recorded after DNS is resolved and secure secure -> a secure https://codereview.chromium.org/1162293004/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.cc:68: // Time when the headers were received. Use timings from LoadTimingInfo, if Are they available at this point? When wouldn't they be?
ptal https://codereview.chromium.org/1162293004/diff/40001/net/base/network_qualit... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1162293004/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.cc:61: // if it is available since it is recorded after DNS is resolved and secure On 2015/06/11 00:22:58, bengr wrote: > secure -> a secure Done. https://codereview.chromium.org/1162293004/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.cc:68: // Time when the headers were received. Use timings from LoadTimingInfo, if On 2015/06/11 00:22:58, bengr wrote: > Are they available at this point? When wouldn't they be? I think they are MOSTLY available, except for one case (see https://code.google.com/p/chromium/codesearch#chromium/src/net/base/load_timi...). Also, AFAICT, there is no guarantee that these will always be available in the future for all cases. I have taken conservative approach to fallback on |request.creation_time| or |now|, both of which are guaranteed to be non-null.
ping.
lgtm
tbansal@chromium.org changed reviewers: + mmenke@chromium.org
rebased. @mmenke: ptal.
https://codereview.chromium.org/1162293004/diff/80001/net/base/network_qualit... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1162293004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:73: base::TimeTicks request_start_time = !load_timing_info.send_start.is_null() Hrm...If load_timing_info.send_start or load_timing_info.receive_headers_end is null, this request probably didn't go over the network (Or it was QUIC - not sure if those are hooked up for QUIC yet). May be better to just record nothing in those cases. https://codereview.chromium.org/1162293004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:89: DCHECK_GE(observed_rtt, base::TimeDelta()); This is only used in the body of the next if statement, so should move it down there. (If you decide to subtract half of this from since_request_start for better bandwidth measurement, of course, you'll need to move it back up here).
ptal. https://codereview.chromium.org/1162293004/diff/80001/net/base/network_qualit... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1162293004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:73: base::TimeTicks request_start_time = !load_timing_info.send_start.is_null() On 2015/06/15 17:21:00, mmenke wrote: > Hrm...If load_timing_info.send_start or load_timing_info.receive_headers_end is > null, this request probably didn't go over the network (Or it was QUIC - not > sure if those are hooked up for QUIC yet). > > May be better to just record nothing in those cases. QUIC still has these 2 values populated. I checked by adding some logging here: https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/quic_http... https://codereview.chromium.org/1162293004/diff/80001/net/base/network_qualit... net/base/network_quality_estimator.cc:89: DCHECK_GE(observed_rtt, base::TimeDelta()); On 2015/06/15 17:21:00, mmenke wrote: > This is only used in the body of the next if statement, so should move it down > there. (If you decide to subtract half of this from since_request_start for > better bandwidth measurement, of course, you'll need to move it back up here). Done.
LGTM
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org Link to the patchset: https://codereview.chromium.org/1162293004/#ps100001 (title: "Return if load timing info is missing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162293004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
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/1162293004/#ps120001 (title: "Fix flaky NQE tests")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162293004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
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/1162293004/#ps140001 (title: "More fixes to flaky tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162293004/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/afc2c1cb34daef6366349037a878be7a554b9dc6 Cr-Commit-Position: refs/heads/master@{#334499} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
