Skip to content

GitLab

T
tangled
Commit 3b1fd5e7 authored by Daniel Cheng's avatar Daniel Cheng Committed by Commit Bot
Browse files
Options

Switch histogram code to using std::atomic. 

Mostly cleanup. Note that this still uses atomics instead of C++11's
thread-safe statics. C++11's thread-safe statics incur a large (200+ KB)
size penalty since they guarantee that only one thread enters the
initialization block.

Change-Id: I3b013b55d5108f86ede39d0e0273ea10f8abf5e9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2045757Reviewed-by: default avatarNico Weber <thakis@chromium.org>
Reviewed-by: default avatarAlbert J. Wong <ajwong@chromium.org>
Reviewed-by: default avatarAlexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Auto-Submit: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#740556}
parent 469e7c11
Show whitespace changes
Inline Side-by-side
Showing with 65 additions and 79 deletions
base/metrics/histogram_macros.h
View file @ 3b1fd5e7
...@@ -378,17 +378,16 @@ ...@@ -378,17 +378,16 @@
// instance will be used only for DCHECK builds and the second will // instance will be used only for DCHECK builds and the second will
// execute only during the first access to the given index, after which // execute only during the first access to the given index, after which
// the pointer is cached and the name never needed again. // the pointer is cached and the name never needed again.
#define STATIC_HISTOGRAM_POINTER_GROUP(constant_histogram_name, index, \ #define STATIC_HISTOGRAM_POINTER_GROUP( \
constant_maximum, \ constant_histogram_name, index, constant_maximum, \
histogram_add_method_invocation, \ histogram_add_method_invocation, histogram_factory_get_invocation) \
histogram_factory_get_invocation) \
do { \ do { \
static base::subtle::AtomicWord atomic_histograms[constant_maximum]; \ static std::atomic_uintptr_t atomic_histograms[constant_maximum]; \
DCHECK_LE(0, index); \ DCHECK_LE(0, index); \
DCHECK_LT(index, constant_maximum); \ DCHECK_LT(index, constant_maximum); \
HISTOGRAM_POINTER_USE(&atomic_histograms[index], constant_histogram_name, \ HISTOGRAM_POINTER_USE( \
histogram_add_method_invocation, \ std::addressof(atomic_histograms[index]), constant_histogram_name, \
histogram_factory_get_invocation); \ histogram_add_method_invocation, histogram_factory_get_invocation); \
} while (0) } while (0)
//------------------------------------------------------------------------------ //------------------------------------------------------------------------------
......
base/metrics/histogram_macros_internal.h
View file @ 3b1fd5e7
...@@ -7,10 +7,11 @@ ...@@ -7,10 +7,11 @@
#include <stdint.h> #include <stdint.h>
#include <atomic>
#include <limits> #include <limits>
#include <memory>
#include <type_traits> #include <type_traits>
#include "base/atomicops.h"
#include "base/logging.h" #include "base/logging.h"
#include "base/metrics/histogram.h" #include "base/metrics/histogram.h"
#include "base/metrics/sparse_histogram.h" #include "base/metrics/sparse_histogram.h"
...@@ -65,39 +66,30 @@ struct EnumSizeTraits< ...@@ -65,39 +66,30 @@ struct EnumSizeTraits<
// define HISTOGRAM_POINTER_USE, which uses an |atomic_histogram_pointer|, and // define HISTOGRAM_POINTER_USE, which uses an |atomic_histogram_pointer|, and
// STATIC_HISTOGRAM_POINTER_BLOCK, which defines an |atomic_histogram_pointer| // STATIC_HISTOGRAM_POINTER_BLOCK, which defines an |atomic_histogram_pointer|
// and forwards to HISTOGRAM_POINTER_USE. // and forwards to HISTOGRAM_POINTER_USE.
#define HISTOGRAM_POINTER_USE(atomic_histogram_pointer, \ #define HISTOGRAM_POINTER_USE( \
constant_histogram_name, \ atomic_histogram_pointer, constant_histogram_name, \
histogram_add_method_invocation, \ histogram_add_method_invocation, histogram_factory_get_invocation) \
histogram_factory_get_invocation) \
do { \ do { \
/* \
* Acquire_Load() ensures that we acquire visibility to the \
* pointed-to data in the histogram. \
*/ \
base::HistogramBase* histogram_pointer( \ base::HistogramBase* histogram_pointer( \
reinterpret_cast<base::HistogramBase*>( \ reinterpret_cast<base::HistogramBase*>( \
base::subtle::Acquire_Load(atomic_histogram_pointer))); \ atomic_histogram_pointer->load(std::memory_order_acquire))); \
if (!histogram_pointer) { \ if (!histogram_pointer) { \
/* \ /* \
* This is the slow path, which will construct OR find the \ * This is the slow path, which will construct OR find the \
* matching histogram. histogram_factory_get_invocation includes \ * matching histogram. |histogram_factory_get_invocation| includes \
* locks on a global histogram name map and is completely thread \ * locks on a global histogram name map and is completely thread \
* safe. \ * safe. \
*/ \ */ \
histogram_pointer = histogram_factory_get_invocation; \ histogram_pointer = histogram_factory_get_invocation; \
\ \
/* \ /* \
* Use Release_Store to ensure that the histogram data is made \ * We could do this without any barrier, since FactoryGet() \
* available globally before we make the pointer visible. Several \ * entered and exited a lock after construction, but this barrier \
* threads may perform this store, but the same value will be \ * makes things clear. \
* stored in all cases (for a given named/spec'ed histogram). \
* We could do this without any barrier, since FactoryGet entered \
* and exited a lock after construction, but this barrier makes \
* things clear. \
*/ \ */ \
base::subtle::Release_Store( \ atomic_histogram_pointer->store( \
atomic_histogram_pointer, \ reinterpret_cast<uintptr_t>(histogram_pointer), \
reinterpret_cast<base::subtle::AtomicWord>(histogram_pointer)); \ std::memory_order_release); \
} \ } \
if (DCHECK_IS_ON()) \ if (DCHECK_IS_ON()) \
histogram_pointer->CheckName(constant_histogram_name); \ histogram_pointer->CheckName(constant_histogram_name); \
...@@ -115,10 +107,10 @@ struct EnumSizeTraits< ...@@ -115,10 +107,10 @@ struct EnumSizeTraits<
* The pointer's presence indicates that the initialization is complete. \ * The pointer's presence indicates that the initialization is complete. \
* Initialization is idempotent, so it can safely be atomically repeated. \ * Initialization is idempotent, so it can safely be atomically repeated. \
*/ \ */ \
static base::subtle::AtomicWord atomic_histogram_pointer = 0; \ static std::atomic_uintptr_t atomic_histogram_pointer; \
HISTOGRAM_POINTER_USE(&atomic_histogram_pointer, constant_histogram_name, \ HISTOGRAM_POINTER_USE( \
histogram_add_method_invocation, \ std::addressof(atomic_histogram_pointer), constant_histogram_name, \
histogram_factory_get_invocation); \ histogram_add_method_invocation, histogram_factory_get_invocation); \
} while (0) } while (0)
// This is a helper macro used by other macros and shouldn't be used directly. // This is a helper macro used by other macros and shouldn't be used directly.
......
skia/ext/skia_histogram.cc
View file @ 3b1fd5e7
...@@ -5,21 +5,13 @@ ...@@ -5,21 +5,13 @@
#include "skia/ext/skia_histogram.h" #include "skia/ext/skia_histogram.h"
#include <type_traits> #include <type_traits>
#include "base/metrics/histogram_macros.h" #include "base/metrics/histogram_macros_internal.h"
// In order to prevent Chrome headers from leaking into Skia, we use a raw
// intptr_t in the header, rather than the base::subtle::AtomicWord. Make sure
// this is a valid assumption.
static_assert(std::is_same<intptr_t, base::subtle::AtomicWord>::value,
"To allow for header decoupling, skia_histogram.h uses intptr_t "
"instead of a base::subtle::AtomicWord. These must be the same "
"type");
namespace skia { namespace skia {
// Wrapper around HISTOGRAM_POINTER_USE - mimics UMA_HISTOGRAM_BOOLEAN but // Wrapper around HISTOGRAM_POINTER_USE - mimics UMA_HISTOGRAM_BOOLEAN but
// allows for an external atomic_histogram_pointer. // allows for an external atomic_histogram_pointer.
void HistogramBoolean(intptr_t* atomic_histogram_pointer, void HistogramBoolean(std::atomic_uintptr_t* atomic_histogram_pointer,
const char* name, const char* name,
bool sample) { bool sample) {
HISTOGRAM_POINTER_USE( HISTOGRAM_POINTER_USE(
...@@ -30,7 +22,7 @@ void HistogramBoolean(intptr_t* atomic_histogram_pointer, ...@@ -30,7 +22,7 @@ void HistogramBoolean(intptr_t* atomic_histogram_pointer,
// Wrapper around HISTOGRAM_POINTER_USE - mimics UMA_HISTOGRAM_ENUMERATION but // Wrapper around HISTOGRAM_POINTER_USE - mimics UMA_HISTOGRAM_ENUMERATION but
// allows for an external atomic_histogram_pointer. // allows for an external atomic_histogram_pointer.
void HistogramEnumeration(intptr_t* atomic_histogram_pointer, void HistogramEnumeration(std::atomic_uintptr_t* atomic_histogram_pointer,
const char* name, const char* name,
int sample, int sample,
int boundary_value) { int boundary_value) {
......
skia/ext/skia_histogram.h
View file @ 3b1fd5e7
...@@ -5,7 +5,10 @@ ...@@ -5,7 +5,10 @@
#ifndef SKIA_EXT_SKIA_HISTOGRAM_H_ #ifndef SKIA_EXT_SKIA_HISTOGRAM_H_
#define SKIA_EXT_SKIA_HISTOGRAM_H_ #define SKIA_EXT_SKIA_HISTOGRAM_H_
#include <cstdint> #include <stdint.h>
#include <atomic>
#include <memory>
// This file exposes Chrome's histogram functionality to Skia, without bringing // This file exposes Chrome's histogram functionality to Skia, without bringing
// in any Chrome specific headers. To achieve the same level of optimization as // in any Chrome specific headers. To achieve the same level of optimization as
...@@ -14,8 +17,8 @@ ...@@ -14,8 +17,8 @@
// placeholder is passed to the actual histogram logic in Chrome. // placeholder is passed to the actual histogram logic in Chrome.
#define SK_HISTOGRAM_POINTER_HELPER(function, ...) \ #define SK_HISTOGRAM_POINTER_HELPER(function, ...) \
do { \ do { \
static intptr_t atomic_histogram_pointer = 0; \ static std::atomic_uintptr_t atomic_histogram_pointer; \
function(&atomic_histogram_pointer, __VA_ARGS__); \ function(std::addressof(atomic_histogram_pointer), __VA_ARGS__); \
} while (0) } while (0)
#define SK_HISTOGRAM_BOOLEAN(name, sample) \ #define SK_HISTOGRAM_BOOLEAN(name, sample) \
...@@ -27,10 +30,10 @@ ...@@ -27,10 +30,10 @@
namespace skia { namespace skia {
void HistogramBoolean(intptr_t* atomic_histogram_pointer, void HistogramBoolean(std::atomic_uintptr_t* atomic_histogram_pointer,
const char* name, const char* name,
bool sample); bool sample);
void HistogramEnumeration(intptr_t* atomic_histogram_pointer, void HistogramEnumeration(std::atomic_uintptr_t* atomic_histogram_pointer,
const char* name, const char* name,
int sample, int sample,
int boundary_value); int boundary_value);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment