Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2841273002:
Add UMA histograms for GPU driver bug workaround entries. (Closed)
DescriptionAdd UMA histograms for GPU driver bug workaround entries.
With this we can see how often different workarounds are being hit.
Review-Url: https://codereview.chromium.org/2841273002
Cr-Commit-Position: refs/heads/master@{#469542}
Committed: https://chromium.googlesource.com/chromium/src/+/2148f56018f190368c84a7562cb048e5e0a0e4f5
Patch Set 1 #
Total comments: 3
Patch Set 2 : fix nits #
Total comments: 3
Patch Set 3 : autogenerate enum #
Total comments: 4
Patch Set 4 : update script, include 0 entry #
Messages
Total messages: 30 (22 generated)
The CQ bit was checked by jbauman@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
jbauman@chromium.org changed reviewers: + zmo@chromium.org
lgtm with nits https://codereview.chromium.org/2841273002/diff/1/content/browser/gpu/gpu_dat... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2841273002/diff/1/content/browser/gpu/gpu_dat... content/browser/gpu/gpu_data_manager_impl_private.cc:213: const std::set<int>& blacklisted_features) { maybe rename blacklisted_features to workarounds or gpu_driver_bugs, so it won't cause confusion? https://codereview.chromium.org/2841273002/diff/1/content/browser/gpu/gpu_dat... content/browser/gpu/gpu_data_manager_impl_private.cc:216: // GPU Blacklist was not loaded. No need to go further. nit: Blacklist -> driver bug list https://codereview.chromium.org/2841273002/diff/1/content/browser/gpu/gpu_dat... content/browser/gpu/gpu_data_manager_impl_private.cc:222: // denominator to compute blacklist percentages for the rest of the nit: blacklist -> driver bug list
The CQ bit was checked by jbauman@chromium.org to run a CQ dry run
jbauman@chromium.org changed reviewers: + isherman@chromium.org
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2841273002/diff/20001/content/browser/gpu/gpu... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2841273002/diff/20001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:227: bug_list->GetDecisionEntries(&flag_entries); It's not entirely clear to me: Are the ids used in the bug list intended to be time-invariant identifiers? That is, is it already expected that the value "54" will always mean the same thing, no matter which version of Chrome is reporting the bug ids? If so, great. If not, you'll want to ensure that the ids recorded to histograms have consistent semantics over time -- either by making the bug list restrictions more rigid, or by providing some translation code to map to a separate set of rigid ids. https://codereview.chromium.org/2841273002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2841273002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22421: +<histogram name="GPU.DriverBugTestResultsPerEntry"> Please add an enum attribute for the values recorded to this histogram. https://codereview.chromium.org/2841273002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22428: + records the id. Entry 0 is the total number of times that data is recorded. Please document when this metric is recorded.
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 jbauman@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...
On 2017/05/04 21:17:38, Ilya Sherman wrote: > https://codereview.chromium.org/2841273002/diff/20001/content/browser/gpu/gpu... > File content/browser/gpu/gpu_data_manager_impl_private.cc (right): > > https://codereview.chromium.org/2841273002/diff/20001/content/browser/gpu/gpu... > content/browser/gpu/gpu_data_manager_impl_private.cc:227: > bug_list->GetDecisionEntries(&flag_entries); > It's not entirely clear to me: Are the ids used in the bug list intended to be > time-invariant identifiers? That is, is it already expected that the value "54" > will always mean the same thing, no matter which version of Chrome is reporting > the bug ids? If so, great. If not, you'll want to ensure that the ids recorded > to histograms have consistent semantics over time -- either by making the bug > list restrictions more rigid, or by providing some translation code to map to a > separate set of rigid ids. Yeah, they're intended to be consistent over time. > > https://codereview.chromium.org/2841273002/diff/20001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2841273002/diff/20001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:22421: +<histogram > name="GPU.DriverBugTestResultsPerEntry"> > Please add an enum attribute for the values recorded to this histogram. > Ok, added a script to generate the enum (and an initial set of entries). > https://codereview.chromium.org/2841273002/diff/20001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:22428: + records the id. Entry 0 is > the total number of times that data is recorded. > Please document when this metric is recorded. Ok, done.
Thanks. Metrics LGTM % remaining nits: https://codereview.chromium.org/2841273002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2841273002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:17212: + <int value="1" Could you please also include an entry for <int value="0">? https://codereview.chromium.org/2841273002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/update_gpu_driver_bug_workaround_entries.py (right): https://codereview.chromium.org/2841273002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/update_gpu_driver_bug_workaround_entries.py:21: nit: Please leave an extra blank line here. https://codereview.chromium.org/2841273002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/update_gpu_driver_bug_workaround_entries.py:23: nit: Please leave an extra blank line here. https://codereview.chromium.org/2841273002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/update_gpu_driver_bug_workaround_entries.py:48: nit: Please leave an extra blank line here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by jbauman@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 jbauman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2841273002/#ps60001 (title: "update script, include 0 entry")
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": 60001, "attempt_start_ts": 1493943212624470,
"parent_rev": "3029a52fe7b0f2c3a04536d33941011143bb1dc3", "commit_rev":
"2148f56018f190368c84a7562cb048e5e0a0e4f5"}
Message was sent while issue was closed.
Description was changed from ========== Add UMA histograms for GPU driver bug workaround entries. With this we can see how often different workarounds are being hit. ========== to ========== Add UMA histograms for GPU driver bug workaround entries. With this we can see how often different workarounds are being hit. Review-Url: https://codereview.chromium.org/2841273002 Cr-Commit-Position: refs/heads/master@{#469542} Committed: https://chromium.googlesource.com/chromium/src/+/2148f56018f190368c84a7562cb0... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/2148f56018f190368c84a7562cb0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
