New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 764145 link

Starred by 1 user
Status: Fixed
Owner:
tbansal@chromium.org
Closed: Sep 2017
Cc:
█ abdulsyed@chromium.org
abdulsyed@google.com
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

Downstream throughput estimate may be set to a very low value due to hanging GETs

Project Member Reported by tbansal@chromium.org, Sep 12 2017 
The downstream throughput estimate exposed by the NetInfo API may sometimes be set to a very low value. This may happen if there is 1 request in flight, and that's a hanging GET request.

To fix this, NQE (Network Quality Estimator) should take a throughput sample only if there is at least 5 requests in flight.
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 12 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fc13d5985a0cd36561f862cc06eb22f97b53d831

commit fc13d5985a0cd36561f862cc06eb22f97b53d831
Author: Tarun Bansal <tbansal@chromium.org>
Date: Tue Sep 12 01:58:25 2017

NQE: Set min_requests for throughput computation to 5

Keep min payload size to be 32 KB, but make that a field trial param.

Also, move |use_small_responses_| to the NQE params class.

Bug:  764145 	
Change-Id: I8bfd37d602705367d294fff037a47ba72c93f2f7
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
Reviewed-on: https://chromium-review.googlesource.com/657202
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501153}
[modify] https://crrev.com/fc13d5985a0cd36561f862cc06eb22f97b53d831/net/nqe/network_quality_estimator.cc
[modify] https://crrev.com/fc13d5985a0cd36561f862cc06eb22f97b53d831/net/nqe/network_quality_estimator.h
[modify] https://crrev.com/fc13d5985a0cd36561f862cc06eb22f97b53d831/net/nqe/network_quality_estimator_params.cc
[modify] https://crrev.com/fc13d5985a0cd36561f862cc06eb22f97b53d831/net/nqe/network_quality_estimator_params.h
[modify] https://crrev.com/fc13d5985a0cd36561f862cc06eb22f97b53d831/net/nqe/network_quality_estimator_unittest.cc
[modify] https://crrev.com/fc13d5985a0cd36561f862cc06eb22f97b53d831/net/nqe/throughput_analyzer.cc
[modify] https://crrev.com/fc13d5985a0cd36561f862cc06eb22f97b53d831/net/nqe/throughput_analyzer.h
[modify] https://crrev.com/fc13d5985a0cd36561f862cc06eb22f97b53d831/net/nqe/throughput_analyzer_unittest.cc

Comment 2 by tbansal@chromium.org, Sep 12 2017

Labels: Merge-Request-62
Merging request for CL in #1. This is a pretty safe merge. Also, the CL makes it possible for us to control the exact param values using field trial configs, and so the parameters can be remotely changed.

Comment 3 by tbansal@chromium.org, Sep 14 2017

Cc: abdulsyed@chromium.org
abdulsyed: ping for the merge request. Thanks.

Comment 4 by tbansal@chromium.org, Sep 14 2017

Cc: abdulsyed@google.com
abdulsyed: ping for the merge request. Thanks.
Project Member

Comment 5 by sheriffbot@chromium.org, Sep 14 2017

Labels: -Merge-Request-62 Merge-Review-62 Hotlist-Merge-Review
This bug requires manual review: M62 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 6 by abdulsyed@google.com, Sep 14 2017

Labels: -Merge-Review-62 Merge-Approved-62
Thanks for the details. Approving merge for M62. Branch:3202
Project Member

Comment 7 by bugdroid1@chromium.org, Sep 14 2017

Labels: -merge-approved-62 merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fa7ad195a4dcb2caf80ec40015051e36e4640373

commit fa7ad195a4dcb2caf80ec40015051e36e4640373
Author: Tarun Bansal <tbansal@chromium.org>
Date: Thu Sep 14 18:46:44 2017

NQE: Set min_requests for throughput computation to 5

Keep min payload size to be 32 KB, but make that a field trial param.

Also, move |use_small_responses_| to the NQE params class.

Bug:  764145 	
Change-Id: I8bfd37d602705367d294fff037a47ba72c93f2f7
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
Reviewed-on: https://chromium-review.googlesource.com/657202
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#501153}(cherry picked from commit fc13d5985a0cd36561f862cc06eb22f97b53d831)
Reviewed-on: https://chromium-review.googlesource.com/667127
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#228}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/fa7ad195a4dcb2caf80ec40015051e36e4640373/net/nqe/network_quality_estimator.cc
[modify] https://crrev.com/fa7ad195a4dcb2caf80ec40015051e36e4640373/net/nqe/network_quality_estimator.h
[modify] https://crrev.com/fa7ad195a4dcb2caf80ec40015051e36e4640373/net/nqe/network_quality_estimator_params.cc
[modify] https://crrev.com/fa7ad195a4dcb2caf80ec40015051e36e4640373/net/nqe/network_quality_estimator_params.h
[modify] https://crrev.com/fa7ad195a4dcb2caf80ec40015051e36e4640373/net/nqe/network_quality_estimator_unittest.cc
[modify] https://crrev.com/fa7ad195a4dcb2caf80ec40015051e36e4640373/net/nqe/throughput_analyzer.cc
[modify] https://crrev.com/fa7ad195a4dcb2caf80ec40015051e36e4640373/net/nqe/throughput_analyzer.h
[modify] https://crrev.com/fa7ad195a4dcb2caf80ec40015051e36e4640373/net/nqe/throughput_analyzer_unittest.cc

Comment 8 by tbansal@chromium.org, Sep 14 2017

Status: Fixed (was: Started)
►

Sign in to add a comment