Skip to content

Commit 05774b2

Browse files
committed
fix memory manager result bug
1 parent ff5c94d commit 05774b2

File tree

6 files changed

+119
-19
lines changed

6 files changed

+119
-19
lines changed

include/benchmark/benchmark.h

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -394,7 +394,8 @@ class MemoryManager {
394394
: num_allocs(0),
395395
max_bytes_used(0),
396396
total_allocated_bytes(TombstoneValue),
397-
net_heap_growth(TombstoneValue) {}
397+
net_heap_growth(TombstoneValue),
398+
is_valid(false) {}
398399

399400
// The number of allocations made in total between Start and Stop.
400401
int64_t num_allocs;
@@ -410,6 +411,8 @@ class MemoryManager {
410411
// ie., total_allocated_bytes - total_deallocated_bytes.
411412
// Init'ed to TombstoneValue if metric not available.
412413
int64_t net_heap_growth;
414+
415+
bool is_valid;
413416
};
414417

415418
virtual ~MemoryManager() {}
@@ -1721,7 +1724,6 @@ class BENCHMARK_EXPORT BenchmarkReporter {
17211724
complexity_n(0),
17221725
report_big_o(false),
17231726
report_rms(false),
1724-
memory_result(NULL),
17251727
allocs_per_iter(0.0) {}
17261728

17271729
std::string benchmark_name() const;
@@ -1777,7 +1779,7 @@ class BENCHMARK_EXPORT BenchmarkReporter {
17771779
UserCounters counters;
17781780

17791781
// Memory metrics.
1780-
const MemoryManager::Result* memory_result;
1782+
MemoryManager::Result memory_result;
17811783
double allocs_per_iter;
17821784
};
17831785

src/benchmark_runner.cc

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ BenchmarkReporter::Run CreateRunReport(
8181
const benchmark::internal::BenchmarkInstance& b,
8282
const internal::ThreadManager::Result& results,
8383
IterationCount memory_iterations,
84-
const MemoryManager::Result* memory_result, double seconds,
84+
const MemoryManager::Result& memory_result, double seconds,
8585
int64_t repetition_index, int64_t repeats) {
8686
// Create report about this benchmark run.
8787
BenchmarkReporter::Run report;
@@ -114,11 +114,11 @@ BenchmarkReporter::Run CreateRunReport(
114114
report.counters = results.counters;
115115

116116
if (memory_iterations > 0) {
117-
assert(memory_result != nullptr);
117+
assert(memory_result.is_valid);
118118
report.memory_result = memory_result;
119119
report.allocs_per_iter =
120120
memory_iterations != 0
121-
? static_cast<double>(memory_result->num_allocs) /
121+
? static_cast<double>(memory_result.num_allocs) /
122122
static_cast<double>(memory_iterations)
123123
: 0;
124124
}
@@ -426,13 +426,8 @@ void BenchmarkRunner::RunWarmUp() {
426426
}
427427
}
428428

429-
MemoryManager::Result* BenchmarkRunner::RunMemoryManager(
429+
MemoryManager::Result BenchmarkRunner::RunMemoryManager(
430430
IterationCount memory_iterations) {
431-
// TODO(vyng): Consider making BenchmarkReporter::Run::memory_result an
432-
// optional so we don't have to own the Result here.
433-
// Can't do it now due to cxx03.
434-
memory_results.push_back(MemoryManager::Result());
435-
MemoryManager::Result* memory_result = &memory_results.back();
436431
memory_manager->Start();
437432
std::unique_ptr<internal::ThreadManager> manager;
438433
manager.reset(new internal::ThreadManager(1));
@@ -443,7 +438,8 @@ MemoryManager::Result* BenchmarkRunner::RunMemoryManager(
443438
manager->WaitForAllThreads();
444439
manager.reset();
445440
b.Teardown();
446-
memory_manager->Stop(*memory_result);
441+
MemoryManager::Result memory_result;
442+
memory_manager->Stop(memory_result);
447443
return memory_result;
448444
}
449445

@@ -508,13 +504,14 @@ void BenchmarkRunner::DoOneRepetition() {
508504
}
509505

510506
// Produce memory measurements if requested.
511-
MemoryManager::Result* memory_result = nullptr;
507+
MemoryManager::Result memory_result;
512508
IterationCount memory_iterations = 0;
513509
if (memory_manager != nullptr) {
514510
// Only run a few iterations to reduce the impact of one-time
515511
// allocations in benchmarks that are not properly managed.
516512
memory_iterations = std::min<IterationCount>(16, iters);
517513
memory_result = RunMemoryManager(memory_iterations);
514+
memory_result.is_valid = true;
518515
}
519516

520517
if (profiler_manager != nullptr) {

src/benchmark_runner.h

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -91,8 +91,6 @@ class BenchmarkRunner {
9191

9292
std::vector<std::thread> pool;
9393

94-
std::vector<MemoryManager::Result> memory_results;
95-
9694
IterationCount iters; // preserved between repetitions!
9795
// So only the first repetition has to find/calculate it,
9896
// the other repetitions will just use that precomputed iteration count.
@@ -106,7 +104,7 @@ class BenchmarkRunner {
106104
};
107105
IterationResults DoNIterations();
108106

109-
MemoryManager::Result* RunMemoryManager(IterationCount memory_iterations);
107+
MemoryManager::Result RunMemoryManager(IterationCount memory_iterations);
110108

111109
void RunProfilerManager(IterationCount profile_iterations);
112110

src/json_reporter.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -306,8 +306,8 @@ void JSONReporter::PrintRunData(Run const& run) {
306306
out << ",\n" << indent << FormatKV(c.first, c.second);
307307
}
308308

309-
if (run.memory_result != nullptr) {
310-
const MemoryManager::Result memory_result = *run.memory_result;
309+
if (run.memory_result.is_valid) {
310+
const MemoryManager::Result& memory_result{run.memory_result};
311311
out << ",\n" << indent << FormatKV("allocs_per_iter", run.allocs_per_iter);
312312
out << ",\n"
313313
<< indent << FormatKV("max_bytes_used", memory_result.max_bytes_used);

test/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,7 @@ if (BENCHMARK_ENABLE_GTEST_TESTS)
233233
add_gtest(min_time_parse_gtest)
234234
add_gtest(profiler_manager_gtest)
235235
add_gtest(benchmark_setup_teardown_cb_types_gtest)
236+
add_gtest(memory_results_gtest)
236237
endif(BENCHMARK_ENABLE_GTEST_TESTS)
237238

238239
###############################################################################

test/memory_results_gtest.cc

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
#include <vector>
2+
3+
#include "../src/benchmark_runner.h"
4+
#include "benchmark/benchmark.h"
5+
#include "gtest/gtest.h"
6+
7+
#define N_REPETITIONS 100
8+
#define N_ITERATIONS 1
9+
10+
using benchmark::ClearRegisteredBenchmarks;
11+
using benchmark::ConsoleReporter;
12+
using benchmark::MemoryManager;
13+
using benchmark::RegisterBenchmark;
14+
using benchmark::RunSpecifiedBenchmarks;
15+
using benchmark::State;
16+
using benchmark::internal::Benchmark;
17+
18+
namespace counts {
19+
int num_allocs = 0;
20+
int max_bytes_used = 0;
21+
int total_allocated_bytes = 0;
22+
int net_heap_growth = 0;
23+
void reset() {
24+
num_allocs = 0;
25+
max_bytes_used = 0;
26+
total_allocated_bytes = 0;
27+
net_heap_growth = 0;
28+
}
29+
} // namespace counts
30+
31+
class TestMemoryManager : public MemoryManager {
32+
void Start() override {}
33+
void Stop(Result& result) override {
34+
result.num_allocs = counts::num_allocs;
35+
result.net_heap_growth = counts::net_heap_growth;
36+
result.max_bytes_used = counts::max_bytes_used;
37+
result.total_allocated_bytes = counts::total_allocated_bytes;
38+
39+
counts::num_allocs += 1;
40+
counts::max_bytes_used += 2;
41+
counts::net_heap_growth += 4;
42+
counts::total_allocated_bytes += 10;
43+
}
44+
};
45+
46+
class TestReporter : public ConsoleReporter {
47+
public:
48+
TestReporter() = default;
49+
virtual ~TestReporter() = default;
50+
51+
bool ReportContext(const Context& /*unused*/) override { return true; }
52+
53+
void PrintHeader(const Run&) override {}
54+
void PrintRunData(const Run& run) override {
55+
if (run.repetition_index == -1) return;
56+
if (!run.memory_result.is_valid) return;
57+
58+
store.push_back(run.memory_result);
59+
}
60+
61+
std::vector<MemoryManager::Result> store;
62+
};
63+
64+
class MemoryResultsTest : public testing::Test {
65+
public:
66+
Benchmark* bm;
67+
TestReporter reporter;
68+
69+
void SetUp() override {
70+
bm = RegisterBenchmark("BM", [](State& st) {
71+
for (auto _ : st) {
72+
}
73+
});
74+
bm->Repetitions(N_REPETITIONS);
75+
bm->Iterations(N_ITERATIONS);
76+
}
77+
void TearDown() override { ClearRegisteredBenchmarks(); }
78+
};
79+
80+
TEST_F(MemoryResultsTest, NoMMTest) {
81+
RunSpecifiedBenchmarks(&reporter);
82+
EXPECT_EQ(reporter.store.size(), 0);
83+
}
84+
85+
TEST_F(MemoryResultsTest, ResultsTest) {
86+
counts::reset();
87+
MemoryManager* mm = new TestMemoryManager;
88+
RegisterMemoryManager(mm);
89+
EXPECT_EQ(benchmark::internal::memory_manager, mm);
90+
91+
RunSpecifiedBenchmarks(&reporter);
92+
EXPECT_EQ(reporter.store.size(), N_REPETITIONS);
93+
94+
for (size_t i = 0; i < reporter.store.size(); i++) {
95+
EXPECT_EQ(reporter.store[i].num_allocs, i);
96+
EXPECT_EQ(reporter.store[i].max_bytes_used, i * 2);
97+
EXPECT_EQ(reporter.store[i].net_heap_growth, i * 4);
98+
EXPECT_EQ(reporter.store[i].total_allocated_bytes, i * 10);
99+
}
100+
101+
delete mm;
102+
}

0 commit comments

Comments
 (0)