Slightly Improve Unit Tests for Checkqueue #10099

pull JeremyRubin wants to merge 1 commits into bitcoin:master from JeremyRubin:speedup-checkqueue-tests changing 1 files +18 −15

JeremyRubin commented at 7:44 PM on March 27, 2017: contributor
This PR is in response to #10026 and some feedback on #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.~ #10321 replicated this
2. Exposes more parallelism with relaxed atomics, increasing chance of catching a bug. This does not change performance on my machine.
3. Makes one test case more restrictive (xor instead of or, see #9938).
MarcoFalke added the label Tests on Mar 27, 2017
laanwj commented at 7:41 AM on March 29, 2017: member

Great!

Removes GetRand() and replaces it with a single deterministic FastRandomContext instance.

I'd prefer to create a deterministic FastRandomContext per test case, and pass that in where necessary instead of using a global helper function. Otherwise determinism depends on the order in which test cases are run, which might change if e.g. new ones are added.
JeremyRubin force-pushed on Mar 29, 2017
JeremyRubin force-pushed on Mar 29, 2017

fanquake commented at 3:44 AM on March 31, 2017: member

Tested with src/test/test_bitcoin --log_level=all --run_test=checkqueue_tests

5 runs of master (edc62c959ab3ed7019b514107a4ab3865fc12c2e):

Leaving test suite "checkqueue_tests"; testing time: 54747669us
Leaving test suite "checkqueue_tests"; testing time: 56930968us
Leaving test suite "checkqueue_tests"; testing time: 54294643us
Leaving test suite "checkqueue_tests"; testing time: 51370752us
Leaving test suite "checkqueue_tests"; testing time: 50775172us

avg = ~53623841us

vs 5 runs of this PR (5004e46):

Leaving test suite "checkqueue_tests"; testing time: 39590527us
Leaving test suite "checkqueue_tests"; testing time: 39301795us
Leaving test suite "checkqueue_tests"; testing time: 40510899us
Leaving test suite "checkqueue_tests"; testing time: 40516578us
Leaving test suite "checkqueue_tests"; testing time: 39856853us

avg = ~39955330us

= ~1.3times speed increase

JeremyRubin commented at 4:44 AM on March 31, 2017: contributor

@fanquake what architecture are you running on? How many cores?

Anyways, on my system I clock the original test suite (e.g., still getrand) as taking 0.3s after my queue improvements, so I think it's safe to just wait until those make it in for a performance gain.

in src/test/checkqueue_tests.cpp:164 in 5004e46063 outdated

 160 | @@ -160,7 +161,7 @@ void Correct_Queue_range(std::vector<size_t> range)
 161 |          FakeCheckCheckCompletion::n_calls = 0;
 162 |          CCheckQueueControl<FakeCheckCheckCompletion> control(small_queue.get());
 163 |          while (total) {
 164 | -            vChecks.resize(std::min(total, (size_t) GetRand(10)));
 165 | +            vChecks.resize(std::min(total, (size_t) (insecure_rand.rand32() % 10)));

MarcoFalke commented at 10:00 PM on August 8, 2017:

This was already replaced in #10321. Needs rebase.

JeremyRubin force-pushed on Aug 9, 2017
Expose more parallelism with relaxed atomics (suggested in #9938). Fix a test to check the exclusive or of two properties rather than just or. 8c2f4b8882
JeremyRubin force-pushed on Aug 9, 2017
JeremyRubin renamed this:
~~Speedup & Slightly Improve Unit Tests for Checkqueue~~
Slightly Improve Unit Tests for Checkqueue
on Aug 9, 2017
MarcoFalke commented at 12:45 PM on October 5, 2017: member

utACK 8c2f4b88828b3e40f6cc690261657e66b2653432

in src/test/checkqueue_tests.cpp:370 in 8c2f4b8882

 370 | +    // Try to get control of the queue a bunch of times
 371 | +    for (auto x = 0; x < 100 && !fails; ++x) {
 372 | +        fails = queue->ControlMutex.try_lock();
 373 | +    }
 374 | +    {
 375 | +        // Unfreeze (we need lock n case of spurious wakeup)

ryanofsky commented at 6:42 PM on October 6, 2017:

s/n/in/

in src/test/checkqueue_tests.cpp:365 in 8c2f4b8882

 365 | -        for (auto x = 0; x < 100 && !fails; ++x) {
 366 | -            fails = queue->ControlMutex.try_lock();
 367 | -        }
 368 | -        // Unfreeze
 369 | +    }
 370 | +    // Try to get control of the queue a bunch of times

ryanofsky commented at 6:43 PM on October 6, 2017:

Maybe add "must always fail" or "should always fail."

ryanofsky commented at 6:55 PM on October 6, 2017: member

utACK 8c2f4b88828b3e40f6cc690261657e66b2653432. Looks good.
ryanofsky commented at 7:14 PM on October 12, 2017: member

Can this be merged? It's a simple test improvement with two acks.
sipa commented at 10:32 PM on October 12, 2017: member

utACK 8c2f4b88828b3e40f6cc690261657e66b2653432
sipa merged this on Oct 12, 2017
sipa closed this on Oct 12, 2017
sipa referenced this in commit 424be03305 on Oct 12, 2017
PastaPastaPasta referenced this in commit 67513d3df3 on Dec 22, 2019
PastaPastaPasta referenced this in commit b85ffe17e5 on Jan 2, 2020
PastaPastaPasta referenced this in commit 84a3603b87 on Jan 4, 2020
PastaPastaPasta referenced this in commit 84ab39d400 on Jan 12, 2020
PastaPastaPasta referenced this in commit 962becc9eb on Jan 12, 2020
PastaPastaPasta referenced this in commit 69991c3764 on Jan 12, 2020
PastaPastaPasta referenced this in commit 4d0475b592 on Jan 12, 2020
PastaPastaPasta referenced this in commit 920e3e709d on Jan 12, 2020
PastaPastaPasta referenced this in commit 40f559ebbb on Jan 12, 2020
PastaPastaPasta referenced this in commit 1e2ab7633f on Jan 16, 2020
ckti referenced this in commit 72d0e25cdd on Mar 28, 2021
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#9938 Lock-Free CheckQueue #10026 Overview of slow unit tests #10321 Use FastRandomContext for all tests