From 1055e187cc61c01996774bcc681d26ea43cac089 Mon Sep 17 00:00:00 2001 From: "Daniel R. Carvalho" Date: Sat, 15 Feb 2020 14:54:21 +0100 Subject: [PATCH] base: Initialize storage params on constructor Force error checking on storage params by imposing it on construction. Change-Id: I66a902cd5a7c809d3ac5be65b406de29fc0acf1c Signed-off-by: Daniel R. Carvalho Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/25426 Tested-by: kokoro Reviewed-by: Bobby R. Bruce Maintainer: Bobby R. Bruce --- src/base/statistics.hh | 18 +---- src/base/stats/storage.cc | 5 +- src/base/stats/storage.hh | 26 +++++-- src/base/stats/storage.test.cc | 131 ++++++++++----------------------- 4 files changed, 64 insertions(+), 116 deletions(-) diff --git a/src/base/statistics.hh b/src/base/statistics.hh index b1f27c726..a1994c843 100644 --- a/src/base/statistics.hh +++ b/src/base/statistics.hh @@ -2005,14 +2005,7 @@ class Distribution : public DistBase Distribution & init(Counter min, Counter max, Counter bkt) { - DistStor::Params *params = new DistStor::Params; - params->min = min; - params->max = max; - params->bucket_size = bkt; - // Division by zero is especially serious in an Aarch64 host, - // where it gets rounded to allocate 32GiB RAM. - assert(bkt > 0); - params->buckets = (size_type)ceil((max - min + 1.0) / bkt); + DistStor::Params *params = new DistStor::Params(min, max, bkt); this->setParams(params); this->doInit(); return this->self(); @@ -2040,8 +2033,7 @@ class Histogram : public DistBase Histogram & init(size_type size) { - HistStor::Params *params = new HistStor::Params; - params->buckets = size; + HistStor::Params *params = new HistStor::Params(size); this->setParams(params); this->doInit(); return this->self(); @@ -2112,11 +2104,7 @@ class VectorDistribution : public VectorDistBase VectorDistribution & init(size_type size, Counter min, Counter max, Counter bkt) { - DistStor::Params *params = new DistStor::Params; - params->min = min; - params->max = max; - params->bucket_size = bkt; - params->buckets = (size_type)ceil((max - min + 1.0) / bkt); + DistStor::Params *params = new DistStor::Params(min, max, bkt); this->setParams(params); this->doInit(size); return this->self(); diff --git a/src/base/stats/storage.cc b/src/base/stats/storage.cc index 02e871649..6e2a7f2f9 100644 --- a/src/base/stats/storage.cc +++ b/src/base/stats/storage.cc @@ -54,10 +54,7 @@ DistStor::sample(Counter val, int number) else if (val > max_track) overflow += number; else { - size_type index = - (size_type)std::floor((val - min_track) / bucket_size); - assert(index < size()); - cvec[index] += number; + cvec[std::floor((val - min_track) / bucket_size)] += number; } if (val < min_val) diff --git a/src/base/stats/storage.hh b/src/base/stats/storage.hh index 66565bbd9..2ee1eb2f1 100644 --- a/src/base/stats/storage.hh +++ b/src/base/stats/storage.hh @@ -31,6 +31,7 @@ #define __BASE_STATS_STORAGE_HH__ #include +#include #include "base/cast.hh" #include "base/logging.hh" @@ -265,8 +266,19 @@ class DistStor /** The number of buckets. Equal to (max-min)/bucket_size. */ size_type buckets; - Params() : DistParams(Dist), min(0), max(0), bucket_size(0), - buckets(0) {} + Params(Counter _min, Counter _max, Counter _bucket_size) + : DistParams(Dist), min(_min), max(_max), bucket_size(_bucket_size), + buckets(0) + { + fatal_if(bucket_size <= 0, + "Bucket size (%f) must be greater than zero", bucket_size); + warn_if(std::floor((max - min + 1.0) / bucket_size) != + std::ceil((max - min + 1.0) / bucket_size), + "Bucket size (%f) does not divide range [%f:%f] into equal-" \ + "sized buckets. Rounding up.", bucket_size, min + 1.0, max); + + buckets = std::ceil((max - min + 1.0) / bucket_size); + } }; DistStor(Info *info) @@ -446,14 +458,18 @@ class HistStor /** The number of buckets. */ size_type buckets; - Params() : DistParams(Hist), buckets(0) {} + Params(size_type _buckets) + : DistParams(Hist) + { + fatal_if(_buckets < 2, + "There must be at least two buckets in a histogram"); + buckets = _buckets; + } }; HistStor(Info *info) : cvec(safe_cast(info->storageParams)->buckets) { - fatal_if(cvec.size() == 1, - "There must be at least two buckets in a histogram"); reset(info); } diff --git a/src/base/stats/storage.test.cc b/src/base/stats/storage.test.cc index 8a4f6ed5e..78df23d06 100644 --- a/src/base/stats/storage.test.cc +++ b/src/base/stats/storage.test.cc @@ -266,18 +266,12 @@ TEST(StatsAvgStorTest, ZeroReset) ASSERT_FALSE(stor.zero()); } -/** - * Test that an assertion is thrown when no bucket size is provided before - * sampling. - */ -TEST(StatsDistStorDeathTest, NoBucketSize) +/** Test that an assertion is thrown when bucket size is 0. */ +TEST(StatsDistStorDeathTest, BucketSize0) { - Stats::Counter val = 10; - Stats::Counter num_samples = 5; - Stats::DistStor::Params params; - MockInfo info(¶ms); - Stats::DistStor stor(&info); - ASSERT_DEATH(stor.sample(val, num_samples), ".+"); + testing::internal::CaptureStderr(); + EXPECT_ANY_THROW(Stats::DistStor::Params params(0, 5, 0)); + testing::internal::GetCapturedStderr(); } /** @@ -287,8 +281,7 @@ TEST(StatsDistStorDeathTest, NoBucketSize) */ TEST(StatsDistStorTest, ZeroReset) { - Stats::DistStor::Params params; - params.bucket_size = 10; + Stats::DistStor::Params params(0, 99, 10); MockInfo info(¶ms); Stats::DistStor stor(&info); Stats::Counter val = 10; @@ -315,9 +308,7 @@ TEST(StatsDistStorTest, Size) Stats::Counter size = 20; Stats::DistData data; - Stats::DistStor::Params params; - params.bucket_size = 1; - params.buckets = size; + Stats::DistStor::Params params(0, 19, 1); MockInfo info(¶ms); Stats::DistStor stor(&info); @@ -406,11 +397,7 @@ prepareCheckDistStor(Stats::DistStor::Params& params, ValueSamples* values, /** Test setting and getting value from storage. */ TEST(StatsDistStorTest, SamplePrepareSingle) { - Stats::DistStor::Params params; - params.min = 0; - params.max = 99; - params.bucket_size = 5; - params.buckets = 20; + Stats::DistStor::Params params(0, 99, 5); ValueSamples values[] = {{10, 5}}; int num_values = sizeof(values) / sizeof(ValueSamples); @@ -433,11 +420,7 @@ TEST(StatsDistStorTest, SamplePrepareSingle) /** Test setting and getting value from storage with multiple values. */ TEST(StatsDistStorTest, SamplePrepareMultiple) { - Stats::DistStor::Params params; - params.min = 0; - params.max = 99; - params.bucket_size = 5; - params.buckets = 20; + Stats::DistStor::Params params(0, 99, 5); // There are 20 buckets: [0,5[, [5,10[, [10,15[, ..., [95,100[. // We test that values that pass the maximum bucket value (1234, 12345678, @@ -474,11 +457,7 @@ TEST(StatsDistStorTest, SamplePrepareMultiple) /** Test resetting storage. */ TEST(StatsDistStorTest, Reset) { - Stats::DistStor::Params params; - params.min = 0; - params.max = 99; - params.bucket_size = 5; - params.buckets = 20; + Stats::DistStor::Params params(0, 99, 5); MockInfo info(¶ms); Stats::DistStor stor(&info); @@ -513,20 +492,20 @@ TEST(StatsDistStorTest, Reset) checkExpectedDistData(data, expected_data, true); } -/** - * Test that an assertion is thrown when no bucket size is provided before - * sampling. - */ -TEST(StatsHistStorDeathTest, NoBucketSize) +/** Test that an assertion is thrown when not enough buckets are provided. */ +TEST(StatsHistStorDeathTest, NotEnoughBuckets0) { - Stats::Counter val = 10; - Stats::Counter num_samples = 5; + testing::internal::CaptureStderr(); + EXPECT_ANY_THROW(Stats::HistStor::Params params(0)); + testing::internal::GetCapturedStderr(); +} - // If no bucket size is specified, it is 0 by default - Stats::HistStor::Params params; - MockInfo info(¶ms); - Stats::HistStor stor(&info); - ASSERT_DEATH(stor.sample(val, num_samples), ".+"); +/** Test that an assertion is thrown when not enough buckets are provided. */ +TEST(StatsHistStorDeathTest, NotEnoughBuckets1) +{ + testing::internal::CaptureStderr(); + EXPECT_ANY_THROW(Stats::HistStor::Params params(1)); + testing::internal::GetCapturedStderr(); } /** @@ -536,8 +515,7 @@ TEST(StatsHistStorDeathTest, NoBucketSize) */ TEST(StatsHistStorTest, ZeroReset) { - Stats::HistStor::Params params; - params.buckets = 10; + Stats::HistStor::Params params(10); MockInfo info(¶ms); Stats::HistStor stor(&info); Stats::Counter val = 10; @@ -564,24 +542,8 @@ TEST(StatsHistStorTest, Size) Stats::DistData data; Stats::size_type sizes[] = {2, 10, 1234}; - // If no bucket size is specified, it is 0 by default - { - Stats::HistStor::Params params; - MockInfo info(¶ms); - Stats::HistStor stor(&info); - - ASSERT_EQ(stor.size(), 0); - stor.prepare(&info, data); - ASSERT_EQ(stor.size(), 0); - stor.reset(&info); - ASSERT_EQ(stor.size(), 0); - stor.zero(); - ASSERT_EQ(stor.size(), 0); - } - for (int i = 0; i < (sizeof(sizes) / sizeof(Stats::size_type)); i++) { - Stats::HistStor::Params params; - params.buckets = sizes[i]; + Stats::HistStor::Params params(sizes[i]); MockInfo info(¶ms); Stats::HistStor stor(&info); @@ -651,8 +613,7 @@ prepareCheckHistStor(Stats::HistStor::Params& params, ValueSamples* values, */ TEST(StatsHistStorTest, SamplePrepareFit) { - Stats::HistStor::Params params; - params.buckets = 4; + Stats::HistStor::Params params(4); // Setup expected data for the hand-carved values given. The final buckets // will be divided at: @@ -680,8 +641,7 @@ TEST(StatsHistStorTest, SamplePrepareFit) */ TEST(StatsHistStorTest, SamplePrepareSingleGrowUp) { - Stats::HistStor::Params params; - params.buckets = 4; + Stats::HistStor::Params params(4); // Setup expected data for the hand-carved values given. Since there // are four buckets, and the highest value is 4, the bucket size will @@ -710,8 +670,7 @@ TEST(StatsHistStorTest, SamplePrepareSingleGrowUp) */ TEST(StatsHistStorTest, SamplePrepareMultipleGrowUp) { - Stats::HistStor::Params params; - params.buckets = 4; + Stats::HistStor::Params params(4); // Setup expected data for the hand-carved values given. Since there // are four buckets, and the highest value is 4, the bucket size will @@ -741,8 +700,7 @@ TEST(StatsHistStorTest, SamplePrepareMultipleGrowUp) */ TEST(StatsHistStorTest, SamplePrepareGrowDownOddBuckets) { - Stats::HistStor::Params params; - params.buckets = 5; + Stats::HistStor::Params params(5); // Setup expected data for the hand-carved values given. Since there // is a negative value, the min bucket will change, and the bucket size @@ -774,8 +732,7 @@ TEST(StatsHistStorTest, SamplePrepareGrowDownOddBuckets) */ TEST(StatsHistStorTest, SamplePrepareGrowDownEvenBuckets) { - Stats::HistStor::Params params; - params.buckets = 4; + Stats::HistStor::Params params(4); // Setup expected data for the hand-carved values given. Since there // is a negative value, the min bucket will change, and the bucket size @@ -805,8 +762,7 @@ TEST(StatsHistStorTest, SamplePrepareGrowDownEvenBuckets) */ TEST(StatsHistStorTest, SamplePrepareGrowDownGrowOutOddBuckets) { - Stats::HistStor::Params params; - params.buckets = 5; + Stats::HistStor::Params params(5); // Setup expected data for the hand-carved values given. Since there // is a negative value, the min bucket will change, and the bucket size @@ -838,8 +794,7 @@ TEST(StatsHistStorTest, SamplePrepareGrowDownGrowOutOddBuckets) */ TEST(StatsHistStorTest, SamplePrepareGrowDownGrowOutEvenBuckets) { - Stats::HistStor::Params params; - params.buckets = 4; + Stats::HistStor::Params params(4); // Setup expected data for the hand-carved values given. Since there // is a negative value, the min bucket will change, and the bucket size @@ -870,8 +825,7 @@ TEST(StatsHistStorTest, SamplePrepareGrowDownGrowOutEvenBuckets) */ TEST(StatsHistStorTest, SamplePrepareMultipleGrowOddBuckets) { - Stats::HistStor::Params params; - params.buckets = 5; + Stats::HistStor::Params params(5); // Setup expected data for the hand-carved values given. This adds quite // a few positive and negative samples, and the bucket size will grow to @@ -904,8 +858,7 @@ TEST(StatsHistStorTest, SamplePrepareMultipleGrowOddBuckets) */ TEST(StatsHistStorTest, SamplePrepareMultipleGrowEvenBuckets) { - Stats::HistStor::Params params; - params.buckets = 4; + Stats::HistStor::Params params(4); // Setup expected data for the hand-carved values given. This adds quite // a few positive and negative samples, and the bucket size will grow to @@ -932,8 +885,7 @@ TEST(StatsHistStorTest, SamplePrepareMultipleGrowEvenBuckets) /** Test resetting storage. */ TEST(StatsHistStorTest, Reset) { - Stats::HistStor::Params params; - params.buckets = 4; + Stats::HistStor::Params params(4); MockInfo info(¶ms); Stats::HistStor stor(&info); @@ -964,13 +916,11 @@ TEST(StatsHistStorTest, Reset) /** Test whether adding storages with different sizes triggers an assertion. */ TEST(StatsHistStorDeathTest, AddDifferentSize) { - Stats::HistStor::Params params; - params.buckets = 4; + Stats::HistStor::Params params(4); MockInfo info(¶ms); Stats::HistStor stor(&info); - Stats::HistStor::Params params2; - params2.buckets = 5; + Stats::HistStor::Params params2(5); MockInfo info2(¶ms2); Stats::HistStor stor2(&info2); @@ -980,15 +930,13 @@ TEST(StatsHistStorDeathTest, AddDifferentSize) /** Test whether adding storages with different min triggers an assertion. */ TEST(StatsHistStorDeathTest, AddDifferentMin) { - Stats::HistStor::Params params; - params.buckets = 4; + Stats::HistStor::Params params(4); MockInfo info(¶ms); Stats::HistStor stor(&info); stor.sample(-1, 3); // On creation, the storage's min is zero - Stats::HistStor::Params params2; - params2.buckets = 4; + Stats::HistStor::Params params2(4); MockInfo info2(¶ms2); Stats::HistStor stor2(&info2); @@ -998,8 +946,7 @@ TEST(StatsHistStorDeathTest, AddDifferentMin) /** Test merging two histograms. */ TEST(StatsHistStorTest, Add) { - Stats::HistStor::Params params; - params.buckets = 4; + Stats::HistStor::Params params(4); MockInfo info(¶ms); // Setup first storage. Buckets are: -- 2.30.2