Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 4d0475b

Browse files
sipaPastaPastaPasta
authored andcommittedJan 12, 2020
Merge bitcoin#10099: Slightly Improve Unit Tests for Checkqueue
8c2f4b8 Expose more parallelism with relaxed atomics (suggested in bitcoin#9938). Fix a test to check the exclusive or of two properties rather than just or. (Jeremy Rubin) Pull request description: This PR is in response to bitcoin#10026 and some feedback on bitcoin#9938. ~Locally, all the checkqueue tests ran 3.2X faster on my machine. The worst offender, `test_CheckQueue_Correct_Random` ran 3.4X faster.~ 1. ~Removes `GetRand()` and replaces it with a single deterministic FastRandomContext instance.~ bitcoin#10321 replicated this 1. Exposes more parallelism with relaxed atomics, increasing chance of catching a bug. This does not change performance on my machine. 1. Makes one test case more restrictive (xor instead of or, see bitcoin#9938). Tree-SHA512: a59dfbee0273c713525a130dfedc1c7ff26f50c2aaca1e94ef5d759b1d6ea6338ffbd97f863b9f6209750d8a788a15fa8ae1bf26774ed2473c520811337e6b00
1 parent 13a5f69 commit 4d0475b

File tree

1 file changed

+18
-15
lines changed

1 file changed

+18
-15
lines changed
 

‎src/test/checkqueue_tests.cpp

+18-15
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ struct FakeCheckCheckCompletion {
3838
static std::atomic<size_t> n_calls;
3939
bool operator()()
4040
{
41-
++n_calls;
41+
n_calls.fetch_add(1, std::memory_order_relaxed);
4242
return true;
4343
}
4444
void swap(FakeCheckCheckCompletion& x){};
@@ -88,15 +88,15 @@ struct MemoryCheck {
8888
//
8989
// Really, copy constructor should be deletable, but CCheckQueue breaks
9090
// if it is deleted because of internal push_back.
91-
fake_allocated_memory += b;
91+
fake_allocated_memory.fetch_add(b, std::memory_order_relaxed);
9292
};
9393
MemoryCheck(bool b_) : b(b_)
9494
{
95-
fake_allocated_memory += b;
95+
fake_allocated_memory.fetch_add(b, std::memory_order_relaxed);
9696
};
97-
~MemoryCheck(){
98-
fake_allocated_memory -= b;
99-
97+
~MemoryCheck()
98+
{
99+
fake_allocated_memory.fetch_sub(b, std::memory_order_relaxed);
100100
};
101101
void swap(MemoryCheck& x) { std::swap(b, x.b); };
102102
};
@@ -117,9 +117,9 @@ struct FrozenCleanupCheck {
117117
{
118118
if (should_freeze) {
119119
std::unique_lock<std::mutex> l(m);
120-
nFrozen = 1;
120+
nFrozen.store(1, std::memory_order_relaxed);
121121
cv.notify_one();
122-
cv.wait(l, []{ return nFrozen == 0;});
122+
cv.wait(l, []{ return nFrozen.load(std::memory_order_relaxed) == 0;});
123123
}
124124
}
125125
void swap(FrozenCleanupCheck& x){std::swap(should_freeze, x.should_freeze);};
@@ -262,7 +262,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Recovers_From_Failure)
262262
control.Add(vChecks);
263263
}
264264
bool r =control.Wait();
265-
BOOST_REQUIRE(r || end_fails);
265+
BOOST_REQUIRE(r != end_fails);
266266
}
267267
}
268268
tg.interrupt_all();
@@ -337,7 +337,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Memory)
337337
tg.join_all();
338338
}
339339

340-
// Test that a new verification cannot occur until all checks
340+
// Test that a new verification cannot occur until all checks
341341
// have been destructed
342342
BOOST_AUTO_TEST_CASE(test_CheckQueue_FrozenCleanup)
343343
{
@@ -361,11 +361,14 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_FrozenCleanup)
361361
std::unique_lock<std::mutex> l(FrozenCleanupCheck::m);
362362
// Wait until the queue has finished all jobs and frozen
363363
FrozenCleanupCheck::cv.wait(l, [](){return FrozenCleanupCheck::nFrozen == 1;});
364-
// Try to get control of the queue a bunch of times
365-
for (auto x = 0; x < 100 && !fails; ++x) {
366-
fails = queue->ControlMutex.try_lock();
367-
}
368-
// Unfreeze
364+
}
365+
// Try to get control of the queue a bunch of times
366+
for (auto x = 0; x < 100 && !fails; ++x) {
367+
fails = queue->ControlMutex.try_lock();
368+
}
369+
{
370+
// Unfreeze (we need lock n case of spurious wakeup)
371+
std::unique_lock<std::mutex> l(FrozenCleanupCheck::m);
369372
FrozenCleanupCheck::nFrozen = 0;
370373
}
371374
// Awaken frozen destructor

0 commit comments

Comments
 (0)
Please sign in to comment.