Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(268)

Issue 108683003: Store gpu crashes in the systemprofileproto (Closed)

Created:
7 years ago by rkaplow
Modified:
4 years, 9 months ago
CC:
chromium-reviews, jar (doing other things), jam, joi+watch-content_chromium.org, darin-cc_chromium.org, asvitkine+watch_chromium.org, piman+watch_chromium.org, Ilya Sherman
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Store gpu crashes in the systemprofileproto as well as in the histogram. BUG=323250

Patch Set 1 #

Patch Set 2 : Extra comments #

Patch Set 3 : Extra comments #

Total comments: 20

Patch Set 4 : Alexei comments1 #

Total comments: 6

Patch Set 5 : ALexei comments 2 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+75 lines, -7 lines) Patch
M chrome/browser/metrics/metrics_log.cc View 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/metrics/metrics_log_unittest.cc View 1 2 3 1 chunk +12 lines, -0 lines 0 comments Download
M chrome/browser/metrics/metrics_service.h View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/metrics/metrics_service.cc View 1 2 3 4 5 chunks +12 lines, -0 lines 0 comments Download
M chrome/common/metrics/proto/system_profile.proto View 1 2 3 2 chunks +5 lines, -0 lines 1 comment Download
M chrome/common/pref_names.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/pref_names.cc View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M content/browser/gpu/gpu_process_host.cc View 3 chunks +5 lines, -2 lines 1 comment Download
M content/browser/gpu/gpu_process_host_ui_shim.h View 1 2 3 4 3 chunks +9 lines, -4 lines 0 comments Download
M content/browser/gpu/gpu_process_host_ui_shim.cc View 1 2 3 4 3 chunks +15 lines, -1 line 2 comments Download
M content/public/browser/notification_types.h View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
rkaplow
Getting comments from you before sending to GPU owners.
7 years ago (2013-12-13 22:20:30 UTC) #1
Alexei Svitkine (slow)
You should create a chromium bug and set it on the review via BUG=. Also, ...
7 years ago (2013-12-13 22:37:22 UTC) #2
rkaplow
Should also mention that I've already gotten a cl LGTM from log team for server ...
7 years ago (2013-12-13 23:54:03 UTC) #3
Alexei Svitkine (slow)
LGTM with a few more nits. https://codereview.chromium.org/108683003/diff/60001/chrome/browser/metrics/metrics_service.h File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/108683003/diff/60001/chrome/browser/metrics/metrics_service.h#newcode460 chrome/browser/metrics/metrics_service.h:460: // Call when ...
7 years ago (2013-12-16 16:04:27 UTC) #4
rkaplow
https://codereview.chromium.org/108683003/diff/60001/chrome/browser/metrics/metrics_service.h File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/108683003/diff/60001/chrome/browser/metrics/metrics_service.h#newcode460 chrome/browser/metrics/metrics_service.h:460: // Call when there is a crash in the ...
7 years ago (2013-12-16 16:39:53 UTC) #5
rkaplow
If I could get this reviewed for ownership that would be great.
7 years ago (2013-12-16 16:51:59 UTC) #6
piman
I'm not sure what's the idea behind routing this to the metrics_service, when we already ...
7 years ago (2013-12-16 22:12:04 UTC) #7
Ilya Sherman
https://codereview.chromium.org/108683003/diff/80001/chrome/common/metrics/proto/system_profile.proto File chrome/common/metrics/proto/system_profile.proto (right): https://codereview.chromium.org/108683003/diff/80001/chrome/common/metrics/proto/system_profile.proto#newcode404 chrome/common/metrics/proto/system_profile.proto:404: optional int32 gpu_crash_count = 23; nit: Hmm, I would ...
7 years ago (2013-12-16 23:04:39 UTC) #8
rkaplow
7 years ago (2013-12-18 16:28:42 UTC) #9
On 2013/12/16 23:04:39, Ilya Sherman wrote:
>
https://codereview.chromium.org/108683003/diff/80001/chrome/common/metrics/pr...
> File chrome/common/metrics/proto/system_profile.proto (right):
> 
>
https://codereview.chromium.org/108683003/diff/80001/chrome/common/metrics/pr...
> chrome/common/metrics/proto/system_profile.proto:404: optional int32
> gpu_crash_count = 23;
> nit: Hmm, I would probably move this to be grouped with the other crash
> counters, or else to be after item 22, if you're keen on trying to keep the
list
> in order.
> 
>
https://codereview.chromium.org/108683003/diff/80001/content/browser/gpu/gpu_...
> File content/browser/gpu/gpu_process_host_ui_shim.cc (right):
> 
>
https://codereview.chromium.org/108683003/diff/80001/content/browser/gpu/gpu_...
> content/browser/gpu/gpu_process_host_ui_shim.cc:118:
> content::NotificationService::current()->Notify(
> On 2013/12/16 22:12:04, piman wrote:
> > We're moving away from the Notification pattern, at least in content/ (see
> > discussions in chromium-dev). Can this be done with an Observer pattern, or
by
> > registering a Callback?
> 
> +1


Regarding piman's comment for why this is required. The original reason I was
working on this is I am changing some of the tools server side that would want
all the stability crash metrics in the same place. However after thinking about
it there is a pretty straightforward workaround - i.e. I can still get it from
the current histogram location, and just handle it a bit server side.

In that case the main reason for it is probably consistency, is it a type of
chrome crash and that analysts might expect for it to reside in this location.
It would make it easier for someone querying the logs direct to find the gpu
crashes if they were here.

However for my purposes it actually isn't as required as I thought and it's
actually not such a huge deal to me. Do people think the consistency of having
it also there is counterbalanced by the additional logging / code complexity?
I'm not against dropping if if people don't think it is worth it.

Powered by Google App Engine
This is Rietveld 408576698