Commit 4c009ba6 authored by Wez's avatar Wez Committed by Commit Bot

Revert "Simplified and optimized StatisticsRecorder."

This reverts commit b86ebc88.

Reason for revert: Introduces flakiness due to removal of global StatisticsRecorder teardown - see crbug.com/798717. This is a cross-platyform issue but in practice only caused base_unittests failures on the Fuchsia FYI bots, presumably due to differences in test ordering.

Original change's description:
> Simplified and optimized StatisticsRecorder.
> 
> StatisticsRecorder is now an actual container of collections.
> 
> There is only one global recorder at any time, referenced by
> StatisticsRecorder::top_.
> 
> Each StatisticsRecorder also has a pointer to the previous global recorder.
> 
> We now have a stack (singly linked list) of StatisticsRecorder objects, where
> the top of the stack (StatisticsRecorder::top_) is the current global recorder.
> 
> This pattern is clearer and cleaner. It avoids a bunch of static variables,
> indirections, unique_ptrs and calls to operator new. It gets rid of the ugly
> hacks of UninitializeForTesting. It reduces memory fragmentation by having fewer
> dynamically allocated objects.
> 
> Removed methods Reset, UninitializeForTesting and DumpHistogramsToVlog. They are
> now unnecessary.
> 
> Reviewed, clarified and documented the thread safety status of each method.
> 
> Added comments.
> 
> 
> Change-Id: Ia1a7611905009d0449068c801464ad0df7813c0d
> Bug: 
> Reviewed-on: https://chromium-review.googlesource.com/830997
> Commit-Queue: François Degros <fdegros@chromium.org>
> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#526570}

TBR=asvitkine@chromium.org,gayane@chromium.org,holte@chromium.org,skare@chromium.org,fdegros@chromium.org

