Use bounded ranges (string_view) for histogram names.
This CL changes the histogram creation APIs to take a durable string view (i.e., a pointer+size pair referring to memory that the programmer has annotated that it will not be freed) instead of just an implicit pointer. The HistogramBase class internally represents the name's length and the histogram's flags using 16 bits in order to not grow the in-memory size of Histogram objects. The name of the histogram is subsequently exposed by HistogramBase as a string_view. This CL updates consumers of the histogram name's to use a string_view of the name. This removes many string length calculations and string copy operations. For histograms allocated in shared memory, it also avoids potential out-of- bounds reads if the underlying string data is modified or corrupted such that it no longer has a trailing NUL char at the end of the string. Lastly, this CL updates a number of call-sites where the histogram names were being copied into short-lived string objects for use as search keys into various containers to perform the functionality without making any unnecessary copies. Low-Coverage-Reason: TRIVIAL_CHANGE Use of string_view instead of string/char* in some error/logging paths have low coverage. AX-Relnotes: n/a Bug: 393394360, 40818143 Change-Id: Iee9ee5ae62335b9e7815f4ae11bde4ff80cfd4a2 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6231451 Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Antonio Rivera <antoniori@google.com> Commit-Queue: Roger McFarlane <rogerm@chromium.org> Reviewed-by: Etienne Pierre-Doray <etiennep@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Cr-Commit-Position: refs/heads/main@{#1422750}
This commit is contained in:

committed by
Chromium LUCI CQ

