Skip to content

Commit

Permalink
Delay PersistentHistogramAllocator creation until it's known to be used.
Browse files Browse the repository at this point in the history 
Roughly 25 histograms get created before the allocator is
created in the field-trial setup but these are generally
information about initialization activities that are
reported quickly. Nothing is generally lost should the
browser crash without these being persistent.

BUG=612470
TBR=pfeldman
pfeldman: moved code out of browser_main_runner

Review-Url: https://codereview.chromium.org/1996843002
Cr-Commit-Position: refs/heads/master@{#395101}
  • Loading branch information
bcwhite authored and Commit bot committed May 20, 2016
1 parent 8b8c576 commit 374d5fe
Show file tree
Hide file tree
Showing 6 changed files with 39 additions and 86 deletions.
21 changes: 0 additions & 21 deletions base/metrics/persistent_histogram_allocator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ enum : uint32_t {
// anything essential at exit anyway due to the fact that they depend on data
// managed elsewhere and which could be destructed first.
GlobalHistogramAllocator* g_allocator = nullptr;
bool g_allocator_enabled = false;

// Take an array of range boundaries and create a proper BucketRanges object
// which is returned to the caller. A return of nullptr indicates that the
Expand Down Expand Up @@ -656,18 +655,6 @@ void GlobalHistogramAllocator::CreateWithSharedMemoryHandle(
std::move(shm), 0, StringPiece(), /*readonly=*/false)))));
}

// static
void GlobalHistogramAllocator::Enable() {
DCHECK(g_allocator);
g_allocator_enabled = true;
}

// static
void GlobalHistogramAllocator::Disable() {
DCHECK(g_allocator);
g_allocator_enabled = false;
}

