Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2487883002:
NQE: Use cached estimates (Closed)
DescriptionNQE: Use cached estimates
Read cached estimates from the network quality estimator (NQE) prefs.
The read estimates are notified to observers if the network ID of the
cached estimates matches the ID of the current network. Both writing
and readings of prefs is guarded behind field trial.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
BUG=490870
Committed: https://crrev.com/7aa44df449065ce83400f2fa115f55636409ad04
Cr-Commit-Position: refs/heads/master@{#439526}
Patch Set 1 : ps #
Total comments: 12
Patch Set 2 : Rebased, ryansturm comments #
Total comments: 9
Patch Set 3 : bengr comments #
Total comments: 2
Patch Set 4 : Rebase, handle comment #
Messages
Total messages: 62 (48 generated)
Description was changed from ========== NQE: Use cached estimates BUG= ========== to ========== NQE: Use cached estimates CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG= ==========
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #1 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== NQE: Use cached estimates CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG= ========== to ========== NQE: Use cached estimates CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=490870 ==========
Description was changed from ========== NQE: Use cached estimates CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=490870 ========== to ========== NQE: Use cached estimates Read cached estimates from the network quality estimator (NQE) prefs. The read estimates are notified to observers if the cached estimates are found for the current network. Both writing and readings of prefs is guarded behind field trial. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=490870 ==========
Description was changed from ========== NQE: Use cached estimates Read cached estimates from the network quality estimator (NQE) prefs. The read estimates are notified to observers if the cached estimates are found for the current network. Both writing and readings of prefs is guarded behind field trial. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=490870 ========== to ========== NQE: Use cached estimates Read cached estimates from the network quality estimator (NQE) prefs. The read estimates are notified to observers if the network ID of the cached estimates matches the ID of the current network. Both writing and readings of prefs is guarded behind field trial. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=490870 ==========
tbansal@chromium.org changed reviewers: + ryansturm@chromium.org
ryansturm: ptal.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2487883002/diff/80001/chrome/browser/net/nqe/... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2487883002/diff/80001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:44: bool persistent_cache_writing_enabled() { Is there really a need for both of these field trial params? I feel like we should be able to either read and write or do neither. https://codereview.chromium.org/2487883002/diff/80001/net/nqe/network_qualiti... File net/nqe/network_qualities_prefs_manager.h (right): https://codereview.chromium.org/2487883002/diff/80001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.h:57: // Returns the peristent prefs. nit: s/the persistent/a copy of the persistent/ https://codereview.chromium.org/2487883002/diff/80001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2487883002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1396: NotifyObserversOfRTT(rtt_observation); Can you add a class comment to NotifyObserversOfRTT since it does more than just notify observers? https://codereview.chromium.org/2487883002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1611: nqe::internal::CachedNetworkQuality>::const_iterator it = nit: maybe use auto instead since the map is const, auto should give you a const_iterator anyway. https://codereview.chromium.org/2487883002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1638: network_quality_store_->Add(it->first, cached_network_quality); talked offline, consider using ListValue instead DictionaryValue for the prefs to keep LRU behavior in prefs instead of removing the smallest key value everytime. https://codereview.chromium.org/2487883002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1654: if (network_quality.downstream_throughput_kbps() != Can you add a comment referencing the bug to add typical throughput? So this conditional gets removed when you add that.
ryansturm: ptal. Thanks. https://codereview.chromium.org/2487883002/diff/80001/chrome/browser/net/nqe/... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2487883002/diff/80001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:44: bool persistent_cache_writing_enabled() { On 2016/12/13 21:08:35, Ryan Sturm wrote: > Is there really a need for both of these field trial params? I feel like we > should be able to either read and write or do neither. Yeah, I do not know of a better way. I want to enable writing for all users (ramp up gradually), and then enable reading for a subset of users and compare with Control. If I enabled writing and reading for a small subset of users, then it is possible that reading the prefs gives them very old prefs, which may be useless. https://codereview.chromium.org/2487883002/diff/80001/net/nqe/network_qualiti... File net/nqe/network_qualities_prefs_manager.h (right): https://codereview.chromium.org/2487883002/diff/80001/net/nqe/network_qualiti... net/nqe/network_qualities_prefs_manager.h:57: // Returns the peristent prefs. On 2016/12/13 21:08:35, Ryan Sturm wrote: > nit: s/the persistent/a copy of the persistent/ Done. https://codereview.chromium.org/2487883002/diff/80001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2487883002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1396: NotifyObserversOfRTT(rtt_observation); On 2016/12/13 21:08:35, Ryan Sturm wrote: > Can you add a class comment to NotifyObserversOfRTT since it does more than just > notify observers? Done. https://codereview.chromium.org/2487883002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1611: nqe::internal::CachedNetworkQuality>::const_iterator it = On 2016/12/13 21:08:35, Ryan Sturm wrote: > nit: maybe use auto instead since the map is const, auto should give you a > const_iterator anyway. Done. This looks much better. https://codereview.chromium.org/2487883002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1638: network_quality_store_->Add(it->first, cached_network_quality); On 2016/12/13 21:08:35, Ryan Sturm wrote: > talked offline, consider using ListValue instead DictionaryValue for the prefs > to keep LRU behavior in prefs instead of removing the smallest key value > everytime. I have changed the behavior in prefs manager to use random deletion. Not using ListValue for now. https://codereview.chromium.org/2487883002/diff/80001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1654: if (network_quality.downstream_throughput_kbps() != On 2016/12/13 21:08:35, Ryan Sturm wrote: > Can you add a comment referencing the bug to add typical throughput? So this > conditional gets removed when you add that. Done.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #2 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
tbansal@chromium.org changed reviewers: + bengr@chromium.org
bengr: ptal. thanks.
https://codereview.chromium.org/2487883002/diff/120001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc (right): https://codereview.chromium.org/2487883002/diff/120001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc:65: if (persistent_cache_writing_enabled) Why do these not also check that the value is "false" if not enabled? https://codereview.chromium.org/2487883002/diff/120001/net/nqe/network_qualit... File net/nqe/network_qualities_prefs_manager.cc (right): https://codereview.chromium.org/2487883002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:136: // Delete one randomly selected value that has key different than has key different than -> has a key that is different from https://codereview.chromium.org/2487883002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:144: for (base::DictionaryValue::Iterator it(*prefs_); !it.IsAtEnd(); How about adding this comment: // Delete the kth element in the dictionary, not including the element that represents the current network. k == |index_to_delete]. https://codereview.chromium.org/2487883002/diff/120001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2487883002/diff/120001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:1636: if (it.first == current_network_id_) { Can you add a helper called something like UpdateNetworkQualityFromCache() that includes this whole block?
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
bengr: ptal. Thanks. https://codereview.chromium.org/2487883002/diff/120001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc (right): https://codereview.chromium.org/2487883002/diff/120001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc:65: if (persistent_cache_writing_enabled) On 2016/12/15 23:51:26, bengr wrote: > Why do these not also check that the value is "false" if not enabled? I am not sure if I understand this question, but I will try. If caching is not enabled, then the variation params |persistent_cache_writing_enabled| or |persistent_cache_reading_enabled| would be unset. In that case, writing or reading of cache would be disabled since the code enables caching only if the finch params are set to true. https://codereview.chromium.org/2487883002/diff/120001/net/nqe/network_qualit... File net/nqe/network_qualities_prefs_manager.cc (right): https://codereview.chromium.org/2487883002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:136: // Delete one randomly selected value that has key different than On 2016/12/15 23:51:26, bengr wrote: > has key different than -> has a key that is different from Done. https://codereview.chromium.org/2487883002/diff/120001/net/nqe/network_qualit... net/nqe/network_qualities_prefs_manager.cc:144: for (base::DictionaryValue::Iterator it(*prefs_); !it.IsAtEnd(); On 2016/12/15 23:51:26, bengr wrote: > How about adding this comment: > > // Delete the kth element in the dictionary, not including the element that > represents the current network. k == |index_to_delete]. Done. https://codereview.chromium.org/2487883002/diff/120001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2487883002/diff/120001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:1636: if (it.first == current_network_id_) { On 2016/12/15 23:51:26, bengr wrote: > Can you add a helper called something like UpdateNetworkQualityFromCache() that > includes this whole block? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2487883002/diff/120001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc (right): https://codereview.chromium.org/2487883002/diff/120001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service_browsertest.cc:65: if (persistent_cache_writing_enabled) On 2016/12/16 18:07:15, tbansal1 wrote: > On 2016/12/15 23:51:26, bengr wrote: > > Why do these not also check that the value is "false" if not enabled? > > I am not sure if I understand this question, but I will try. > > If caching is not enabled, then the variation params > |persistent_cache_writing_enabled| or |persistent_cache_reading_enabled| would > be unset. In that case, writing or reading of cache would be disabled since the > code enables caching only if the finch params are set to true. Acknowledged. https://codereview.chromium.org/2487883002/diff/140001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2487883002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:1642: const nqe::internal::CachedNetworkQuality cached_network_quality) { Why isn't this a const ref?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2487883002/diff/140001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2487883002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:1642: const nqe::internal::CachedNetworkQuality cached_network_quality) { On 2016/12/16 20:33:43, bengr wrote: > Why isn't this a const ref? Done.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ryansturm@chromium.org, bengr@chromium.org Link to the patchset: https://codereview.chromium.org/2487883002/#ps160001 (title: "Rebase, handle comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by tbansal@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1482173376777730,
"parent_rev": "998fe02160841d962d495307cc6a3a906d5a652c", "commit_rev":
"216078e1b9175547877f40210c925fa2773aac6e"}
Message was sent while issue was closed.
Description was changed from ========== NQE: Use cached estimates Read cached estimates from the network quality estimator (NQE) prefs. The read estimates are notified to observers if the network ID of the cached estimates matches the ID of the current network. Both writing and readings of prefs is guarded behind field trial. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=490870 ========== to ========== NQE: Use cached estimates Read cached estimates from the network quality estimator (NQE) prefs. The read estimates are notified to observers if the network ID of the cached estimates matches the ID of the current network. Both writing and readings of prefs is guarded behind field trial. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=490870 Review-Url: https://codereview.chromium.org/2487883002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== NQE: Use cached estimates Read cached estimates from the network quality estimator (NQE) prefs. The read estimates are notified to observers if the network ID of the cached estimates matches the ID of the current network. Both writing and readings of prefs is guarded behind field trial. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=490870 Review-Url: https://codereview.chromium.org/2487883002 ========== to ========== NQE: Use cached estimates Read cached estimates from the network quality estimator (NQE) prefs. The read estimates are notified to observers if the network ID of the cached estimates matches the ID of the current network. Both writing and readings of prefs is guarded behind field trial. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=490870 Committed: https://crrev.com/7aa44df449065ce83400f2fa115f55636409ad04 Cr-Commit-Position: refs/heads/master@{#439526} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7aa44df449065ce83400f2fa115f55636409ad04 Cr-Commit-Position: refs/heads/master@{#439526} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
