Daniel Carvalho has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/25426 )
Change subject: base: Initialize storage params on constructor
......................................................................
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 <oda...@yahoo.com.br>
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/25426
Tested-by: kokoro <noreply+kok...@google.com>
Reviewed-by: Bobby R. Bruce <bbr...@ucdavis.edu>
Maintainer: Bobby R. Bruce <bbr...@ucdavis.edu>
---
M src/base/statistics.hh
M src/base/stats/storage.cc
M src/base/stats/storage.hh
M src/base/stats/storage.test.cc
4 files changed, 64 insertions(+), 116 deletions(-)
Approvals:
Bobby R. Bruce: Looks good to me, approved; Looks good to me, approved
kokoro: Regressions pass
diff --git a/src/base/statistics.hh b/src/base/statistics.hh
index b1f27c7..a1994c8 100644
--- a/src/base/statistics.hh
+++ b/src/base/statistics.hh
@@ -2005,14 +2005,7 @@
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 @@
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 @@
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 02e8716..6e2a7f2 100644
--- a/src/base/stats/storage.cc
+++ b/src/base/stats/storage.cc
@@ -54,10 +54,7 @@
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 66565bb..2ee1eb2 100644
--- a/src/base/stats/storage.hh
+++ b/src/base/stats/storage.hh
@@ -31,6 +31,7 @@
#define __BASE_STATS_STORAGE_HH__
#include <cassert>
+#include <cmath>
#include "base/cast.hh"
#include "base/logging.hh"
@@ -265,8 +266,19 @@
/** 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 @@
/** 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<const Params *>(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 8a4f6ed..78df23d 100644
--- a/src/base/stats/storage.test.cc
+++ b/src/base/stats/storage.test.cc
@@ -266,18 +266,12 @@
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(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 @@
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 @@
/** 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 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 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 @@
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(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 @@
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 @@
*/
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, 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, 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, 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, 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, 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, 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, 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, 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 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 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 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 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:
The change was submitted with unreviewed changes in the following files:
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/25426
To unsubscribe, or for help writing mail filters, visit
https://gem5-review.googlesource.com/settings
Gerrit-Project: public/gem5
Gerrit-Branch: develop
Gerrit-Change-Id: I66a902cd5a7c809d3ac5be65b406de29fc0acf1c
Gerrit-Change-Number: 25426
Gerrit-PatchSet: 15
Gerrit-Owner: Daniel Carvalho <oda...@yahoo.com.br>
Gerrit-Reviewer: Alexandru Duțu <alexandru.d...@amd.com>
Gerrit-Reviewer: Bobby R. Bruce <bbr...@ucdavis.edu>
Gerrit-Reviewer: Daniel Carvalho <oda...@yahoo.com.br>
Gerrit-Reviewer: Jason Lowe-Power <power...@gmail.com>
Gerrit-Reviewer: kokoro <noreply+kok...@google.com>
Gerrit-MessageType: merged
_______________________________________________
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
