Slightly Improve Unit Tests for Checkqueue #10099

JeremyRubin · 2017-03-27T19:44:18Z

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.~~

~~Removes GetRand() and replaces it with a single deterministic FastRandomContext instance.~~ Use FastRandomContext for all tests #10321 replicated this
Exposes more parallelism with relaxed atomics, increasing chance of catching a bug. This does not change performance on my machine.
Makes one test case more restrictive (xor instead of or, see Lock-Free CheckQueue #9938).

laanwj · 2017-03-29T07:41:40Z

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.

fanquake · 2017-03-31T03:44:42Z

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

5 runs of master (edc62c9):

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 · 2017-03-31T04:44:48Z

@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.

maflcko · 2017-08-08T22:00:05Z

src/test/checkqueue_tests.cpp

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


This was already replaced in #10321. Needs rebase.

…). Fix a test to check the exclusive or of two properties rather than just or.

maflcko · 2017-10-05T12:45:02Z

utACK 8c2f4b8

ryanofsky

utACK 8c2f4b8. Looks good.

ryanofsky · 2017-10-06T18:42:34Z

src/test/checkqueue_tests.cpp

+        fails = queue->ControlMutex.try_lock();
+    }
+    {
+        // Unfreeze (we need lock n case of spurious wakeup)


ryanofsky · 2017-10-06T18:43:35Z

src/test/checkqueue_tests.cpp

-        }
-        // Unfreeze
+    }
+    // Try to get control of the queue a bunch of times


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

ryanofsky · 2017-10-12T19:14:28Z

Can this be merged? It's a simple test improvement with two acks.

sipa · 2017-10-12T22:32:44Z

utACK 8c2f4b8

8c2f4b8 Expose more parallelism with relaxed atomics (suggested in #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 #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 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 #9938). Tree-SHA512: a59dfbee0273c713525a130dfedc1c7ff26f50c2aaca1e94ef5d759b1d6ea6338ffbd97f863b9f6209750d8a788a15fa8ae1bf26774ed2473c520811337e6b00

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

maflcko added the Tests label Mar 27, 2017

JeremyRubin force-pushed the speedup-checkqueue-tests branch 2 times, most recently from 18b4830 to 690acd1 Compare March 29, 2017 15:03

maflcko reviewed Aug 8, 2017

View reviewed changes

JeremyRubin force-pushed the speedup-checkqueue-tests branch from abfcd05 to 7831902 Compare August 9, 2017 09:00

Expose more parallelism with relaxed atomics (suggested in bitcoin#9938…

8c2f4b8

…). Fix a test to check the exclusive or of two properties rather than just or.

JeremyRubin force-pushed the speedup-checkqueue-tests branch from 7831902 to 8c2f4b8 Compare August 9, 2017 09:07

JeremyRubin changed the title ~~Speedup & Slightly Improve Unit Tests for Checkqueue~~ Slightly Improve Unit Tests for Checkqueue Aug 9, 2017

ryanofsky reviewed Oct 6, 2017

View reviewed changes

sipa merged commit 8c2f4b8 into bitcoin:master Oct 12, 2017

maflcko mentioned this pull request Nov 22, 2017

qa: Add multiwallet prefix test #11743

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Slightly Improve Unit Tests for Checkqueue #10099

Slightly Improve Unit Tests for Checkqueue #10099

Uh oh!

JeremyRubin commented Mar 27, 2017 •

edited

Loading

Uh oh!

laanwj commented Mar 29, 2017 •

edited

Loading

Uh oh!

fanquake commented Mar 31, 2017

Uh oh!

JeremyRubin commented Mar 31, 2017

Uh oh!

maflcko Aug 8, 2017

Uh oh!

maflcko commented Oct 5, 2017

Uh oh!

ryanofsky left a comment

Uh oh!

ryanofsky Oct 6, 2017

Uh oh!

ryanofsky Oct 6, 2017

Uh oh!

ryanofsky commented Oct 12, 2017

Uh oh!

sipa commented Oct 12, 2017

Uh oh!

Uh oh!

Slightly Improve Unit Tests for Checkqueue #10099

Slightly Improve Unit Tests for Checkqueue #10099

Uh oh!

Conversation

JeremyRubin commented Mar 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Mar 31, 2017

avg = ~53623841us

avg = ~39955330us

= ~1.3times speed increase

Uh oh!

JeremyRubin commented Mar 31, 2017

Uh oh!

maflcko Aug 8, 2017

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 5, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 6, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 6, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Oct 12, 2017

Uh oh!

sipa commented Oct 12, 2017

Uh oh!

Uh oh!

JeremyRubin commented Mar 27, 2017 •

edited

Loading

laanwj commented Mar 29, 2017 •

edited

Loading