Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2308763002:
Integrate Crashpad UMA (Closed)
DescriptionIntegrate Crashpad UMA
Creates an global persistent metrics storage helper in base/metrics to
allow stubbing in Crashpad when built outside of Chrome.
Uses a pair of persistent metrics files, one that is the currently
active one, which is renamed to a "previous run" on startup. In normal
operation -active will be created, used, and removed on shutdown.
However, if Crashpad itself crashes, then on next startup, -active will
be moved to previous so that previous can be slurped in on startup to
retrieve data from the crash.
BUG=crashpad:100
Committed: https://crrev.com/785ed6054d704b40cf4be1da82a88c979efe03bb
Cr-Commit-Position: refs/heads/master@{#419251}
Patch Set 1 #Patch Set 2 : . #
Total comments: 12
Patch Set 3 : . #Patch Set 4 : . #
Total comments: 5
Patch Set 5 : pass through user data dir for metrics #
Total comments: 15
Patch Set 6 : . #Patch Set 7 : . #
Total comments: 6
Patch Set 8 : . #Patch Set 9 : . #
Total comments: 10
Patch Set 10 : . #
Total comments: 2
Patch Set 11 : oops #
Total comments: 21
Patch Set 12 : fixes #
Total comments: 2
Patch Set 13 : . #Patch Set 14 : logging for integration test failures #Patch Set 15 : more logging #Patch Set 16 : logging 2 #Patch Set 17 : logging 3 #Patch Set 18 : . #Messages
Total messages: 101 (59 generated)
Description was changed from ========== Integrate Crashpad UMA BUG=crashpad:100 ========== to ========== Integrate Crashpad UMA BUG=crashpad:100 ==========
scottmg@chromium.org changed reviewers: + bcwhite@chromium.org, mark@chromium.org
I haven't figured out how to stub this out when not building against //base yet, but I figured it made more sense to review the actual functionality on the Chrome side before worrying about how to do so when building in standalone Crashpad. Once we're happy with the functionality here, I'll move the third_party/ part over to the Crashpad repo and figure out how to stub, and then just land the chrome/ part here. https://codereview.chromium.org/2308763002/diff/20001/third_party/crashpad/cr... File third_party/crashpad/crashpad/handler/handler_main.cc (right): https://codereview.chromium.org/2308763002/diff/20001/third_party/crashpad/cr... third_party/crashpad/crashpad/handler/handler_main.cc:359: std::unique_ptr<CrashReportDatabase> database(CrashReportDatabase::Initialize( This is moved earlier so that the database directory will be created on the first run, as the memory mapped file is stored there.
Brian, could you take a look at the persistent metrics usage code in third_party/crashpad/crashpad/handler/handler_main.cc? I was thinking the easiest way to integrate this into Crashpad is probably to have a utility function in base that does a parameterized version of InitializePersistentMetricsStorage(), which can then easily be a no-op when Crashpad is built outside of Chrome.
https://codereview.chromium.org/2308763002/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2308763002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:131: void RegisterInstallerFileMetricsPreferences(PrefRegistrySimple* registry) { How about changing the name of this function while you're here... RegisterFileMetricsPreferences https://codereview.chromium.org/2308763002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:140: metrics::FileMetricsProvider::RegisterPrefs(registry, Move above the windows-specific stuff. https://codereview.chromium.org/2308763002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:145: CreateInstallerFileMetricsProvider(bool metrics_reporting_enabled) { How about changing the name of this function while you're here... CreateFileMetricsProvider https://codereview.chromium.org/2308763002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:193: // Read the Crashpad metrics file. Move above the windows-specific stuff. https://codereview.chromium.org/2308763002/diff/20001/third_party/crashpad/cr... File third_party/crashpad/crashpad/handler/handler_main.cc (right): https://codereview.chromium.org/2308763002/diff/20001/third_party/crashpad/cr... third_party/crashpad/crashpad/handler/handler_main.cc:149: base::GlobalHistogramAllocator::CreateWithFile( This file needs to be deleted when the browser exits cleanly. Previous-run metrics increase startup time so we definitely don't want to do it if there is nothing to report.
https://codereview.chromium.org/2308763002/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2308763002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:131: void RegisterInstallerFileMetricsPreferences(PrefRegistrySimple* registry) { On 2016/09/13 14:27:48, bcwhite wrote: > How about changing the name of this function while you're here... > > RegisterFileMetricsPreferences Done. https://codereview.chromium.org/2308763002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:140: metrics::FileMetricsProvider::RegisterPrefs(registry, On 2016/09/13 14:27:48, bcwhite wrote: > Move above the windows-specific stuff. Done. https://codereview.chromium.org/2308763002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:145: CreateInstallerFileMetricsProvider(bool metrics_reporting_enabled) { On 2016/09/13 14:27:48, bcwhite wrote: > How about changing the name of this function while you're here... > > CreateFileMetricsProvider Done. https://codereview.chromium.org/2308763002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:193: // Read the Crashpad metrics file. On 2016/09/13 14:27:48, bcwhite wrote: > Move above the windows-specific stuff. Done. https://codereview.chromium.org/2308763002/diff/20001/third_party/crashpad/cr... File third_party/crashpad/crashpad/handler/handler_main.cc (right): https://codereview.chromium.org/2308763002/diff/20001/third_party/crashpad/cr... third_party/crashpad/crashpad/handler/handler_main.cc:149: base::GlobalHistogramAllocator::CreateWithFile( On 2016/09/13 14:27:48, bcwhite wrote: > This file needs to be deleted when the browser exits cleanly. > > Previous-run metrics increase startup time so we definitely don't want to do it > if there is nothing to report. Ah, I didn't understand the reporting model. So on a clean exit the -active file just needs to be deleted, but all the data will have been sent to the server already, so there's no need to do anything like RegisterSource()?
https://codereview.chromium.org/2308763002/diff/20001/third_party/crashpad/cr... File third_party/crashpad/crashpad/handler/handler_main.cc (right): https://codereview.chromium.org/2308763002/diff/20001/third_party/crashpad/cr... third_party/crashpad/crashpad/handler/handler_main.cc:149: base::GlobalHistogramAllocator::CreateWithFile( On 2016/09/13 19:15:32, scottmg wrote: > On 2016/09/13 14:27:48, bcwhite wrote: > > This file needs to be deleted when the browser exits cleanly. > > > > Previous-run metrics increase startup time so we definitely don't want to do > it > > if there is nothing to report. > > Ah, I didn't understand the reporting model. So on a clean exit the -active file > just needs to be deleted, but all the data will have been sent to the server > already, so there's no need to do anything like RegisterSource()? The model you've selected is that "CrashpadMetrics" will be sent only once and that is when the browser _next_ starts up, associating those metrics with the previous run. It must be registered for that to happen. You can simply delete the "-active" file upon clean exit and report nothing. If there are things to report during the _current_ session, then you can also register the "-active" file with SOURCE_HISTOGRAMS_ACTIVE_FILE and ASSOCIATE_CURRENT_RUN. Metrics from that file will be (efficiently) included during every reporting cycle. There will be no duplicated reporting of results even during the next startup because each file knows what has already been reported from it. But you should still delete the "-active" file when you know there is nothing additional to report.
Patchset #4 (id:60001) has been deleted
On 2016/09/13 19:38:24, bcwhite wrote: > https://codereview.chromium.org/2308763002/diff/20001/third_party/crashpad/cr... > File third_party/crashpad/crashpad/handler/handler_main.cc (right): > > https://codereview.chromium.org/2308763002/diff/20001/third_party/crashpad/cr... > third_party/crashpad/crashpad/handler/handler_main.cc:149: > base::GlobalHistogramAllocator::CreateWithFile( > On 2016/09/13 19:15:32, scottmg wrote: > > On 2016/09/13 14:27:48, bcwhite wrote: > > > This file needs to be deleted when the browser exits cleanly. > > > > > > Previous-run metrics increase startup time so we definitely don't want to do > > it > > > if there is nothing to report. > > > > Ah, I didn't understand the reporting model. So on a clean exit the -active > file > > just needs to be deleted, but all the data will have been sent to the server > > already, so there's no need to do anything like RegisterSource()? > > The model you've selected is that "CrashpadMetrics" will be sent only once and > that is when the browser _next_ starts up, associating those metrics with the > previous run. It must be registered for that to happen. > > You can simply delete the "-active" file upon clean exit and report nothing. > > If there are things to report during the _current_ session, then you can also > register the "-active" file with SOURCE_HISTOGRAMS_ACTIVE_FILE and > ASSOCIATE_CURRENT_RUN. Metrics from that file will be (efficiently) included > during every reporting cycle. There will be no duplicated reporting of results > even during the next startup because each file knows what has already been > reported from it. > > But you should still delete the "-active" file when you know there is nothing > additional to report. Updated to work this way. Maybe we should have just gone with assuming a clean-exit, but anyway, hopefully this will be useful for Crashpad crashes. I also refactored to pull the code that was in handler_main.cc out into a helper file (base/metrics/persistent_metrics_file_util) so that it's easily stubbable in mini_chromium. If you could both take a look now, that'd be great. Thanks!
LGTM Are we concerned about the lag in getting reports back from the field? We won’t get anything from a session until the next browser restart, right? Is there another way? https://codereview.chromium.org/2308763002/diff/80001/base/metrics/persistent... File base/metrics/persistent_metrics_file_util.cc (right): https://codereview.chromium.org/2308763002/diff/80001/base/metrics/persistent... base/metrics/persistent_metrics_file_util.cc:62: base::File file(path, base::File::FLAG_OPEN | base::File::FLAG_READ | Ha, there’s a river here at column 24!
https://codereview.chromium.org/2308763002/diff/80001/third_party/crashpad/cr... File third_party/crashpad/crashpad/handler/handler_main.cc (right): https://codereview.chromium.org/2308763002/diff/80001/third_party/crashpad/cr... third_party/crashpad/crashpad/handler/handler_main.cc:325: ToolSupport::CommandLineArgumentToFilePathStringType(options.database)); Hmm, this will delay the handshake with the browser, and the browser’s in turn blocked on that.
Description was changed from ========== Integrate Crashpad UMA BUG=crashpad:100 ========== to ========== Integrate Crashpad UMA Creates an global persistent metrics storage helper in base/metrics to allow stubbing in Crashpad when built outside of Chrome. Uses a pair of persistent metrics files, one that is the currently active one, which is renamed to a "previous run" on startup. In normal operation -active will be created, used, and removed on shutdown. However, if Crashpad itself crashes, then on next startup, -active will be moved to previous so that previous can be slurped in on startup to retrieve data from the crash. BUG=crashpad:100 ==========
On 2016/09/13 21:34:06, Mark Mentovai wrote: > LGTM > > Are we concerned about the lag in getting reports back from the field? We won’t > get anything from a session until the next browser restart, right? Is there > another way? I had the same confusion in ps#2, but how it actually works is that normally -active is created, used, and deleted on shutdown (and so data should be coming in relatively quickly). The non-active name is only used in the case that Crashpad crashes, where it's used as a temporary storage location for the data that was in -active from last time, which is then passed to the metrics system to be associated with the previous run and then soon deleted (and -active is used again for the now-current run). I updated the cl desc to try to explain. > > https://codereview.chromium.org/2308763002/diff/80001/base/metrics/persistent... > File base/metrics/persistent_metrics_file_util.cc (right): > > https://codereview.chromium.org/2308763002/diff/80001/base/metrics/persistent... > base/metrics/persistent_metrics_file_util.cc:62: base::File file(path, > base::File::FLAG_OPEN | base::File::FLAG_READ | > Ha, there’s a river here at column 24!
https://codereview.chromium.org/2308763002/diff/80001/base/metrics/persistent... File base/metrics/persistent_metrics_file_util.cc (right): https://codereview.chromium.org/2308763002/diff/80001/base/metrics/persistent... base/metrics/persistent_metrics_file_util.cc:62: base::File file(path, base::File::FLAG_OPEN | base::File::FLAG_READ | On 2016/09/13 21:34:06, Mark Mentovai wrote: > Ha, there’s a river here at column 24! Yeah, I was surprised by that too, but I guess clang-format is trying to avoid it looking like a 3rd argument.
https://codereview.chromium.org/2308763002/diff/80001/third_party/crashpad/cr... File third_party/crashpad/crashpad/handler/handler_main.cc (right): https://codereview.chromium.org/2308763002/diff/80001/third_party/crashpad/cr... third_party/crashpad/crashpad/handler/handler_main.cc:325: ToolSupport::CommandLineArgumentToFilePathStringType(options.database)); On 2016/09/13 21:35:56, Mark Mentovai wrote: > Hmm, this will delay the handshake with the browser, and the browser’s in turn > blocked on that. It will. :( It "only" creates the directory if it doesn't exist and sets a couple in-memory variables, but on first run startup we have the cost of creating that directory. All we need is a place to put the file where the browser can find them -- I guess we should just put it in the User Data dir directly instead of inside the Crashpad subdirectory since we can rely on the installer to have created that one for us?
On 2016/09/13 21:46:24, scottmg wrote: > https://codereview.chromium.org/2308763002/diff/80001/third_party/crashpad/cr... > File third_party/crashpad/crashpad/handler/handler_main.cc (right): > > https://codereview.chromium.org/2308763002/diff/80001/third_party/crashpad/cr... > third_party/crashpad/crashpad/handler/handler_main.cc:325: > ToolSupport::CommandLineArgumentToFilePathStringType(options.database)); > On 2016/09/13 21:35:56, Mark Mentovai wrote: > > Hmm, this will delay the handshake with the browser, and the browser’s in turn > > blocked on that. > > It will. :( It "only" creates the directory if it doesn't exist and sets a > couple in-memory variables, but on first run startup we have the cost of > creating that directory. All we need is a place to put the file where the > browser can find them -- I guess we should just put it in the User Data dir > directly instead of inside the Crashpad subdirectory since we can rely on the > installer to have created that one for us? I guess that makes sense -- I'll also have to pass it into crashpad_handler as a useless-except-in-Chrome --metrics command line argument. Which starts to get a bit ugly, but I guess no moreso than hardcoding it to be inside the database directory.
https://codereview.chromium.org/2308763002/diff/80001/third_party/crashpad/cr... File third_party/crashpad/crashpad/handler/handler_main.cc (right): https://codereview.chromium.org/2308763002/diff/80001/third_party/crashpad/cr... third_party/crashpad/crashpad/handler/handler_main.cc:325: ToolSupport::CommandLineArgumentToFilePathStringType(options.database)); On 2016/09/13 21:46:24, scottmg wrote: > On 2016/09/13 21:35:56, Mark Mentovai wrote: > > Hmm, this will delay the handshake with the browser, and the browser’s in turn > > blocked on that. > > It will. :( It "only" creates the directory if it doesn't exist and sets a > couple in-memory variables, but on first run startup we have the cost of > creating that directory. All we need is a place to put the file where the > browser can find them -- I guess we should just put it in the User Data dir > directly instead of inside the Crashpad subdirectory since we can rely on the > installer to have created that one for us? Well, crashpad_handler won’t run at all unless the parent of database_dir already exists. We’d hit the EXIT_FAILURE just above in that case. But now that you bring this up, there’s another concern: we’ve never promised that options.database is a directory. Some future database implementation might stick a file there instead. I don’t think we should litter an arbitrary database implementation’s innards with files it’s not expecting, either. So there’s another argument in favor of just the user-data-dir.
On 2016/09/13 21:52:01, Mark Mentovai wrote: > https://codereview.chromium.org/2308763002/diff/80001/third_party/crashpad/cr... > File third_party/crashpad/crashpad/handler/handler_main.cc (right): > > https://codereview.chromium.org/2308763002/diff/80001/third_party/crashpad/cr... > third_party/crashpad/crashpad/handler/handler_main.cc:325: > ToolSupport::CommandLineArgumentToFilePathStringType(options.database)); > On 2016/09/13 21:46:24, scottmg wrote: > > On 2016/09/13 21:35:56, Mark Mentovai wrote: > > > Hmm, this will delay the handshake with the browser, and the browser’s in > turn > > > blocked on that. > > > > It will. :( It "only" creates the directory if it doesn't exist and sets a > > couple in-memory variables, but on first run startup we have the cost of > > creating that directory. All we need is a place to put the file where the > > browser can find them -- I guess we should just put it in the User Data dir > > directly instead of inside the Crashpad subdirectory since we can rely on the > > installer to have created that one for us? > > Well, crashpad_handler won’t run at all unless the parent of database_dir > already exists. We’d hit the EXIT_FAILURE just above in that case. > > But now that you bring this up, there’s another concern: we’ve never promised > that options.database is a directory. Some future database implementation might > stick a file there instead. I don’t think we should litter an arbitrary database > implementation’s innards with files it’s not expecting, either. > > So there’s another argument in favor of just the user-data-dir. Got icky, but passing through DIR_USER_DATA now. We're still going to be delaying browser connection in order to initialize the metrics files, even though we're not waiting for the Crashpad database dir creation now. I'm not sure if we should defer that initialization to later. I guess the only side effect of that is that UMA would be lost for anything until then. (? assuming UMA won't just crash if it's not initialized) In which case, maybe what I *should* have done is instead moved metrics initialization down to where the database is initialized. We'd miss a bit of UMA, but in practice really only the constructor of ExceptionHandlerServer. Any strong feelings?
Publishing my drafts doesn't show my comments in the same order as the files given for review (.cc files come first in the log but after .h in the file list). Apologies if it seems confusing. https://codereview.chromium.org/2308763002/diff/100001/base/metrics/persisten... File base/metrics/persistent_metrics_file_util.cc (right): https://codereview.chromium.org/2308763002/diff/100001/base/metrics/persisten... base/metrics/persistent_metrics_file_util.cc:44: allocator->CreateTrackingHistograms(metrics_name); This is the only line cannot be moved into CreateWithActiveFile() as it isn't always the creating process that manages those histograms but at least it's only one external line of code. Feel free change the CreateWith*() methods to return true/false indicating success. That'll simplify the "did it work" check before calling CreateTrackingHitograms(). https://codereview.chromium.org/2308763002/diff/100001/base/metrics/persisten... base/metrics/persistent_metrics_file_util.cc:45: allocator->SetPersistentLocation(active_file); I think this could be part of CreateWithFile(). https://codereview.chromium.org/2308763002/diff/100001/base/metrics/persisten... File base/metrics/persistent_metrics_file_util.h (right): https://codereview.chromium.org/2308763002/diff/100001/base/metrics/persisten... base/metrics/persistent_metrics_file_util.h:5: #ifndef BASE_METRICS_PERSISTENT_METRICS_FILE_UTIL_H_ Don't we have some policy against "*_util" files? You could put this as CreateWithActiveFile(base_path, active_path, size, id, name) under GlobalHistogramAllocator in persistent_histogram_allocator.{h,cc}. An addition CreateWithActiveFile(dir, size, id, name) that calls the former with constructed paths would also be useful. A ConstructFilePaths(dir, name, out_base_path, out_active_path) would also simplify the code both here and during registration. It would concatenate everything and add the standard extension. Pass nullptr for out_* if not interested in that path. https://codereview.chromium.org/2308763002/diff/100001/base/metrics/persisten... base/metrics/persistent_metrics_file_util.h:30: BASE_EXPORT void CleanUpGlobalPersistentHistogramStorage(); There's a WriteToPersistentLocation() in that class so perhaps DeletePersistentLocation() would be fitting. https://codereview.chromium.org/2308763002/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2308763002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:210: task_runner->PostTask( Comment: This file must be deleted here so it isn't mistakenly associated with an incorrect instance of the browser by a later run. https://codereview.chromium.org/2308763002/diff/100001/components/crash/conte... File components/crash/content/app/crash_reporter_client.h (right): https://codereview.chromium.org/2308763002/diff/100001/components/crash/conte... components/crash/content/app/crash_reporter_client.h:133: #if defined(OS_WIN) Why can't Windows use FilePath?
Thanks Brian. https://codereview.chromium.org/2308763002/diff/100001/base/metrics/persisten... File base/metrics/persistent_metrics_file_util.cc (right): https://codereview.chromium.org/2308763002/diff/100001/base/metrics/persisten... base/metrics/persistent_metrics_file_util.cc:44: allocator->CreateTrackingHistograms(metrics_name); On 2016/09/14 13:16:12, bcwhite wrote: > This is the only line cannot be moved into CreateWithActiveFile() as it isn't > always the creating process that manages those histograms but at least it's only > one external line of code. > > Feel free change the CreateWith*() methods to return true/false indicating > success. That'll simplify the "did it work" check before calling > CreateTrackingHitograms(). Done. https://codereview.chromium.org/2308763002/diff/100001/base/metrics/persisten... base/metrics/persistent_metrics_file_util.cc:45: allocator->SetPersistentLocation(active_file); On 2016/09/14 13:16:12, bcwhite wrote: > I think this could be part of CreateWithFile(). Done. I removed it from chrome/browser/chrome_browser_field_trials.cc too, which looks like a slight behaviour change (for the "LocalMemory" case) but that seemed like it probably had no effect anyway (?). https://codereview.chromium.org/2308763002/diff/100001/base/metrics/persisten... File base/metrics/persistent_metrics_file_util.h (right): https://codereview.chromium.org/2308763002/diff/100001/base/metrics/persisten... base/metrics/persistent_metrics_file_util.h:5: #ifndef BASE_METRICS_PERSISTENT_METRICS_FILE_UTIL_H_ On 2016/09/14 13:16:12, bcwhite wrote: > Don't we have some policy against "*_util" files? > > You could put this as CreateWithActiveFile(base_path, active_path, size, id, > name) under GlobalHistogramAllocator in persistent_histogram_allocator.{h,cc}. > An addition CreateWithActiveFile(dir, size, id, name) that calls the former with > constructed paths would also be useful. > > A ConstructFilePaths(dir, name, out_base_path, out_active_path) would also > simplify the code both here and during registration. It would concatenate > everything and add the standard extension. Pass nullptr for out_* if not > interested in that path. Done, much better! It'll be slightly more to stub in mini_chromium, but should be fine. https://codereview.chromium.org/2308763002/diff/100001/base/metrics/persisten... base/metrics/persistent_metrics_file_util.h:30: BASE_EXPORT void CleanUpGlobalPersistentHistogramStorage(); On 2016/09/14 13:16:12, bcwhite wrote: > There's a WriteToPersistentLocation() in that class so perhaps > DeletePersistentLocation() would be fitting. Done. https://codereview.chromium.org/2308763002/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2308763002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:210: task_runner->PostTask( On 2016/09/14 13:16:12, bcwhite wrote: > Comment: This file must be deleted here so it isn't mistakenly associated with > an incorrect instance of the browser by a later run. Done. https://codereview.chromium.org/2308763002/diff/100001/components/crash/conte... File components/crash/content/app/crash_reporter_client.h (right): https://codereview.chromium.org/2308763002/diff/100001/components/crash/conte... components/crash/content/app/crash_reporter_client.h:133: #if defined(OS_WIN) On 2016/09/14 13:16:12, bcwhite wrote: > Why can't Windows use FilePath? (This is copied from GetCrashDumpLocation() above.) base::FilePath uses a lot of base (e.g. logging and so on) which in turn pulls in a variety of Windows DLLs. This code is used in chrome_elf.dll where kernel32.dll is the only allowed dependency.
https://codereview.chromium.org/2308763002/diff/100001/third_party/crashpad/c... File third_party/crashpad/crashpad/handler/handler_main.cc (right): https://codereview.chromium.org/2308763002/diff/100001/third_party/crashpad/c... third_party/crashpad/crashpad/handler/handler_main.cc:406: std::unique_ptr<CrashReportDatabase> database(CrashReportDatabase::Initialize( I think we should move metrics initialization here (so that it's after the handshake). Mark, do you think it's worth passing the --metrics directory through to avoid stuffing the two files into the database directory? I understand your concern about theoretically polluting something that might not even be a directory, but the passing through of an option that non-Chrome doesn't use and even the Chrome case "barely" needs is a bit gross.
LGTM Thanks for doing all that! https://codereview.chromium.org/2308763002/diff/100001/components/crash/conte... File components/crash/content/app/crash_reporter_client.h (right): https://codereview.chromium.org/2308763002/diff/100001/components/crash/conte... components/crash/content/app/crash_reporter_client.h:133: #if defined(OS_WIN) On 2016/09/14 20:37:02, scottmg wrote: > On 2016/09/14 13:16:12, bcwhite wrote: > > Why can't Windows use FilePath? > > (This is copied from GetCrashDumpLocation() above.) base::FilePath uses a lot of > base (e.g. logging and so on) which in turn pulls in a variety of Windows DLLs. > This code is used in chrome_elf.dll where kernel32.dll is the only allowed > dependency. Makes sense. Add to comment? https://codereview.chromium.org/2308763002/diff/140001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.h (right): https://codereview.chromium.org/2308763002/diff/140001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.h:415: // are then used for CreateWithActiveFile(). Note that |name| will be used for both the internal name of the allocator and the name of the file within |dir|. https://codereview.chromium.org/2308763002/diff/140001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.h:423: static void ConstructBaseActivePairFilePaths(const FilePath& dir, How about shortening this method to just ContructFilePaths so it can be used even when a pair isn't needed (by passing nullptr for the second FilePath)? Then it could be used for getting the path passed to CreateWithFile(). Also, it's worth noting that |name| will be the file name and though it's commonly the same as the name of the persistent memory block, it doesn't have to be. https://codereview.chromium.org/2308763002/diff/140001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.h:478: // deleted when the process exits. A normal shutdown is almost complete so Comment needs updating for its new location. A shutdown being almost complete is a likely use but not strictly necessary.
Thanks https://codereview.chromium.org/2308763002/diff/100001/components/crash/conte... File components/crash/content/app/crash_reporter_client.h (right): https://codereview.chromium.org/2308763002/diff/100001/components/crash/conte... components/crash/content/app/crash_reporter_client.h:133: #if defined(OS_WIN) On 2016/09/14 21:11:29, bcwhite wrote: > On 2016/09/14 20:37:02, scottmg wrote: > > On 2016/09/14 13:16:12, bcwhite wrote: > > > Why can't Windows use FilePath? > > > > (This is copied from GetCrashDumpLocation() above.) base::FilePath uses a lot > of > > base (e.g. logging and so on) which in turn pulls in a variety of Windows > DLLs. > > This code is used in chrome_elf.dll where kernel32.dll is the only allowed > > dependency. > > Makes sense. Add to comment? Done. https://codereview.chromium.org/2308763002/diff/140001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.h (right): https://codereview.chromium.org/2308763002/diff/140001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.h:415: // are then used for CreateWithActiveFile(). On 2016/09/14 21:11:29, bcwhite wrote: > Note that |name| will be used for both the internal name of the allocator and > the name of the file within |dir|. Done. https://codereview.chromium.org/2308763002/diff/140001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.h:423: static void ConstructBaseActivePairFilePaths(const FilePath& dir, On 2016/09/14 21:11:30, bcwhite wrote: > How about shortening this method to just ContructFilePaths so it can be used > even when a pair isn't needed (by passing nullptr for the second FilePath)? > Then it could be used for getting the path passed to CreateWithFile(). > > Also, it's worth noting that |name| will be the file name and though it's > commonly the same as the name of the persistent memory block, it doesn't have to > be. Done. https://codereview.chromium.org/2308763002/diff/140001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.h:478: // deleted when the process exits. A normal shutdown is almost complete so On 2016/09/14 21:11:30, bcwhite wrote: > Comment needs updating for its new location. A shutdown being almost complete > is a likely use but not strictly necessary. Done.
The CQ bit was checked by scottmg@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...
LGTM otherwise https://codereview.chromium.org/2308763002/diff/180001/third_party/crashpad/c... File third_party/crashpad/crashpad/client/crashpad_client.h (right): https://codereview.chromium.org/2308763002/diff/180001/third_party/crashpad/c... third_party/crashpad/crashpad/client/crashpad_client.h:58: //! \param[in] metrics The path to an already existing directory where metrics Since you’re requiring this be a directory, “metrics_dir” to disambiguate a bit more. https://codereview.chromium.org/2308763002/diff/180001/third_party/crashpad/c... third_party/crashpad/crashpad/client/crashpad_client.h:60: //! `--metrics` argument. We should go with --metrics-dir for this too. https://codereview.chromium.org/2308763002/diff/180001/third_party/crashpad/c... File third_party/crashpad/crashpad/handler/handler_main.cc (right): https://codereview.chromium.org/2308763002/diff/180001/third_party/crashpad/c... third_party/crashpad/crashpad/handler/handler_main.cc:82: " --metrics=PATH store metrics files at PATH (only in Chromium)\n" --metrics-dir=DIR…store metrics files in DIR (only in Chromium) https://codereview.chromium.org/2308763002/diff/180001/third_party/crashpad/c... third_party/crashpad/crashpad/handler/handler_main.cc:435: base::GlobalHistogramAllocator* allocator = histogram_allocator https://codereview.chromium.org/2308763002/diff/180001/third_party/crashpad/c... third_party/crashpad/crashpad/handler/handler_main.cc:436: base::GlobalHistogramAllocator::Get(); Looks like we’re already calling this above in the (options.metrics) block. We should probably only call it once, up there.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks! I'll land here first to make sure things are OK and then apply upstream and figure out mini_chromium if that's OK with you. https://codereview.chromium.org/2308763002/diff/180001/third_party/crashpad/c... File third_party/crashpad/crashpad/client/crashpad_client.h (right): https://codereview.chromium.org/2308763002/diff/180001/third_party/crashpad/c... third_party/crashpad/crashpad/client/crashpad_client.h:58: //! \param[in] metrics The path to an already existing directory where metrics On 2016/09/14 23:48:39, Mark Mentovai wrote: > Since you’re requiring this be a directory, “metrics_dir” to disambiguate a bit > more. Done. https://codereview.chromium.org/2308763002/diff/180001/third_party/crashpad/c... third_party/crashpad/crashpad/client/crashpad_client.h:60: //! `--metrics` argument. On 2016/09/14 23:48:40, Mark Mentovai wrote: > We should go with --metrics-dir for this too. Done. https://codereview.chromium.org/2308763002/diff/180001/third_party/crashpad/c... File third_party/crashpad/crashpad/handler/handler_main.cc (right): https://codereview.chromium.org/2308763002/diff/180001/third_party/crashpad/c... third_party/crashpad/crashpad/handler/handler_main.cc:82: " --metrics=PATH store metrics files at PATH (only in Chromium)\n" On 2016/09/14 23:48:40, Mark Mentovai wrote: > --metrics-dir=DIR…store metrics files in DIR (only in Chromium) Done. https://codereview.chromium.org/2308763002/diff/180001/third_party/crashpad/c... third_party/crashpad/crashpad/handler/handler_main.cc:435: base::GlobalHistogramAllocator* allocator = On 2016/09/14 23:48:40, Mark Mentovai wrote: > histogram_allocator Done. https://codereview.chromium.org/2308763002/diff/180001/third_party/crashpad/c... third_party/crashpad/crashpad/handler/handler_main.cc:436: base::GlobalHistogramAllocator::Get(); On 2016/09/14 23:48:40, Mark Mentovai wrote: > Looks like we’re already calling this above in the (options.metrics) block. We > should probably only call it once, up there. Done.
The CQ bit was checked by scottmg@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...
LGTM. I’m cool with landing in Chrome first. https://codereview.chromium.org/2308763002/diff/200001/third_party/crashpad/c... File third_party/crashpad/crashpad/handler/handler_main.cc (right): https://codereview.chromium.org/2308763002/diff/200001/third_party/crashpad/c... third_party/crashpad/crashpad/handler/handler_main.cc:437: histogram_allocator->DeletePersistentLocation(); Optional: consider a scoper to do this instead. There’s already an early return between where it’s set up and here, and although we’re usually thinking that the process will just exit after that return, we have in fact made this a library function and we’re not assured of the process’ lifetime after this function returns.
scottmg@chromium.org changed reviewers: + asvitkine@chromium.org
+asvitkine: could you review chrome\browser\chrome_browser_field_trials.cc chrome\browser\metrics\chrome_metrics_service_client.cc for OWNERS, please?
https://codereview.chromium.org/2308763002/diff/200001/third_party/crashpad/c... File third_party/crashpad/crashpad/handler/handler_main.cc (right): https://codereview.chromium.org/2308763002/diff/200001/third_party/crashpad/c... third_party/crashpad/crashpad/handler/handler_main.cc:437: histogram_allocator->DeletePersistentLocation(); On 2016/09/15 00:08:55, Mark Mentovai wrote: > Optional: consider a scoper to do this instead. There’s already an early return > between where it’s set up and here, and although we’re usually thinking that the > process will just exit after that return, we have in fact made this a library > function and we’re not assured of the process’ lifetime after this function > returns. I wonder if we shouldn't do that, so that we gather the metrics for not-quite-clean exits in case of database initialization failure? I'm not sure if that's a good enough rationale for non-scoping.
The CQ bit was checked by scottmg@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks! Some comments for you below. https://codereview.chromium.org/2308763002/diff/220001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/2308763002/diff/220001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.cc:747: *out_base_path = dir.AppendASCII(name) Nit: I'd prefer to wrap after the = But you can keep as-is if this is what git cl format produced. https://codereview.chromium.org/2308763002/diff/220001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.cc:755: #endif // !OS_NACL Nit: !defined(OS_NACL) https://codereview.chromium.org/2308763002/diff/220001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.cc:887: File file(persistent_location_, Is this supported for NACL builds? Seems like it shouldn't given the other file manipulation code isn't. Maybe we should add a persistent_metrics_file_utils.cc file and move all the file handled code there instead of having the NACL ifdefs? https://codereview.chromium.org/2308763002/diff/220001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2308763002/diff/220001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:212: } This block seems very similar to the one above - can you make a helper function so they share code? https://codereview.chromium.org/2308763002/diff/220001/components/crash/conte... File components/crash/content/app/crash_reporter_client.h (right): https://codereview.chromium.org/2308763002/diff/220001/components/crash/conte... components/crash/content/app/crash_reporter_client.h:137: #if defined(OS_WIN) This seems a bit ugly. Have you considered just use base::string16 on all platforms and handling it from the calling code appropriately? https://codereview.chromium.org/2308763002/diff/220001/third_party/crashpad/c... File third_party/crashpad/crashpad/handler/handler_main.cc (right): https://codereview.chromium.org/2308763002/diff/220001/third_party/crashpad/c... third_party/crashpad/crashpad/handler/handler_main.cc:402: const char kMetricsName[] = "CrashpadMetrics"; Nit: static const for - as otherwise this results in extra goop in the generated code. https://codereview.chromium.org/2308763002/diff/220001/third_party/crashpad/c... third_party/crashpad/crashpad/handler/handler_main.cc:404: metrics_dir, 1 << 20, 0, kMetricsName)) { I'd move 1 << 20 into a constant above this call with a comment about the value. Otherwise, looks quite cryptic.
https://codereview.chromium.org/2308763002/diff/220001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/2308763002/diff/220001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.cc:887: File file(persistent_location_, On 2016/09/15 16:49:44, Alexei Svitkine (very slow) wrote: > Is this supported for NACL builds? Seems like it shouldn't given the other file > manipulation code isn't. > > Maybe we should add a persistent_metrics_file_utils.cc file and move all the > file handled code there instead of having the NACL ifdefs? Heh. I told him to move it in here along-side WriteToPersistentLocation(). Back when working on profiles, there was a general desire *against* _util files preferring integration or complete segregation.
Yeah, it's true "utils" file are not advised. In this case, it's pretty targeted - so maybe _file_handling.cc? On Thu, Sep 15, 2016 at 1:07 PM, <bcwhite@chromium.org> wrote: > > https://codereview.chromium.org/2308763002/diff/220001/ > base/metrics/persistent_histogram_allocator.cc > File base/metrics/persistent_histogram_allocator.cc (right): > > https://codereview.chromium.org/2308763002/diff/220001/ > base/metrics/persistent_histogram_allocator.cc#newcode887 > base/metrics/persistent_histogram_allocator.cc:887: File > file(persistent_location_, > On 2016/09/15 16:49:44, Alexei Svitkine (very slow) wrote: > > Is this supported for NACL builds? Seems like it shouldn't given the > other file > > manipulation code isn't. > > > > Maybe we should add a persistent_metrics_file_utils.cc file and move > all the > > file handled code there instead of having the NACL ifdefs? > > Heh. I told him to move it in here along-side > WriteToPersistentLocation(). > > Back when working on profiles, there was a general desire *against* > _util files preferring integration or complete segregation. > > https://codereview.chromium.org/2308763002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #12 (id:240001) has been deleted
https://codereview.chromium.org/2308763002/diff/220001/third_party/crashpad/c... File third_party/crashpad/crashpad/handler/handler_main.cc (right): https://codereview.chromium.org/2308763002/diff/220001/third_party/crashpad/c... third_party/crashpad/crashpad/handler/handler_main.cc:402: const char kMetricsName[] = "CrashpadMetrics"; Alexei Svitkine (very slow) wrote: > Nit: static const for - as otherwise this results in extra goop in the generated > code. I think that this is highly dependent on the compiler. The “static” doesn’t contribute anything valuable semantically.
> Yeah, it's true "utils" file are not advised. In this case, it's pretty > targeted - so maybe _file_handling.cc? Except it's targeted to a GlobalHistogramAllocator... So why not leave it in there?
https://codereview.chromium.org/2308763002/diff/220001/third_party/crashpad/c... File third_party/crashpad/crashpad/handler/handler_main.cc (right): https://codereview.chromium.org/2308763002/diff/220001/third_party/crashpad/c... third_party/crashpad/crashpad/handler/handler_main.cc:402: const char kMetricsName[] = "CrashpadMetrics"; On 2016/09/15 17:22:28, Mark Mentovai wrote: > Alexei Svitkine (very slow) wrote: > > Nit: static const for - as otherwise this results in extra goop in the > generated > > code. > > I think that this is highly dependent on the compiler. > > The “static” doesn’t contribute anything valuable semantically. I am referring to this discussion: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pF7nAmUQ2uw/wrNhX... Unless something changed with our compilers that resulted a change in this (which someone needs to demonstrate), I suggest following the syntax that results in the smaller more efficient code generated.
On Thu, Sep 15, 2016 at 1:26 PM, <bcwhite@chromium.org> wrote: > > Yeah, it's true "utils" file are not advised. In this case, it's pretty > > targeted - so maybe _file_handling.cc? > > Except it's targeted to a GlobalHistogramAllocator... So why not leave it > in > there? > Well, the idea is to reduce number of ifdefs. It's generally preferred to have separate files if there's enough functionality behind an ifdef than to have large blocks of ifdef'd code. > > > https://codereview.chromium.org/2308763002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2308763002/diff/220001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/2308763002/diff/220001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.cc:747: *out_base_path = dir.AppendASCII(name) On 2016/09/15 16:49:44, Alexei Svitkine (very slow) wrote: > Nit: I'd prefer to wrap after the = > > But you can keep as-is if this is what git cl format produced. Yeah, this is clang-format output. It doesn't quite fit only after the =, so one of the .s has to be a break point anyway. https://codereview.chromium.org/2308763002/diff/220001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.cc:755: #endif // !OS_NACL On 2016/09/15 16:49:44, Alexei Svitkine (very slow) wrote: > Nit: !defined(OS_NACL) Done. https://codereview.chromium.org/2308763002/diff/220001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.cc:887: File file(persistent_location_, On 2016/09/15 16:49:44, Alexei Svitkine (very slow) wrote: > Is this supported for NACL builds? Seems like it shouldn't given the other file > manipulation code isn't. > > Maybe we should add a persistent_metrics_file_utils.cc file and move all the > file handled code there instead of having the NACL ifdefs? #ifdef'd for now to match and fix the build. As Brian said I had a persistent_metrics_file_util.cc/h in a previous CL, but I think this is tidier overall. Maybe we could break it up into NaCl files and non-NaCl files in a future change maybe? https://codereview.chromium.org/2308763002/diff/220001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2308763002/diff/220001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:212: } On 2016/09/15 16:49:44, Alexei Svitkine (very slow) wrote: > This block seems very similar to the one above - can you make a helper function > so they share code? Done. https://codereview.chromium.org/2308763002/diff/220001/components/crash/conte... File components/crash/content/app/crash_reporter_client.h (right): https://codereview.chromium.org/2308763002/diff/220001/components/crash/conte... components/crash/content/app/crash_reporter_client.h:137: #if defined(OS_WIN) On 2016/09/15 16:49:44, Alexei Svitkine (very slow) wrote: > This seems a bit ugly. Have you considered just use base::string16 on all > platforms and handling it from the calling code appropriately? That seems reasonable. I should do GetCrashDumpLocation() at the same time and that means fiddling with Breakpad platforms too, so I'll investigate that in a followup if that's OK. https://codereview.chromium.org/2308763002/diff/220001/third_party/crashpad/c... File third_party/crashpad/crashpad/handler/handler_main.cc (right): https://codereview.chromium.org/2308763002/diff/220001/third_party/crashpad/c... third_party/crashpad/crashpad/handler/handler_main.cc:402: const char kMetricsName[] = "CrashpadMetrics"; On 2016/09/15 16:49:44, Alexei Svitkine (very slow) wrote: > Nit: static const for - as otherwise this results in extra goop in the generated > code. Done. https://codereview.chromium.org/2308763002/diff/220001/third_party/crashpad/c... third_party/crashpad/crashpad/handler/handler_main.cc:404: metrics_dir, 1 << 20, 0, kMetricsName)) { On 2016/09/15 16:49:44, Alexei Svitkine (very slow) wrote: > I'd move 1 << 20 into a constant above this call with a comment about the value. > Otherwise, looks quite cryptic. Done.
LGTM thanks! https://codereview.chromium.org/2308763002/diff/220001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/2308763002/diff/220001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.cc:887: File file(persistent_location_, On 2016/09/15 17:41:13, scottmg wrote: > On 2016/09/15 16:49:44, Alexei Svitkine (very slow) wrote: > > Is this supported for NACL builds? Seems like it shouldn't given the other > file > > manipulation code isn't. > > > > Maybe we should add a persistent_metrics_file_utils.cc file and move all the > > file handled code there instead of having the NACL ifdefs? > > #ifdef'd for now to match and fix the build. As Brian said I had a > persistent_metrics_file_util.cc/h in a previous CL, but I think this is tidier > overall. > > Maybe we could break it up into NaCl files and non-NaCl files in a future change > maybe? Sure, I'm ok with that being explored in a separate CL. https://codereview.chromium.org/2308763002/diff/220001/components/crash/conte... File components/crash/content/app/crash_reporter_client.h (right): https://codereview.chromium.org/2308763002/diff/220001/components/crash/conte... components/crash/content/app/crash_reporter_client.h:137: #if defined(OS_WIN) On 2016/09/15 17:41:13, scottmg wrote: > On 2016/09/15 16:49:44, Alexei Svitkine (very slow) wrote: > > This seems a bit ugly. Have you considered just use base::string16 on all > > platforms and handling it from the calling code appropriately? > > That seems reasonable. I should do GetCrashDumpLocation() at the same time and > that means fiddling with Breakpad platforms too, so I'll investigate that in a > followup if that's OK. SGTM. https://codereview.chromium.org/2308763002/diff/260001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2308763002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:145: void RegisterOrRemovePreviousRunMetricsFile( Nit: Add a comment introducing this.
https://codereview.chromium.org/2308763002/diff/220001/base/metrics/persisten... File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/2308763002/diff/220001/base/metrics/persisten... base/metrics/persistent_histogram_allocator.cc:887: File file(persistent_location_, On 2016/09/15 17:49:16, Alexei Svitkine (very slow) wrote: > On 2016/09/15 17:41:13, scottmg wrote: > > On 2016/09/15 16:49:44, Alexei Svitkine (very slow) wrote: > > > Is this supported for NACL builds? Seems like it shouldn't given the other > > file > > > manipulation code isn't. > > > > > > Maybe we should add a persistent_metrics_file_utils.cc file and move all the > > > file handled code there instead of having the NACL ifdefs? > > > > #ifdef'd for now to match and fix the build. As Brian said I had a > > persistent_metrics_file_util.cc/h in a previous CL, but I think this is tidier > > overall. > > > > Maybe we could break it up into NaCl files and non-NaCl files in a future > change > > maybe? > > Sure, I'm ok with that being explored in a separate CL. If I remember correctly, it's only the .cc file that is the problem; I included the #ifdef in the .h file for correctness-sake. But you could omit the #ifdef here and have a separate persistent_histogram_allocator_nonacl.cc file with those definitions (in that future CL).
https://codereview.chromium.org/2308763002/diff/260001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2308763002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:145: void RegisterOrRemovePreviousRunMetricsFile( On 2016/09/15 17:49:16, Alexei Svitkine (very slow) wrote: > Nit: Add a comment introducing this. Done.
The CQ bit was checked by scottmg@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...
https://codereview.chromium.org/2308763002/diff/220001/third_party/crashpad/c... File third_party/crashpad/crashpad/handler/handler_main.cc (right): https://codereview.chromium.org/2308763002/diff/220001/third_party/crashpad/c... third_party/crashpad/crashpad/handler/handler_main.cc:402: const char kMetricsName[] = "CrashpadMetrics"; On 2016/09/15 17:41:13, scottmg wrote: > On 2016/09/15 16:49:44, Alexei Svitkine (very slow) wrote: > > Nit: static const for - as otherwise this results in extra goop in the > generated > > code. > > Done. I missed this conversation before I made the change, but for the record, here's the results of static const vs. just const on an Official (LTCG) Windows Release build. LHS is static const char kMetricsName[] = "CrashpadMetrics"; RHS is const char kMetricsName[] = "CrashpadMetrics"; https://www.diffchecker.com/YR5dl230
scottmg wrote: > https://codereview.chromium.org/2308763002/diff/220001/third_party/crashpad/c... > File third_party/crashpad/crashpad/handler/handler_main.cc (right): > > https://codereview.chromium.org/2308763002/diff/220001/third_party/crashpad/c... > third_party/crashpad/crashpad/handler/handler_main.cc:402: const char > kMetricsName[] = "CrashpadMetrics"; > On 2016/09/15 17:41:13, scottmg wrote: > > On 2016/09/15 16:49:44, Alexei Svitkine (very slow) wrote: > > > Nit: static const for - as otherwise this results in extra goop in the > > generated > > > code. > > > > Done. > > I missed this conversation before I made the change, but for the record, here's > the results of static const vs. just const on an Official (LTCG) Windows Release > build. > > LHS is static const char kMetricsName[] = "CrashpadMetrics"; > RHS is const char kMetricsName[] = "CrashpadMetrics"; Well, clang generates the same code for each without needing a static crutch.
On 2016/09/15 19:37:26, Mark Mentovai wrote: > scottmg wrote: > > > https://codereview.chromium.org/2308763002/diff/220001/third_party/crashpad/c... > > File third_party/crashpad/crashpad/handler/handler_main.cc (right): > > > > > https://codereview.chromium.org/2308763002/diff/220001/third_party/crashpad/c... > > third_party/crashpad/crashpad/handler/handler_main.cc:402: const char > > kMetricsName[] = "CrashpadMetrics"; > > On 2016/09/15 17:41:13, scottmg wrote: > > > On 2016/09/15 16:49:44, Alexei Svitkine (very slow) wrote: > > > > Nit: static const for - as otherwise this results in extra goop in the > > > generated > > > > code. > > > > > > Done. > > > > I missed this conversation before I made the change, but for the record, > here's > > the results of static const vs. just const on an Official (LTCG) Windows > Release > > build. > > > > LHS is static const char kMetricsName[] = "CrashpadMetrics"; > > RHS is const char kMetricsName[] = "CrashpadMetrics"; > > Well, clang generates the same code for each without needing a static crutch. Yeah, it seems pretty crappy. I'm really surprised it can't figure that out, especially in LTCG where it can see everything at link time.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by scottmg@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 #14 (id:300001) has been deleted
The CQ bit was unchecked by scottmg@chromium.org
The CQ bit was checked by scottmg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bcwhite@chromium.org, mark@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2308763002/#ps280001 (title: ".")
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 scottmg@chromium.org
The CQ bit was checked by scottmg@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 scottmg@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...)
The CQ bit was checked by scottmg@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by scottmg@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_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #18 (id:390001) has been deleted
The CQ bit was checked by scottmg@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #18 (id:410001) has been deleted
The CQ bit was checked by scottmg@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: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Patchset #18 (id:430001) has been deleted
The CQ bit was checked by scottmg@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 scottmg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bcwhite@chromium.org, mark@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2308763002/#ps450001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #18 (id:450001)
Message was sent while issue was closed.
Description was changed from ========== Integrate Crashpad UMA Creates an global persistent metrics storage helper in base/metrics to allow stubbing in Crashpad when built outside of Chrome. Uses a pair of persistent metrics files, one that is the currently active one, which is renamed to a "previous run" on startup. In normal operation -active will be created, used, and removed on shutdown. However, if Crashpad itself crashes, then on next startup, -active will be moved to previous so that previous can be slurped in on startup to retrieve data from the crash. BUG=crashpad:100 ========== to ========== Integrate Crashpad UMA Creates an global persistent metrics storage helper in base/metrics to allow stubbing in Crashpad when built outside of Chrome. Uses a pair of persistent metrics files, one that is the currently active one, which is renamed to a "previous run" on startup. In normal operation -active will be created, used, and removed on shutdown. However, if Crashpad itself crashes, then on next startup, -active will be moved to previous so that previous can be slurped in on startup to retrieve data from the crash. BUG=crashpad:100 Committed: https://crrev.com/785ed6054d704b40cf4be1da82a88c979efe03bb Cr-Commit-Position: refs/heads/master@{#419251} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/785ed6054d704b40cf4be1da82a88c979efe03bb Cr-Commit-Position: refs/heads/master@{#419251} |