Change-Id: I4bd60c636a821bf681eb452636ade1098d3a4899
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/848273Reviewed-by: default avatarWez <wez@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526718}
parent e8955cf2
This diff is collapsed.
This diff is collapsed.
...@@ -30,7 +30,9 @@ class LogStateSaver { ...@@ -30,7 +30,9 @@ class LogStateSaver {
public: public:
LogStateSaver() : old_min_log_level_(logging::GetMinLogLevel()) {} LogStateSaver() : old_min_log_level_(logging::GetMinLogLevel()) {}
~LogStateSaver() { logging::SetMinLogLevel(old_min_log_level_); } ~LogStateSaver() {
logging::SetMinLogLevel(old_min_log_level_);
}
private: private:
int old_min_log_level_; int old_min_log_level_;
...@@ -68,8 +70,8 @@ class StatisticsRecorderTest : public testing::TestWithParam<bool> { ...@@ -68,8 +70,8 @@ class StatisticsRecorderTest : public testing::TestWithParam<bool> {
// Use persistent memory for histograms if so indicated by test parameter. // Use persistent memory for histograms if so indicated by test parameter.
if (use_persistent_histogram_allocator_) { if (use_persistent_histogram_allocator_) {
GlobalHistogramAllocator::CreateWithLocalMemory(kAllocatorMemorySize, 0, GlobalHistogramAllocator::CreateWithLocalMemory(
"StatisticsRecorderTest"); kAllocatorMemorySize, 0, "StatisticsRecorderTest");
} }
} }
...@@ -80,10 +82,14 @@ class StatisticsRecorderTest : public testing::TestWithParam<bool> { ...@@ -80,10 +82,14 @@ class StatisticsRecorderTest : public testing::TestWithParam<bool> {
void InitializeStatisticsRecorder() { void InitializeStatisticsRecorder() {
DCHECK(!statistics_recorder_); DCHECK(!statistics_recorder_);
StatisticsRecorder::UninitializeForTesting();
statistics_recorder_ = StatisticsRecorder::CreateTemporaryForTesting(); statistics_recorder_ = StatisticsRecorder::CreateTemporaryForTesting();
} }
void UninitializeStatisticsRecorder() { statistics_recorder_.reset(); } void UninitializeStatisticsRecorder() {
statistics_recorder_.reset();
StatisticsRecorder::UninitializeForTesting();
}
Histogram* CreateHistogram(const char* name, Histogram* CreateHistogram(const char* name,
HistogramBase::Sample min, HistogramBase::Sample min,
...@@ -96,15 +102,18 @@ class StatisticsRecorderTest : public testing::TestWithParam<bool> { ...@@ -96,15 +102,18 @@ class StatisticsRecorderTest : public testing::TestWithParam<bool> {
return new Histogram(name, min, max, registered_ranges); return new Histogram(name, min, max, registered_ranges);
} }
void DeleteHistogram(HistogramBase* histogram) { delete histogram; } void DeleteHistogram(HistogramBase* histogram) {
delete histogram;
void InitLogOnShutdown() { StatisticsRecorder::InitLogOnShutdown(); } }
bool IsVLogInitialized() { return StatisticsRecorder::is_vlog_initialized_; } void InitLogOnShutdown() {
DCHECK(statistics_recorder_);
statistics_recorder_->InitLogOnShutdownWithoutLock();
}
void ResetVLogInitialized() { bool VLogInitialized() {
UninitializeStatisticsRecorder(); DCHECK(statistics_recorder_);
StatisticsRecorder::is_vlog_initialized_ = false; return statistics_recorder_->vlog_initialized_;
} }
const bool use_persistent_histogram_allocator_; const bool use_persistent_histogram_allocator_;
...@@ -268,8 +277,8 @@ TEST_P(StatisticsRecorderTest, RegisterHistogramWithFactoryGet) { ...@@ -268,8 +277,8 @@ TEST_P(StatisticsRecorderTest, RegisterHistogramWithFactoryGet) {
ASSERT_EQ(0u, registered_histograms.size()); ASSERT_EQ(0u, registered_histograms.size());
// Create a histogram. // Create a histogram.
HistogramBase* histogram = Histogram::FactoryGet("TestHistogram", 1, 1000, 10, HistogramBase* histogram = Histogram::FactoryGet(
HistogramBase::kNoFlags); "TestHistogram", 1, 1000, 10, HistogramBase::kNoFlags);
registered_histograms.clear(); registered_histograms.clear();
StatisticsRecorder::GetHistograms(&registered_histograms); StatisticsRecorder::GetHistograms(&registered_histograms);
EXPECT_EQ(1u, registered_histograms.size()); EXPECT_EQ(1u, registered_histograms.size());
...@@ -283,15 +292,15 @@ TEST_P(StatisticsRecorderTest, RegisterHistogramWithFactoryGet) { ...@@ -283,15 +292,15 @@ TEST_P(StatisticsRecorderTest, RegisterHistogramWithFactoryGet) {
EXPECT_EQ(histogram, histogram2); EXPECT_EQ(histogram, histogram2);
// Create a LinearHistogram. // Create a LinearHistogram.
histogram = LinearHistogram::FactoryGet("TestLinearHistogram", 1, 1000, 10, histogram = LinearHistogram::FactoryGet(
HistogramBase::kNoFlags); "TestLinearHistogram", 1, 1000, 10, HistogramBase::kNoFlags);
registered_histograms.clear(); registered_histograms.clear();
StatisticsRecorder::GetHistograms(&registered_histograms); StatisticsRecorder::GetHistograms(&registered_histograms);
EXPECT_EQ(2u, registered_histograms.size()); EXPECT_EQ(2u, registered_histograms.size());
// Create a BooleanHistogram. // Create a BooleanHistogram.
histogram = BooleanHistogram::FactoryGet("TestBooleanHistogram", histogram = BooleanHistogram::FactoryGet(
HistogramBase::kNoFlags); "TestBooleanHistogram", HistogramBase::kNoFlags);
registered_histograms.clear(); registered_histograms.clear();
StatisticsRecorder::GetHistograms(&registered_histograms); StatisticsRecorder::GetHistograms(&registered_histograms);
EXPECT_EQ(3u, registered_histograms.size()); EXPECT_EQ(3u, registered_histograms.size());
...@@ -300,8 +309,8 @@ TEST_P(StatisticsRecorderTest, RegisterHistogramWithFactoryGet) { ...@@ -300,8 +309,8 @@ TEST_P(StatisticsRecorderTest, RegisterHistogramWithFactoryGet) {
std::vector<int> custom_ranges; std::vector<int> custom_ranges;
custom_ranges.push_back(1); custom_ranges.push_back(1);
custom_ranges.push_back(5); custom_ranges.push_back(5);
histogram = CustomHistogram::FactoryGet("TestCustomHistogram", custom_ranges, histogram = CustomHistogram::FactoryGet(
HistogramBase::kNoFlags); "TestCustomHistogram", custom_ranges, HistogramBase::kNoFlags);
registered_histograms.clear(); registered_histograms.clear();
StatisticsRecorder::GetHistograms(&registered_histograms); StatisticsRecorder::GetHistograms(&registered_histograms);
EXPECT_EQ(4u, registered_histograms.size()); EXPECT_EQ(4u, registered_histograms.size());
...@@ -615,33 +624,33 @@ TEST_P(StatisticsRecorderTest, CallbackUsedBeforeHistogramCreatedTest) { ...@@ -615,33 +624,33 @@ TEST_P(StatisticsRecorderTest, CallbackUsedBeforeHistogramCreatedTest) {
} }
TEST_P(StatisticsRecorderTest, LogOnShutdownNotInitialized) { TEST_P(StatisticsRecorderTest, LogOnShutdownNotInitialized) {
ResetVLogInitialized(); UninitializeStatisticsRecorder();
logging::SetMinLogLevel(logging::LOG_WARNING); logging::SetMinLogLevel(logging::LOG_WARNING);
InitializeStatisticsRecorder(); InitializeStatisticsRecorder();
EXPECT_FALSE(VLOG_IS_ON(1)); EXPECT_FALSE(VLOG_IS_ON(1));
EXPECT_FALSE(IsVLogInitialized()); EXPECT_FALSE(VLogInitialized());
InitLogOnShutdown(); InitLogOnShutdown();
EXPECT_FALSE(IsVLogInitialized()); EXPECT_FALSE(VLogInitialized());
} }
TEST_P(StatisticsRecorderTest, LogOnShutdownInitializedExplicitly) { TEST_P(StatisticsRecorderTest, LogOnShutdownInitializedExplicitly) {
ResetVLogInitialized(); UninitializeStatisticsRecorder();
logging::SetMinLogLevel(logging::LOG_WARNING); logging::SetMinLogLevel(logging::LOG_WARNING);
InitializeStatisticsRecorder(); InitializeStatisticsRecorder();
EXPECT_FALSE(VLOG_IS_ON(1)); EXPECT_FALSE(VLOG_IS_ON(1));
EXPECT_FALSE(IsVLogInitialized()); EXPECT_FALSE(VLogInitialized());
logging::SetMinLogLevel(logging::LOG_VERBOSE); logging::SetMinLogLevel(logging::LOG_VERBOSE);
EXPECT_TRUE(VLOG_IS_ON(1)); EXPECT_TRUE(VLOG_IS_ON(1));
InitLogOnShutdown(); InitLogOnShutdown();
EXPECT_TRUE(IsVLogInitialized()); EXPECT_TRUE(VLogInitialized());
} }
TEST_P(StatisticsRecorderTest, LogOnShutdownInitialized) { TEST_P(StatisticsRecorderTest, LogOnShutdownInitialized) {
ResetVLogInitialized(); UninitializeStatisticsRecorder();
logging::SetMinLogLevel(logging::LOG_VERBOSE); logging::SetMinLogLevel(logging::LOG_VERBOSE);
InitializeStatisticsRecorder(); InitializeStatisticsRecorder();
EXPECT_TRUE(VLOG_IS_ON(1)); EXPECT_TRUE(VLOG_IS_ON(1));
EXPECT_TRUE(IsVLogInitialized()); EXPECT_TRUE(VLogInitialized());
} }
class TestHistogramProvider : public StatisticsRecorder::HistogramProvider { class TestHistogramProvider : public StatisticsRecorder::HistogramProvider {
......
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