parent
a2d6b9176f
commit
8952859c39
base
BUILD.gn
android
metrics
dummy_histogram.hhistogram.cchistogram.hhistogram_base.cchistogram_base.hhistogram_base_unittest.cchistogram_snapshot_manager_unittest.cchistogram_threadsafe_unittest.ccpersistent_histogram_allocator.ccpersistent_histogram_allocator.hpersistent_histogram_allocator_unittest.ccsparse_histogram.ccsparse_histogram.hsparse_histogram_unittest.ccstatistics_recorder.ccstatistics_recorder.hstatistics_recorder_unittest.cc
strings
test
metrics
chrome/browser
ash
autofill
metrics
notifications
os_crypt
password_manager
privacy_sandbox
segmentation_platform
site_protection
ui
chromecast/base/metrics
chromeos/ash/services/ime/public/mojom
components
fuchsia_legacymetrics
metrics
content
file_metrics_provider_unittest.cchistogram_encoder.cchistogram_encoder.hmetrics_log.ccmetrics_log.hnet
segmentation_platform
internal
value_store
content/browser
devtools
metrics
shared_storage
tracing
ios/chrome
services/tracing/public/cpp
background_tracing
perfetto
third_party/webrtc_overrides
@ -80,7 +80,7 @@ void BackgroundTracingAgentImpl::OnHistogramChanged(
|
||||
const std::string& rule_id,
|
||||
base::Histogram::Sample32 histogram_lower_value,
|
||||
base::Histogram::Sample32 histogram_upper_value,
|
||||
const char* histogram_name,
|
||||
std::string_view histogram_name,
|
||||
uint64_t name_hash,
|
||||
base::Histogram::Sample32 actual_value) {
|
||||
if (actual_value < histogram_lower_value ||
|
||||
|
@ -49,7 +49,7 @@ class COMPONENT_EXPORT(BACKGROUND_TRACING_CPP) BackgroundTracingAgentImpl
|
||||
void OnHistogramChanged(const std::string& rule_id,
|
||||
base::Histogram::Sample32 reference_lower_value,
|
||||
base::Histogram::Sample32 reference_upper_value,
|
||||
const char* histogram_name,
|
||||
std::string_view histogram_name,
|
||||
uint64_t name_hash,
|
||||
base::Histogram::Sample32 actual_value);
|
||||
|
||||
@ -61,7 +61,7 @@ class COMPONENT_EXPORT(BACKGROUND_TRACING_CPP) BackgroundTracingAgentImpl
|
||||
base::Time histogram_last_changed_;
|
||||
// Tracks histogram names and corresponding registered callbacks.
|
||||
std::map<
|
||||
std::string,
|
||||
std::string /*=rule_id*/,
|
||||
std::unique_ptr<base::StatisticsRecorder::ScopedHistogramSampleObserver>>
|
||||
histogram_callback_map_;
|
||||
|
||||
|
@ -177,8 +177,8 @@ void CustomEventRecorder::LogHistogram(base::HistogramBase* histogram) {
|
||||
}
|
||||
base::Pickle pickle;
|
||||
samples->Serialize(&pickle);
|
||||
std::string buckets =
|
||||
base::Base64Encode(std::string(pickle.data_as_char(), pickle.size()));
|
||||
std::string buckets = base::Base64Encode(
|
||||
std::string_view(pickle.data_as_char(), pickle.size()));
|
||||
TRACE_EVENT_INSTANT2("benchmark,uma", "UMAHistogramSamples",
|
||||
TRACE_EVENT_SCOPE_PROCESS, "name",
|
||||
histogram->histogram_name(), "buckets", buckets);
|
||||
|
@ -67,7 +67,7 @@ class COMPONENT_EXPORT(TRACING_CPP) CustomEventRecorder
|
||||
// For each of the Histogram that we are tracking, cache the snapshot of their
|
||||
// HistogramSamples from before tracing began. So that we can calculate the
|
||||
// delta when we go to LogHistograms.
|
||||
std::map<std::string, std::unique_ptr<base::HistogramSamples>>
|
||||
std::map<std::string, std::unique_ptr<base::HistogramSamples>, std::less<>>
|
||||
startup_histogram_samples_;
|
||||
std::vector<std::string> histograms_;
|
||||
base::ActionCallback user_action_callback_ =
|
||||
|
@ -37,13 +37,13 @@ std::optional<base::HistogramBase::Sample32> MaybeReferenceValue(
|
||||
|
||||
struct HistogramSamplesIncrementalState {
|
||||
using InternedHistogramName =
|
||||
perfetto::SmallInternedDataTraits::Index<const char*>;
|
||||
perfetto::SmallInternedDataTraits::Index<std::string_view>;
|
||||
|
||||
bool was_cleared = true;
|
||||
InternedHistogramName histogram_names_;
|
||||
|
||||
size_t GetInternedHistogramName(
|
||||
const char* value,
|
||||
std::string_view value,
|
||||
HistogramSamplesDataSource::TraceContext::TracePacketHandle& packet) {
|
||||
size_t iid;
|
||||
if (histogram_names_.LookUpOrInsert(&iid, value)) {
|
||||
@ -52,7 +52,7 @@ struct HistogramSamplesIncrementalState {
|
||||
auto* interned_data = packet->set_interned_data();
|
||||
auto* msg = interned_data->add_histogram_names();
|
||||
msg->set_iid(iid);
|
||||
msg->set_name(value);
|
||||
msg->set_name(value.data(), value.size());
|
||||
return iid;
|
||||
}
|
||||
};
|
||||
@ -136,7 +136,7 @@ void HistogramSamplesDataSource::OnStop(const StopArgs&) {
|
||||
void HistogramSamplesDataSource::OnMetricSample(
|
||||
std::optional<base::HistogramBase::Sample32> reference_lower_value,
|
||||
std::optional<base::HistogramBase::Sample32> reference_upper_value,
|
||||
const char* histogram_name,
|
||||
std::string_view histogram_name,
|
||||
uint64_t name_hash,
|
||||
base::HistogramBase::Sample32 sample) {
|
||||
if ((reference_lower_value && sample < reference_lower_value) ||
|
||||
@ -148,14 +148,14 @@ void HistogramSamplesDataSource::OnMetricSample(
|
||||
}
|
||||
|
||||
void HistogramSamplesDataSource::OnAnyMetricSample(
|
||||
const char* histogram_name,
|
||||
std::string_view histogram_name,
|
||||
uint64_t name_hash,
|
||||
base::HistogramBase::Sample32 sample) {
|
||||
OnMetricSampleImpl(histogram_name, name_hash, sample, std::nullopt);
|
||||
}
|
||||
|
||||
void HistogramSamplesDataSource::OnMetricSampleImpl(
|
||||
const char* histogram_name,
|
||||
std::string_view histogram_name,
|
||||
uint64_t name_hash,
|
||||
base::HistogramBase::Sample32 sample,
|
||||
std::optional<uintptr_t> instance) {
|
||||
|
@ -5,6 +5,7 @@
|
||||
#ifndef SERVICES_TRACING_PUBLIC_CPP_PERFETTO_HISTOGRAM_SAMPLES_DATA_SOURCE_H_
|
||||
#define SERVICES_TRACING_PUBLIC_CPP_PERFETTO_HISTOGRAM_SAMPLES_DATA_SOURCE_H_
|
||||
|
||||
#include <string_view>
|
||||
#include <vector>
|
||||
|
||||
#include "base/component_export.h"
|
||||
@ -45,15 +46,15 @@ class COMPONENT_EXPORT(TRACING_CPP) HistogramSamplesDataSource
|
||||
void OnMetricSample(
|
||||
std::optional<base::HistogramBase::Sample32> reference_lower_value,
|
||||
std::optional<base::HistogramBase::Sample32> reference_upper_value,
|
||||
const char* histogram_name,
|
||||
std::string_view histogram_name,
|
||||
uint64_t name_hash,
|
||||
base::HistogramBase::Sample32 actual_value);
|
||||
static void OnAnyMetricSample(const char* histogram_name,
|
||||
static void OnAnyMetricSample(std::string_view histogram_name,
|
||||
uint64_t name_hash,
|
||||
base::HistogramBase::Sample32 sample);
|
||||
// `instance` identifies the instance that registered a callback, or nullopt
|
||||
// if this is a global callback.
|
||||
static void OnMetricSampleImpl(const char* histogram_name,
|
||||
static void OnMetricSampleImpl(std::string_view histogram_name,
|
||||
uint64_t name_hash,
|
||||
base::HistogramBase::Sample32 sample,
|
||||
std::optional<uintptr_t> instance);
|
||||
|
Reference in New Issue
Block a user