Throughput observations may have a very low value if there are too many hanging GETs |
||||||||||||
Issue descriptionReported in: https://bugs.chromium.org/p/chromium/issues/detail?id=368358#c44 Throughput observations may have a very low value if there are too many hanging GETs. This happens more frequently when there are too many hanging GET requests. e.g., for dogfood users, the median value of Kbps estimate is 80 kbps (http://shortn/_9r6ia8Pxxb). For non-dogfood users, the value is 5 Mbps (http://shortn/_8I4BLlzz5u). In my local tests, I saw that this mostly occurs when there are too many Google docs open which may issue xhr requests. In general, we should try to identify if there are hanging GETs in flight, and avoid taking a sample if there are hanging GETs in flight.
,
Oct 2 2017
I see. This makes sense, but Shaka Player won't be able to launch support for this API (https://github.com/google/shaka-player/issues/994) until this issue is resolved. The data coming out of Chrome would cause us to make worse initial decisions for video, not better. Can the priority be increased from P3?
,
Oct 2 2017
M63 is probably the earliest. If the fix is small, I might be able to cherry pick to M62. Thanks for the bug report btw.
,
Oct 2 2017
Thank you for looking into it! I really appreciate it.
,
Oct 3 2017
,
Oct 4 2017
,
Oct 4 2017
,
Oct 4 2017
,
Oct 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4ce57c9699512b34fae05218b51e272403e510ed commit 4ce57c9699512b34fae05218b51e272403e510ed Author: Tarun Bansal <tbansal@chromium.org> Date: Fri Oct 06 18:07:13 2017 Do not take throughput observation if a hanging request is in-flight. Add a heuristic algorithm to detect hanging requests (guarded behind experiment) in network quality estimator (NQE). If a hanging request is detected, then a throughput observation is not taken. Bug: 770897 Change-Id: Iddd4f32a2a0936d0532fb5b4859a7c1c72225dce Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester Reviewed-on: https://chromium-review.googlesource.com/698889 Reviewed-by: Ryan Sturm <ryansturm@chromium.org> Reviewed-by: Jesse Doherty <jwd@chromium.org> Reviewed-by: Matt Menke <mmenke@chromium.org> Commit-Queue: Tarun Bansal <tbansal@chromium.org> Cr-Commit-Position: refs/heads/master@{#507118} [modify] https://crrev.com/4ce57c9699512b34fae05218b51e272403e510ed/net/nqe/network_quality_estimator.cc [modify] https://crrev.com/4ce57c9699512b34fae05218b51e272403e510ed/net/nqe/network_quality_estimator.h [modify] https://crrev.com/4ce57c9699512b34fae05218b51e272403e510ed/net/nqe/network_quality_estimator_params.cc [modify] https://crrev.com/4ce57c9699512b34fae05218b51e272403e510ed/net/nqe/network_quality_estimator_params.h [modify] https://crrev.com/4ce57c9699512b34fae05218b51e272403e510ed/net/nqe/network_quality_provider.cc [modify] https://crrev.com/4ce57c9699512b34fae05218b51e272403e510ed/net/nqe/network_quality_provider.h [modify] https://crrev.com/4ce57c9699512b34fae05218b51e272403e510ed/net/nqe/throughput_analyzer.cc [modify] https://crrev.com/4ce57c9699512b34fae05218b51e272403e510ed/net/nqe/throughput_analyzer.h [modify] https://crrev.com/4ce57c9699512b34fae05218b51e272403e510ed/net/nqe/throughput_analyzer_unittest.cc [modify] https://crrev.com/4ce57c9699512b34fae05218b51e272403e510ed/net/url_request/url_request_job.cc [modify] https://crrev.com/4ce57c9699512b34fae05218b51e272403e510ed/testing/variations/fieldtrial_testing_config.json [modify] https://crrev.com/4ce57c9699512b34fae05218b51e272403e510ed/tools/metrics/histograms/histograms.xml
,
Oct 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b9633a112462aaccb40b90041089052e9d8e6961 commit b9633a112462aaccb40b90041089052e9d8e6961 Author: Ken Rockot <rockot@chromium.org> Date: Fri Oct 06 19:22:36 2017 Revert "Do not take throughput observation if a hanging request is in-flight." This reverts commit 4ce57c9699512b34fae05218b51e272403e510ed. Reason for revert: Breaks net_unittests at least on Mac 10.9. see https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.9%20Tests/builds/48502 Original change's description: > Do not take throughput observation if a hanging request is in-flight. > > Add a heuristic algorithm to detect hanging requests (guarded behind > experiment) in network quality estimator (NQE). If a hanging request > is detected, then a throughput observation is not taken. > > Bug: 770897 > Change-Id: Iddd4f32a2a0936d0532fb5b4859a7c1c72225dce > Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester > Reviewed-on: https://chromium-review.googlesource.com/698889 > Reviewed-by: Ryan Sturm <ryansturm@chromium.org> > Reviewed-by: Jesse Doherty <jwd@chromium.org> > Reviewed-by: Matt Menke <mmenke@chromium.org> > Commit-Queue: Tarun Bansal <tbansal@chromium.org> > Cr-Commit-Position: refs/heads/master@{#507118} TBR=jwd@chromium.org,mmenke@chromium.org,tbansal@chromium.org,ryansturm@chromium.org Change-Id: I3f360e74131d7f26b2ddcfbc50005f7bd55e3b61 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 770897 Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester Reviewed-on: https://chromium-review.googlesource.com/705875 Reviewed-by: Ken Rockot <rockot@chromium.org> Commit-Queue: Ken Rockot <rockot@chromium.org> Cr-Commit-Position: refs/heads/master@{#507138} [modify] https://crrev.com/b9633a112462aaccb40b90041089052e9d8e6961/net/nqe/network_quality_estimator.cc [modify] https://crrev.com/b9633a112462aaccb40b90041089052e9d8e6961/net/nqe/network_quality_estimator.h [modify] https://crrev.com/b9633a112462aaccb40b90041089052e9d8e6961/net/nqe/network_quality_estimator_params.cc [modify] https://crrev.com/b9633a112462aaccb40b90041089052e9d8e6961/net/nqe/network_quality_estimator_params.h [modify] https://crrev.com/b9633a112462aaccb40b90041089052e9d8e6961/net/nqe/network_quality_provider.cc [modify] https://crrev.com/b9633a112462aaccb40b90041089052e9d8e6961/net/nqe/network_quality_provider.h [modify] https://crrev.com/b9633a112462aaccb40b90041089052e9d8e6961/net/nqe/throughput_analyzer.cc [modify] https://crrev.com/b9633a112462aaccb40b90041089052e9d8e6961/net/nqe/throughput_analyzer.h [modify] https://crrev.com/b9633a112462aaccb40b90041089052e9d8e6961/net/nqe/throughput_analyzer_unittest.cc [modify] https://crrev.com/b9633a112462aaccb40b90041089052e9d8e6961/net/url_request/url_request_job.cc [modify] https://crrev.com/b9633a112462aaccb40b90041089052e9d8e6961/testing/variations/fieldtrial_testing_config.json [modify] https://crrev.com/b9633a112462aaccb40b90041089052e9d8e6961/tools/metrics/histograms/histograms.xml
,
Oct 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/45d9026ba97c596846a20943647bd1989e0ed70e commit 45d9026ba97c596846a20943647bd1989e0ed70e Author: Tarun Bansal <tbansal@chromium.org> Date: Mon Oct 09 19:43:35 2017 Do not take throughput observation if a hanging request is in-flight. Add a heuristic algorithm to detect hanging requests (guarded behind experiment) in network quality estimator (NQE). If a hanging request is detected, then a throughput observation is not taken. PS1 was reviewed on http://shortn/_Lw3hAoRNsF, and was later reverted. This is a reland of that CL. Bug: 770897 Change-Id: I91a32a1f7a7a965f5ae3d68938fcc8cdfff2e893 Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester TBR: ryansturm@chromium.org,jwd@chromium.org,mmenke@chromium.org Reviewed-on: https://chromium-review.googlesource.com/706070 Reviewed-by: Tarun Bansal <tbansal@chromium.org> Commit-Queue: Tarun Bansal <tbansal@chromium.org> Cr-Commit-Position: refs/heads/master@{#507451} [modify] https://crrev.com/45d9026ba97c596846a20943647bd1989e0ed70e/net/nqe/network_quality_estimator.cc [modify] https://crrev.com/45d9026ba97c596846a20943647bd1989e0ed70e/net/nqe/network_quality_estimator.h [modify] https://crrev.com/45d9026ba97c596846a20943647bd1989e0ed70e/net/nqe/network_quality_estimator_params.cc [modify] https://crrev.com/45d9026ba97c596846a20943647bd1989e0ed70e/net/nqe/network_quality_estimator_params.h [modify] https://crrev.com/45d9026ba97c596846a20943647bd1989e0ed70e/net/nqe/network_quality_provider.cc [modify] https://crrev.com/45d9026ba97c596846a20943647bd1989e0ed70e/net/nqe/network_quality_provider.h [modify] https://crrev.com/45d9026ba97c596846a20943647bd1989e0ed70e/net/nqe/throughput_analyzer.cc [modify] https://crrev.com/45d9026ba97c596846a20943647bd1989e0ed70e/net/nqe/throughput_analyzer.h [modify] https://crrev.com/45d9026ba97c596846a20943647bd1989e0ed70e/net/nqe/throughput_analyzer_unittest.cc [modify] https://crrev.com/45d9026ba97c596846a20943647bd1989e0ed70e/net/url_request/url_request_job.cc [modify] https://crrev.com/45d9026ba97c596846a20943647bd1989e0ed70e/testing/variations/fieldtrial_testing_config.json [modify] https://crrev.com/45d9026ba97c596846a20943647bd1989e0ed70e/tools/metrics/histograms/histograms.xml
,
Oct 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/87f8c490a18b724ec295a0ebbc3e5a749bbe66e7 commit 87f8c490a18b724ec295a0ebbc3e5a749bbe66e7 Author: Anthony Berent <aberent@chromium.org> Date: Tue Oct 10 10:19:08 2017 Revert "Do not take throughput observation if a hanging request is in-flight." This reverts commit 45d9026ba97c596846a20943647bd1989e0ed70e. Reason for revert: The same tests that previously failed on Mac are now failing or flaky on multiple Android bots. See the details in the bug BUG=773227 Original change's description: > Do not take throughput observation if a hanging request is in-flight. > > Add a heuristic algorithm to detect hanging requests (guarded behind > experiment) in network quality estimator (NQE). If a hanging request > is detected, then a throughput observation is not taken. > > PS1 was reviewed on http://shortn/_Lw3hAoRNsF, > and was later reverted. This is a reland of that CL. > > Bug: 770897 > Change-Id: I91a32a1f7a7a965f5ae3d68938fcc8cdfff2e893 > Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester > TBR: ryansturm@chromium.org,jwd@chromium.org,mmenke@chromium.org > Reviewed-on: https://chromium-review.googlesource.com/706070 > Reviewed-by: Tarun Bansal <tbansal@chromium.org> > Commit-Queue: Tarun Bansal <tbansal@chromium.org> > Cr-Commit-Position: refs/heads/master@{#507451} TBR=tbansal@chromium.org Change-Id: I960c4ee542268a1c2ed2bf048b85e84210ead5b0 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 770897 Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester Reviewed-on: https://chromium-review.googlesource.com/708734 Reviewed-by: Anthony Berent <aberent@chromium.org> Commit-Queue: Anthony Berent <aberent@chromium.org> Cr-Commit-Position: refs/heads/master@{#507628} [modify] https://crrev.com/87f8c490a18b724ec295a0ebbc3e5a749bbe66e7/net/nqe/network_quality_estimator.cc [modify] https://crrev.com/87f8c490a18b724ec295a0ebbc3e5a749bbe66e7/net/nqe/network_quality_estimator.h [modify] https://crrev.com/87f8c490a18b724ec295a0ebbc3e5a749bbe66e7/net/nqe/network_quality_estimator_params.cc [modify] https://crrev.com/87f8c490a18b724ec295a0ebbc3e5a749bbe66e7/net/nqe/network_quality_estimator_params.h [modify] https://crrev.com/87f8c490a18b724ec295a0ebbc3e5a749bbe66e7/net/nqe/network_quality_provider.cc [modify] https://crrev.com/87f8c490a18b724ec295a0ebbc3e5a749bbe66e7/net/nqe/network_quality_provider.h [modify] https://crrev.com/87f8c490a18b724ec295a0ebbc3e5a749bbe66e7/net/nqe/throughput_analyzer.cc [modify] https://crrev.com/87f8c490a18b724ec295a0ebbc3e5a749bbe66e7/net/nqe/throughput_analyzer.h [modify] https://crrev.com/87f8c490a18b724ec295a0ebbc3e5a749bbe66e7/net/nqe/throughput_analyzer_unittest.cc [modify] https://crrev.com/87f8c490a18b724ec295a0ebbc3e5a749bbe66e7/net/url_request/url_request_job.cc [modify] https://crrev.com/87f8c490a18b724ec295a0ebbc3e5a749bbe66e7/testing/variations/fieldtrial_testing_config.json [modify] https://crrev.com/87f8c490a18b724ec295a0ebbc3e5a749bbe66e7/tools/metrics/histograms/histograms.xml
,
Oct 10 2017
Issue 773227 has been merged into this issue.
,
Oct 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0094311340e722ef4680d5b33e94901017469923 commit 0094311340e722ef4680d5b33e94901017469923 Author: Tarun Bansal <tbansal@chromium.org> Date: Tue Oct 10 22:49:14 2017 Do not take throughput observation if a hanging request is in-flight. Add a heuristic algorithm to detect hanging requests (guarded behind experiment) in network quality estimator (NQE). If a hanging request is detected, then a throughput observation is not taken. PS1 was reviewed on http://shortn/_Lw3hAoRNsF, and was later reverted. This is a reland of that CL. Bug: 770897 Change-Id: Ib7fcbc29feb8fb9bf100bd4b22d4b6a168188f30 Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester TBR: ryansturm@chromium.org,jwd@chromium.org,mmenke@chromium.org Reviewed-on: https://chromium-review.googlesource.com/710220 Reviewed-by: Tarun Bansal <tbansal@chromium.org> Commit-Queue: Tarun Bansal <tbansal@chromium.org> Cr-Commit-Position: refs/heads/master@{#507803} [modify] https://crrev.com/0094311340e722ef4680d5b33e94901017469923/net/nqe/network_quality_estimator.cc [modify] https://crrev.com/0094311340e722ef4680d5b33e94901017469923/net/nqe/network_quality_estimator.h [modify] https://crrev.com/0094311340e722ef4680d5b33e94901017469923/net/nqe/network_quality_estimator_params.cc [modify] https://crrev.com/0094311340e722ef4680d5b33e94901017469923/net/nqe/network_quality_estimator_params.h [modify] https://crrev.com/0094311340e722ef4680d5b33e94901017469923/net/nqe/network_quality_provider.cc [modify] https://crrev.com/0094311340e722ef4680d5b33e94901017469923/net/nqe/network_quality_provider.h [modify] https://crrev.com/0094311340e722ef4680d5b33e94901017469923/net/nqe/throughput_analyzer.cc [modify] https://crrev.com/0094311340e722ef4680d5b33e94901017469923/net/nqe/throughput_analyzer.h [modify] https://crrev.com/0094311340e722ef4680d5b33e94901017469923/net/nqe/throughput_analyzer_unittest.cc [modify] https://crrev.com/0094311340e722ef4680d5b33e94901017469923/net/url_request/url_request_job.cc [modify] https://crrev.com/0094311340e722ef4680d5b33e94901017469923/testing/variations/fieldtrial_testing_config.json [modify] https://crrev.com/0094311340e722ef4680d5b33e94901017469923/tools/metrics/histograms/histograms.xml
,
Oct 26 2017
The experiment has been showing promise. The expected downlink estimate now shows up at 10 Mbps (http://shortn/_aoMm80WfBK) compared to ~200 kbps (http://shortn/_7rdF0mFzsp). I am going to enable this more widely.
,
Oct 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/de34299ae0335072042638c1f310cb200a643e97 commit de34299ae0335072042638c1f310cb200a643e97 Author: Tarun Bansal <tbansal@chromium.org> Date: Fri Oct 27 21:10:47 2017 Enable hanging requests detection in Network quality estimation Bug: 770897 Change-Id: I351a7990c7db4496af39b39fc62a501ff7b1db7c Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet Reviewed-on: https://chromium-review.googlesource.com/740915 Reviewed-by: Ryan Sturm <ryansturm@chromium.org> Commit-Queue: Tarun Bansal <tbansal@chromium.org> Cr-Commit-Position: refs/heads/master@{#512285} [modify] https://crrev.com/de34299ae0335072042638c1f310cb200a643e97/net/nqe/network_quality_estimator_params.cc
,
Nov 2 2017
Merging request for CL in #16. This CL is pretty safe, and enables a feature by default which involves the accuracy of the network quality estimator.
,
Nov 2 2017
This bug requires manual review: Reverts referenced in bugdroid comments after merge request. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Nov 2 2017
Pls apply appropriate OSs label.
,
Nov 2 2017
,
Nov 2 2017
Is the change listed at #16 well baked/verified in Canary/Dev?
,
Nov 2 2017
Re #21: Yes. 6+ days so far, and metrics look good. As a last resort, we also have a finch way to disable the logic in case something breaks.
,
Nov 2 2017
Approving merge to M63 branch 3239 based on comments #17 and #22. Please merge ASAP. Thank you.
,
Nov 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a40250969768cf7ec6f6d43d54781a507fb750da commit a40250969768cf7ec6f6d43d54781a507fb750da Author: Tarun Bansal <tbansal@chromium.org> Date: Thu Nov 02 21:49:18 2017 Enable hanging requests detection in Network quality estimation Bug: 770897 Change-Id: I351a7990c7db4496af39b39fc62a501ff7b1db7c Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet Reviewed-on: https://chromium-review.googlesource.com/740915 Reviewed-by: Ryan Sturm <ryansturm@chromium.org> Commit-Queue: Tarun Bansal <tbansal@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#512285}(cherry picked from commit de34299ae0335072042638c1f310cb200a643e97) Reviewed-on: https://chromium-review.googlesource.com/752204 Reviewed-by: Tarun Bansal <tbansal@chromium.org> Cr-Commit-Position: refs/branch-heads/3239@{#356} Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578} [modify] https://crrev.com/a40250969768cf7ec6f6d43d54781a507fb750da/net/nqe/network_quality_estimator_params.cc
,
Nov 2 2017
|
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by tbansal@chromium.org
, Oct 2 2017Labels: Restrict-View-Google M-63
Status: Started (was: Sta)