// static
void GlobalHistogramAllocator::Set(
std::unique_ptr<GlobalHistogramAllocator> allocator) {
Expand All @@ -676,7 +663,6 @@ void GlobalHistogramAllocator::Set(
// also released, future accesses to those histograms will seg-fault.
CHECK(!g_allocator);
g_allocator = allocator.release();
g_allocator_enabled = true;
size_t existing = StatisticsRecorder::GetHistogramCount();

DVLOG_IF(1, existing)
Expand All @@ -685,11 +671,6 @@ void GlobalHistogramAllocator::Set(

// static
GlobalHistogramAllocator* GlobalHistogramAllocator::Get() {
return g_allocator_enabled ? g_allocator : nullptr;
}

// static
GlobalHistogramAllocator* GlobalHistogramAllocator::GetEvenIfDisabled() {
return g_allocator;
}

Expand Down Expand Up @@ -733,8 +714,6 @@ void GlobalHistogramAllocator::SetPersistentLocation(const FilePath& location) {
}

bool GlobalHistogramAllocator::WriteToPersistentLocation() {
DCHECK(g_allocator_enabled);

#if defined(OS_NACL)
// NACL doesn't support file operations, including ImportantFileWriter.
NOTREACHED();
Expand Down
24 changes: 6 additions & 18 deletions base/metrics/persistent_histogram_allocator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -394,26 +394,14 @@ class BASE_EXPORT GlobalHistogramAllocator
// while operating single-threaded so there are no race-conditions.
static void Set(std::unique_ptr<GlobalHistogramAllocator> allocator);

// Enables the global persistent allocator. Existing histograms will not be
// affected; only newly created ones will go into the allocator.
static void Enable();

// Disables the global persistent allocator. Existing histograms will not be
// affected; newly created histograms will be allocated from the heap.
static void Disable();

// Gets a pointer to an enabled global histogram allocator. If a global
// allocator exists but is disabled, this will return null.
// Gets a pointer to the global histogram allocator. Returns null if none
// exists.
static GlobalHistogramAllocator* Get();

// Gets a pointer to a global histogram allocator if one exists, even if
// that allocator is disabled.
static GlobalHistogramAllocator* GetEvenIfDisabled();

// This access to the persistent allocator is only for testing; it extracts
// the current allocator completely regardless whether it is enabled or not.
// This allows easy creation of histograms within persistent memory segments
// which can then be extracted and used in other ways.
// the current allocator completely. This allows easy creation of histograms
// within persistent memory segments which can then be extracted and used in
// other ways.
static std::unique_ptr<GlobalHistogramAllocator> ReleaseForTesting();

// Stores a pathname to which the contents of this allocator should be saved
Expand All @@ -429,7 +417,7 @@ class BASE_EXPORT GlobalHistogramAllocator
private:
friend class StatisticsRecorder;

// Creates a new global histogram allocator. It will be enabled by default.
// Creates a new global histogram allocator.
explicit GlobalHistogramAllocator(
std::unique_ptr<PersistentMemoryAllocator> memory);

Expand Down
14 changes: 11 additions & 3 deletions chrome/browser/chrome_browser_field_trials.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "base/strings/string_util.h"
#include "base/time/time.h"
#include "build/build_config.h"
#include "chrome/browser/metrics/chrome_metrics_service_client.h"
#include "chrome/common/chrome_switches.h"
#include "components/metrics/metrics_pref_names.h"

Expand All @@ -29,11 +30,18 @@ namespace {
// enable the global allocator if so.
void InstantiatePersistentHistograms() {
if (base::FeatureList::IsEnabled(base::kPersistentHistogramsFeature)) {
base::GlobalHistogramAllocator::Enable();
// Create persistent/shared memory and allow histograms to be stored in
// it. Memory that is not actualy used won't be physically mapped by the
// system. BrowserMetrics usage, as reported in UMA, peaked around 1.9MiB
// as of 2016-02-20.
base::GlobalHistogramAllocator::CreateWithLocalMemory(
3 << 20, // 3 MiB
0x935DDD43, // SHA1(BrowserMetrics)
ChromeMetricsServiceClient::kBrowserMetricsName);
base::GlobalHistogramAllocator* allocator =
base::GlobalHistogramAllocator::Get();
DCHECK(allocator); // Should have been created during startup.
allocator->CreateTrackingHistograms(allocator->Name());
allocator->CreateTrackingHistograms(
ChromeMetricsServiceClient::kBrowserMetricsName);
}
}

Expand Down
49 changes: 16 additions & 33 deletions chrome/browser/metrics/chrome_metrics_service_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ namespace {
// This specifies the amount of time to wait for all renderers to send their
// data.
const int kMaxHistogramGatheringWaitDuration = 60000; // 60 seconds.
const char kBrowserMetricsFileName[] = "SavedMetrics";

// Checks whether it is the first time that cellular uploads logic should be
// enabled based on whether the the preference for that logic is initialized.
Expand All @@ -120,23 +119,15 @@ bool ShouldClearSavedMetrics() {
}

void RegisterInstallerFileMetricsPreferences(PrefRegistrySimple* registry) {
base::GlobalHistogramAllocator* allocator =
base::GlobalHistogramAllocator::GetEvenIfDisabled();
if (allocator)
metrics::FileMetricsProvider::RegisterPrefs(registry, allocator->Name());
metrics::FileMetricsProvider::RegisterPrefs(
registry, ChromeMetricsServiceClient::kBrowserMetricsName);

#if defined(OS_WIN)
metrics::FileMetricsProvider::RegisterPrefs(
registry, installer::kSetupHistogramAllocatorName);
#endif
}

// A wrapper around base::DeleteFile that has no return value and thus can be
// used as a callback.
void LocalDeleteFile(const base::FilePath& path) {
base::DeleteFile(path, /*recursive=*/false);
}

std::unique_ptr<metrics::FileMetricsProvider>
CreateInstallerFileMetricsProvider(bool metrics_reporting_enabled) {
// Fetch a worker-pool for performing I/O tasks that are not allowed on
Expand All @@ -151,37 +142,27 @@ CreateInstallerFileMetricsProvider(bool metrics_reporting_enabled) {
new metrics::FileMetricsProvider(task_runner,
g_browser_process->local_state()));

// Build the pathname for browser's persistent metrics file. Set it as the
// destination for the GlobalHistogramAllocator during process exit and add
// it to the file-metrics-provider so one written from a previous run will
// be loaded.
// TODO(bcwhite): Actually create those files.
// Create the full pathname of the file holding browser metrics.
base::FilePath metrics_file;
if (base::PathService::Get(chrome::DIR_USER_DATA, &metrics_file)) {
metrics_file =
metrics_file.AppendASCII(kBrowserMetricsFileName)
metrics_file
.AppendASCII(ChromeMetricsServiceClient::kBrowserMetricsName)
.AddExtension(base::PersistentMemoryAllocator::kFileExtension);

if (metrics_reporting_enabled) {
// Metrics get persisted to disk when the process exits. Store the path
// to that file in the global allocator for use at exit time and tell
// the FileMetricsProvider about that file so it can read one created
// by the previous run.
base::GlobalHistogramAllocator* allocator =
base::GlobalHistogramAllocator::GetEvenIfDisabled();
if (allocator) {
const char* allocator_name = allocator->Name();
allocator->SetPersistentLocation(metrics_file);
file_metrics_provider->RegisterSource(
metrics_file,
metrics::FileMetricsProvider::SOURCE_HISTOGRAMS_ATOMIC_FILE,
metrics::FileMetricsProvider::ASSOCIATE_PREVIOUS_RUN,
allocator_name);
}
// Enable reading any existing saved metrics.
file_metrics_provider->RegisterSource(
metrics_file,
metrics::FileMetricsProvider::SOURCE_HISTOGRAMS_ATOMIC_FILE,
metrics::FileMetricsProvider::ASSOCIATE_PREVIOUS_RUN,
ChromeMetricsServiceClient::kBrowserMetricsName);
} else {
// When metrics reporting is not enabled, any existing file should be
// deleted in order to preserve user privacy.
task_runner->PostTask(FROM_HERE,
base::Bind(&LocalDeleteFile, metrics_file));
base::Bind(base::IgnoreResult(&base::DeleteFile),
metrics_file, /*recursive=*/false));
}
}

Expand All @@ -202,6 +183,8 @@ CreateInstallerFileMetricsProvider(bool metrics_reporting_enabled) {
} // namespace


const char ChromeMetricsServiceClient::kBrowserMetricsName[] = "BrowserMetrics";

ChromeMetricsServiceClient::ChromeMetricsServiceClient(
metrics::MetricsStateManager* state_manager)
: metrics_state_manager_(state_manager),
Expand Down
6 changes: 6 additions & 0 deletions chrome/browser/metrics/chrome_metrics_service_client.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,12 @@ class ChromeMetricsServiceClient
EnableMetricsDefault GetDefaultOptIn() override;
bool IsUMACellularUploadLogicEnabled() override;

// Persistent browser metrics need to be persisted somewhere. This constant
// provides a known string to be used for both the allocator's internal name
// and for a file on disk (relative to chrome::DIR_USER_DATA) to which they
// can be saved.
static const char kBrowserMetricsName[];

private:
explicit ChromeMetricsServiceClient(
metrics::MetricsStateManager* state_manager);
Expand Down
11 changes: 0 additions & 11 deletions content/browser/browser_main_runner.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#include "base/macros.h"
#include "base/metrics/histogram.h"
#include "base/metrics/histogram_macros.h"
#include "base/metrics/persistent_histogram_allocator.h"
#include "base/metrics/statistics_recorder.h"
#include "base/profiler/scoped_profile.h"
#include "base/profiler/scoped_tracker.h"
Expand Down Expand Up @@ -82,16 +81,6 @@ class BrowserMainRunnerImpl : public BrowserMainRunner {

const base::TimeTicks start_time_step1 = base::TimeTicks::Now();

// Create persistent/shared memory and allow histograms to be stored in
// it. Memory that is not actualy used won't be physically mapped by the
// system. BrowserMetrics usage, as reported in UMA, peaked around 1.9MiB
// as of 2016-02-20.
base::GlobalHistogramAllocator::CreateWithLocalMemory(
3 << 20, // 3 MiB
0x935DDD43, // SHA1(BrowserMetrics)
"BrowserMetrics");
base::GlobalHistogramAllocator::Disable(); // Enabled by experiment only.

SkGraphics::Init();

if (parameters.command_line.HasSwitch(switches::kWaitForDebugger))
Expand Down

0 comments on commit 374d5fe

Please sign in to comment.