Skip to content

Commit 8ee8c85

Browse files
rdemellowrdemellow
committed
MB-41510: Fix TSAN failure due to lock order inversion
Fix TSAN failures in module_tests/hdrhistogram_test.cc, due to theoretical deadlock that could occur when using two iterators that hold read locks on different HdrHistograms (this isn't the case now bug could be if the code was modified). To avoid this WARNING, ensure in our HdrHistogramTests that we never hold multiple HdrHistogram::Iterators in the same scope. Example TSAN Failure: WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=16448) Cycle in lock order graph: M732257135894671160 (0x000000000000) => M730849761011117904 (0x000000000000) => M732257135894671160 Mutex M730849761011117904 acquired here while holding mutex M732257135894671160 in main thread: #0 AnnotateRWLockAcquired <null> (ep-engine_ep_unit_tests+0x64007b) #1 folly::detail::annotate_rwlock_acquired_impl(void const volatile*, folly::annotate_rwlock_level, char const*, int) /home/couchbase/jenkins/workspace/cbdeps-platform-build-old/deps/packages/build/folly/follytsan-prefix/src/follytsan/folly/synchronization/SanitizeThread.cpp:99 (ep-engine_ep_unit_tests+0x162f6be) #2 annotate_rwlock_acquired /home/couchbase/jenkins/workspace/cbdeps-platform-build-old/deps/packages/build/folly/follytsan-prefix/src/follytsan/folly/synchronization/SanitizeThread.h:111 (ep-engine_ep_unit_tests+0x15a9881) #3 folly::SharedMutexImpl<false, void, std::atomic, false, true>::annotateAcquired(folly::annotate_rwlock_level) /home/couchbase/jenkins/workspace/cbdeps-platform-build-old/deps/packages/build/folly/follytsan-prefix/src/follytsan/folly/SharedMutex.h:740 (ep-engine_ep_unit_tests+0x15a9881) #4 folly::SharedMutexImpl<false, void, std::atomic, false, true>::lock() /home/couchbase/jenkins/workspace/cbdeps-platform-build-old/deps/packages/build/folly/follytsan-prefix/src/follytsan/folly/SharedMutex.h:372 (ep-engine_ep_unit_tests+0x15a9881) #5 folly::detail::LockTraitsImpl<folly::SharedMutexImpl<false, void, std::atomic, false, true>, (folly::detail::MutexLevel)0, false>::lock(folly::SharedMutexImpl<false, void, std::atomic, false, true>&) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/tlm/deps/folly.exploded/include/folly/LockTraits.h:124:11 (ep-engine_ep_unit_tests+0x717b75) #6 std::integral_constant<bool, true> folly::LockPolicyExclusive::lock<folly::SharedMutexImpl<false, void, std::atomic, false, true> >(folly::SharedMutexImpl<false, void, std::atomic, false, true>&) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/tlm/deps/folly.exploded/include/folly/LockTraits.h:479:5 (ep-engine_ep_unit_tests+0x717b45) #7 folly::LockedPtrBase<folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> >, folly::SharedMutexImpl<false, void, std::atomic, false, true>, folly::LockPolicyExclusive>::LockedPtrBase(folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> >*) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/tlm/deps/folly.exploded/include/folly/Synchronized.h:1097:10 (ep-engine_ep_unit_tests+0x158cb02) #8 folly::LockedPtr<folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> >, folly::LockPolicyExclusive>::LockedPtr(folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> >*) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/tlm/deps/folly.exploded/include/folly/Synchronized.h:1402:50 (ep-engine_ep_unit_tests+0x158caae) #9 folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> >::contextualLock() /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/tlm/deps/folly.exploded/include/folly/Synchronized.h:653:12 (ep-engine_ep_unit_tests+0x158ceae) #10 std::tuple<std::conditional<std::is_const<folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> > >::value, folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> >::ConstLockedPtr, folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> >::LockedPtr>::type, std::conditional<std::is_const<folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> > const>::value, folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> > const::ConstLockedPtr, folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> > const::LockedPtr>::type> folly::acquireLocked<folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> >, folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> > const>(folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> >&, folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> > const&) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/tlm/deps/folly.exploded/include/folly/Synchronized.h:1741:18 (ep-engine_ep_unit_tests+0x158ccda) #11 std::pair<std::conditional<std::is_const<folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> > >::value, folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> >::ConstLockedPtr, folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> >::LockedPtr>::type, std::conditional<std::is_const<folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> > const>::value, folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> > const::ConstLockedPtr, folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> > const::LockedPtr>::type> folly::acquireLockedPair<folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> >, folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> > const>(folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> >&, folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> > const&) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/tlm/deps/folly.exploded/include/folly/Synchronized.h:1753:21 (ep-engine_ep_unit_tests+0x158bbf9) #12 HdrHistogram::operator+=(HdrHistogram const&) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../kv_engine/utilities/hdrhistogram.cc:67:21 (ep-engine_ep_unit_tests+0x158a3bf) #13 HdrHistogramTest_aggregationTest_Test::TestBody() /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../kv_engine/engines/ep/tests/module_tests/hdrhistogram_test.cc:317:18 (ep-engine_ep_unit_tests+0x13479b1) #14 void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../third_party/googletest/googletest/src/gtest.cc:2433:10 (ep-engine_ep_unit_tests+0x1567683) #15 void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../third_party/googletest/googletest/src/gtest.cc:2469:14 (ep-engine_ep_unit_tests+0x1553012) #16 testing::Test::Run() /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../third_party/googletest/googletest/src/gtest.cc:2508:5 (ep-engine_ep_unit_tests+0x153b87c) #17 testing::TestInfo::Run() /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../third_party/googletest/googletest/src/gtest.cc:2684:11 (ep-engine_ep_unit_tests+0x153c26a) #18 testing::TestSuite::Run() /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../third_party/googletest/googletest/src/gtest.cc:2816:28 (ep-engine_ep_unit_tests+0x153c841) #19 testing::internal::UnitTestImpl::RunAllTests() /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../third_party/googletest/googletest/src/gtest.cc:5338:44 (ep-engine_ep_unit_tests+0x1545a21) #20 bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../third_party/googletest/googletest/src/gtest.cc:2433:10 (ep-engine_ep_unit_tests+0x156aa43) #21 bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../third_party/googletest/googletest/src/gtest.cc:2469:14 (ep-engine_ep_unit_tests+0x15551e2) #22 testing::UnitTest::Run() /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../third_party/googletest/googletest/src/gtest.cc:4925:10 (ep-engine_ep_unit_tests+0x15455d7) #23 RUN_ALL_TESTS() /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../third_party/googletest/googletest/include/gtest/gtest.h:2473:46 (ep-engine_ep_unit_tests+0x1079927) #24 main /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../kv_engine/engines/ep/tests/module_tests/ep_unit_tests_main.cc:175:16 (ep-engine_ep_unit_tests+0x107978e) Hint: use TSAN_OPTIONS=second_deadlock_stack=1 to get more informative warning message Mutex M732257135894671160 acquired here while holding mutex M730849761011117904 in main thread: #0 AnnotateRWLockAcquired <null> (ep-engine_ep_unit_tests+0x64007b) #1 folly::detail::annotate_rwlock_acquired_impl(void const volatile*, folly::annotate_rwlock_level, char const*, int) /home/couchbase/jenkins/workspace/cbdeps-platform-build-old/deps/packages/build/folly/follytsan-prefix/src/follytsan/folly/synchronization/SanitizeThread.cpp:99 (ep-engine_ep_unit_tests+0x162f6be) #2 annotate_rwlock_acquired /home/couchbase/jenkins/workspace/cbdeps-platform-build-old/deps/packages/build/folly/follytsan-prefix/src/follytsan/folly/synchronization/SanitizeThread.h:111 (ep-engine_ep_unit_tests+0x15ad433) #3 folly::SharedMutexImpl<false, void, std::atomic, false, true>::annotateAcquired(folly::annotate_rwlock_level) /home/couchbase/jenkins/workspace/cbdeps-platform-build-old/deps/packages/build/folly/follytsan-prefix/src/follytsan/folly/SharedMutex.h:740 (ep-engine_ep_unit_tests+0x15ad433) #4 folly::SharedMutexImpl<false, void, std::atomic, false, true>::lock_shared() /home/couchbase/jenkins/workspace/cbdeps-platform-build-old/deps/packages/build/folly/follytsan-prefix/src/follytsan/folly/SharedMutex.h:414 (ep-engine_ep_unit_tests+0x15ad433) #5 folly::detail::LockTraitsImpl<folly::SharedMutexImpl<false, void, std::atomic, false, true>, (folly::detail::MutexLevel)1, false>::lock_shared(folly::SharedMutexImpl<false, void, std::atomic, false, true>&) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/tlm/deps/folly.exploded/include/folly/LockTraits.h:157:11 (ep-engine_ep_unit_tests+0x703215) #6 std::integral_constant<bool, true> folly::LockPolicyShared::lock<folly::SharedMutexImpl<false, void, std::atomic, false, true> >(folly::SharedMutexImpl<false, void, std::atomic, false, true>&) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/tlm/deps/folly.exploded/include/folly/LockTraits.h:499:5 (ep-engine_ep_unit_tests+0x7031c5) #7 folly::LockedPtrBase<folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> > const, folly::SharedMutexImpl<false, void, std::atomic, false, true>, folly::LockPolicyShared>::LockedPtrBase(folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> > const*) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/tlm/deps/folly.exploded/include/folly/Synchronized.h:1097:10 (ep-engine_ep_unit_tests+0x915242) #8 folly::LockedPtr<folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> > const, folly::LockPolicyShared>::LockedPtr(folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> > const*) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/tlm/deps/folly.exploded/include/folly/Synchronized.h:1402:50 (ep-engine_ep_unit_tests+0x9151ee) #9 folly::SynchronizedBase<folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> >, (folly::detail::MutexLevel)1>::rlock() const /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/tlm/deps/folly.exploded/include/folly/Synchronized.h:144:12 (ep-engine_ep_unit_tests+0x91502e) #10 HdrHistogram::Iterator::Iterator(folly::Synchronized<std::unique_ptr<hdr_histogram, HdrHistogram::HdrDeleter>, folly::SharedMutexImpl<false, void, std::atomic, false, true> > const&, HdrHistogram::Iterator::IterMode) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../kv_engine/utilities/hdrhistogram.h:89:66 (ep-engine_ep_unit_tests+0x158be71) #11 HdrHistogram::makeLinearIterator(long) const /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../kv_engine/utilities/hdrhistogram.cc:162:28 (ep-engine_ep_unit_tests+0x158aafb) #12 HdrHistogramTest_aggregationTest_Test::TestBody() /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../kv_engine/engines/ep/tests/module_tests/hdrhistogram_test.cc:322:26 (ep-engine_ep_unit_tests+0x13479e2) #13 void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../third_party/googletest/googletest/src/gtest.cc:2433:10 (ep-engine_ep_unit_tests+0x1567683) #14 void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../third_party/googletest/googletest/src/gtest.cc:2469:14 (ep-engine_ep_unit_tests+0x1553012) #15 testing::Test::Run() /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../third_party/googletest/googletest/src/gtest.cc:2508:5 (ep-engine_ep_unit_tests+0x153b87c) #16 testing::TestInfo::Run() /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../third_party/googletest/googletest/src/gtest.cc:2684:11 (ep-engine_ep_unit_tests+0x153c26a) #17 testing::TestSuite::Run() /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../third_party/googletest/googletest/src/gtest.cc:2816:28 (ep-engine_ep_unit_tests+0x153c841) #18 testing::internal::UnitTestImpl::RunAllTests() /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../third_party/googletest/googletest/src/gtest.cc:5338:44 (ep-engine_ep_unit_tests+0x1545a21) #19 bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../third_party/googletest/googletest/src/gtest.cc:2433:10 (ep-engine_ep_unit_tests+0x156aa43) #20 bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../third_party/googletest/googletest/src/gtest.cc:2469:14 (ep-engine_ep_unit_tests+0x15551e2) #21 testing::UnitTest::Run() /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../third_party/googletest/googletest/src/gtest.cc:4925:10 (ep-engine_ep_unit_tests+0x15455d7) #22 RUN_ALL_TESTS() /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../third_party/googletest/googletest/include/gtest/gtest.h:2473:46 (ep-engine_ep_unit_tests+0x1079927) #23 main /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/build/../kv_engine/engines/ep/tests/module_tests/ep_unit_tests_main.cc:175:16 (ep-engine_ep_unit_tests+0x107978e) Change-Id: I7c3e9369065e5344333c410602267835f9bcc7e1 Reviewed-on: http://review.couchbase.org/c/kv_engine/+/137745 Reviewed-by: Dave Rigby <daver@couchbase.com> Tested-by: Build Bot <build@couchbase.com>
1 parent 5cf5a80 commit 8ee8c85

File tree

1 file changed

+74
-69
lines changed

1 file changed

+74
-69
lines changed

‎engines/ep/tests/module_tests/hdrhistogram_test.cc‎

Lines changed: 74 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,16 @@
2525
#include <thread>
2626
#include <utility>
2727

28+
static std::vector<std::pair<uint64_t, uint64_t>> getValuesOnePerBucket(
29+
HdrHistogram& histo) {
30+
std::vector<std::pair<uint64_t, uint64_t>> values;
31+
auto iter = histo.makeLinearIterator(/* valueUnitsPerBucket */ 1);
32+
while (auto pair = iter.getNextValueAndCount()) {
33+
values.push_back(*pair);
34+
}
35+
return values;
36+
}
37+
2838
/*
2939
* Unit tests for the HdrHistogram
3040
*/
@@ -76,13 +86,11 @@ TEST(HdrHistogramTest, linearIteratorTest) {
7686
histogram.addValue(ii);
7787
}
7888

79-
// Need to create the iterator after we have added the data
80-
HdrHistogram::Iterator iter{
81-
histogram.makeLinearIterator(/* valueUnitsPerBucket */ 1)};
8289
uint64_t valueCount = 0;
83-
while (auto result = iter.getNextValueAndCount()) {
84-
EXPECT_EQ(valueCount, result->first);
85-
++valueCount;
90+
auto values = getValuesOnePerBucket(histogram);
91+
EXPECT_EQ(256, values.size());
92+
for (auto& result : values) {
93+
EXPECT_EQ(valueCount++, result.first);
8694
}
8795
}
8896

@@ -159,11 +167,12 @@ TEST(HdrHistogramTest, addValueAndCountTest) {
159167
HdrHistogram histogram{0, 255, 3};
160168

161169
histogram.addValueAndCount(0, 100);
162-
// Need to create the iterator after we have added the data
163-
HdrHistogram::Iterator iter{histogram.makeLinearIterator(1)};
164-
while (auto result = iter.getNextValueAndCount()) {
165-
EXPECT_EQ(0, result->first);
166-
EXPECT_EQ(100, result->second);
170+
171+
auto values = getValuesOnePerBucket(histogram);
172+
EXPECT_EQ(1, values.size());
173+
for (auto& result : values) {
174+
EXPECT_EQ(0, result.first);
175+
EXPECT_EQ(100, result.second);
167176
}
168177
}
169178

@@ -284,14 +293,12 @@ TEST(HdrHistogramTest, addValueParallel) {
284293
ASSERT_EQ(maxVal - 1, histogram.getMaxValue());
285294
ASSERT_EQ(0, histogram.getMinValue());
286295

287-
HdrHistogram::Iterator iter{
288-
histogram.makeLinearIterator(/* valueUnitsPerBucket */ 1)};
289296
uint64_t valueCount = 0;
290-
// Assert that the right number of values were added to the histogram
291-
while (auto result = iter.getNextValueAndCount()) {
292-
ASSERT_EQ(valueCount, result->first);
293-
ASSERT_EQ(threads.size() * numOfAddIterations, result->second);
294-
++valueCount;
297+
auto values = getValuesOnePerBucket(histogram);
298+
EXPECT_EQ(maxVal, values.size());
299+
for (auto& result : values) {
300+
ASSERT_EQ(valueCount++, result.first);
301+
ASSERT_EQ(threads.size() * numOfAddIterations, result.second);
295302
}
296303
}
297304

@@ -306,68 +313,64 @@ TEST(HdrHistogramTest, percentileWhenEmptyTest) {
306313

307314
// Test the aggregation operator method
308315
TEST(HdrHistogramTest, aggregationTest) {
309-
HdrHistogram histogramOne{0, 15, 3};
310-
HdrHistogram histogramTwo{0, 15, 3};
316+
const uint16_t numberOfValues = 15;
317+
HdrHistogram histogramOne{0, numberOfValues, 3};
318+
HdrHistogram histogramTwo{0, numberOfValues, 3};
311319

312-
for (int i = 0; i < 15; i++) {
320+
for (int i = 0; i < numberOfValues; i++) {
313321
histogramOne.addValue(i);
314322
histogramTwo.addValue(i);
315323
}
316324
// Do aggregation
317325
histogramOne += histogramTwo;
318326

319-
HdrHistogram::Iterator iterOne{
320-
histogramOne.makeLinearIterator(/* valueUnitsPerBucket */ 1)};
321-
HdrHistogram::Iterator iterTwo{
322-
histogramTwo.makeLinearIterator(/* valueUnitsPerBucket */ 1)};
327+
auto histoOneValues = getValuesOnePerBucket(histogramOne);
328+
EXPECT_EQ(numberOfValues, histoOneValues.size());
329+
330+
auto histoTwoValues = getValuesOnePerBucket(histogramTwo);
331+
EXPECT_EQ(numberOfValues, histoTwoValues.size());
332+
333+
EXPECT_NE(histoOneValues, histoTwoValues);
334+
323335
uint64_t valueCount = 0;
324-
for (int i = 0; i < 15; i++) {
325-
auto resultOne = iterOne.getNextValueAndCount();
326-
auto resultTwo = iterTwo.getNextValueAndCount();
336+
for (int i = 0; i < numberOfValues; i++) {
327337
// check values are the same for both histograms
328-
EXPECT_EQ(valueCount, resultTwo->first);
329-
EXPECT_EQ(valueCount, resultOne->first);
338+
EXPECT_EQ(valueCount, histoTwoValues[i].first);
339+
EXPECT_EQ(valueCount, histoOneValues[i].first);
330340
// check that the counts for each value is twice as much as
331341
// in a bucket in histogram one as it is in histogram two
332-
EXPECT_EQ(resultOne->second, resultTwo->second * 2);
342+
EXPECT_EQ(histoOneValues[i].second, histoTwoValues[i].second * 2);
333343
++valueCount;
334344
}
335345

336346
// Check the totals of each histogram
337-
EXPECT_EQ(30, histogramOne.getValueCount());
338-
EXPECT_EQ(15, histogramTwo.getValueCount());
347+
EXPECT_EQ(numberOfValues * 2, histogramOne.getValueCount());
348+
EXPECT_EQ(numberOfValues, histogramTwo.getValueCount());
339349
}
340350

341351
// Test the aggregation operator method
342352
TEST(HdrHistogramTest, aggregationTestEmptyLhr) {
353+
const uint16_t numberOfValues = 200;
343354
HdrHistogram histogramOne{0, 15, 3};
344-
HdrHistogram histogramTwo{0, 200, 3};
355+
HdrHistogram histogramTwo{0, numberOfValues, 3};
345356

346-
for (int i = 0; i < 200; i++) {
357+
for (int i = 0; i < numberOfValues; i++) {
347358
histogramTwo.addValue(i);
348359
}
349360
// Do aggregation
350361
histogramOne += histogramTwo;
351362

352-
HdrHistogram::Iterator iterOne{
353-
histogramOne.makeLinearIterator(/* valueUnitsPerBucket */ 1)};
354-
HdrHistogram::Iterator iterTwo{
355-
histogramTwo.makeLinearIterator(/* valueUnitsPerBucket */ 1)};
363+
auto histoOneValues = getValuesOnePerBucket(histogramOne);
364+
EXPECT_EQ(numberOfValues, histoOneValues.size());
356365

357-
// Max value of LHS should be updated too 200 thus counts should be the
358-
// same for every value in both histograms
359-
for (int i = 0; i < 200; i++) {
360-
auto resultOne = iterOne.getNextValueAndCount();
361-
auto resultTwo = iterTwo.getNextValueAndCount();
362-
// check values are the same for both histograms
363-
EXPECT_EQ(resultOne->first, resultTwo->first);
364-
// check that the counts for each value are the same
365-
EXPECT_EQ(resultOne->second, resultTwo->second);
366-
}
366+
auto histoTwoValues = getValuesOnePerBucket(histogramTwo);
367+
EXPECT_EQ(numberOfValues, histoTwoValues.size());
368+
369+
EXPECT_EQ(histoTwoValues, histoOneValues);
367370

368371
// Check the totals of each histogram
369-
EXPECT_EQ(200, histogramOne.getValueCount());
370-
EXPECT_EQ(200, histogramTwo.getValueCount());
372+
EXPECT_EQ(numberOfValues, histogramOne.getValueCount());
373+
EXPECT_EQ(numberOfValues, histogramTwo.getValueCount());
371374
}
372375

373376
// Test the aggregation operator method
@@ -381,15 +384,13 @@ TEST(HdrHistogramTest, aggregationTestEmptyRhs) {
381384
// Do aggregation
382385
histogramOne += histogramTwo;
383386

384-
HdrHistogram::Iterator iter{
385-
histogramOne.makeLinearIterator(/* valueUnitsPerBucket */ 1)};
386-
387-
uint64_t valueCount = 0;
388387
// make sure the histogram has expanded in size for all 200 values
389-
while (auto result = iter.getNextValueAndCount()) {
390-
EXPECT_EQ(valueCount, result->first);
391-
EXPECT_EQ(1, result->second);
392-
++valueCount;
388+
auto values = getValuesOnePerBucket(histogramOne);
389+
EXPECT_EQ(200, values.size());
390+
uint64_t valueCount = 0;
391+
for (auto& result : values) {
392+
EXPECT_EQ(valueCount++, result.first);
393+
EXPECT_EQ(1, result.second);
393394
}
394395

395396
// Check the totals of each histogram
@@ -412,11 +413,13 @@ TEST(HdrHistogramTest, int32MaxSizeTest) {
412413
EXPECT_EQ(0, histogram.getValueAtPercentile(100.0));
413414
EXPECT_EQ(0, histogram.getMinValue());
414415

415-
HdrHistogram::Iterator iter{histogram.getHistogramsIterator()};
416-
auto res = iter.getNextBucketLowHighAndCount();
417-
EXPECT_TRUE(res);
418-
// The 3rd field [2] of the returned tuple is the count
419-
EXPECT_EQ(limit, std::get<2>(*res));
416+
{ // iter read lock scope
417+
HdrHistogram::Iterator iter{histogram.getHistogramsIterator()};
418+
auto res = iter.getNextBucketLowHighAndCount();
419+
EXPECT_TRUE(res);
420+
// The 3rd field [2] of the returned tuple is the count
421+
EXPECT_EQ(limit, std::get<2>(*res));
422+
}
420423

421424
// Add 1 more count (previously this would overflow the total_count field
422425
// in the iterator)
@@ -428,11 +431,13 @@ TEST(HdrHistogramTest, int32MaxSizeTest) {
428431
EXPECT_EQ(0, histogram.getValueAtPercentile(100.0));
429432
EXPECT_EQ(0, histogram.getMinValue());
430433

431-
HdrHistogram::Iterator iter2{histogram.getHistogramsIterator()};
432-
auto res2 = iter2.getNextBucketLowHighAndCount();
433-
EXPECT_TRUE(res2);
434-
// The 3rd field [2] of the returned tuple is the count
435-
EXPECT_EQ(limit, std::get<2>(*res2));
434+
{ // iter2 read lock scope
435+
HdrHistogram::Iterator iter2{histogram.getHistogramsIterator()};
436+
auto res2 = iter2.getNextBucketLowHighAndCount();
437+
EXPECT_TRUE(res2);
438+
// The 3rd field [2] of the returned tuple is the count
439+
EXPECT_EQ(limit, std::get<2>(*res2));
440+
}
436441
}
437442

438443
TEST(HdrHistogramTest, int64MaxSizeTest) {

0 commit comments

Comments
 (0)