Slightly Improve Unit Tests for Checkqueue#10099
Merged
sipa merged 1 commit intobitcoin:masterfrom Oct 12, 2017
Merged
Conversation
Member
|
Great!
I'd prefer to create a deterministic |
JeremyRubin
force-pushed
the
speedup-checkqueue-tests
branch
2 times, most recently
from
18b4830 to
690acd1
Compare
Member
|
Tested with 5 runs of master (edc62c9): avg = ~53623841usvs 5 runs of this PR (5004e46): avg = ~39955330us= ~1.3times speed increase |
Contributor
Author
|
@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
reviewed
Aug 8, 2017
src/test/checkqueue_tests.cpp
Outdated
JeremyRubin
force-pushed
the
speedup-checkqueue-tests
branch
from
abfcd05 to
7831902
Compare
…). 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
JeremyRubin
changed the title
Member
|
utACK 8c2f4b8 |
ryanofsky
reviewed
Oct 6, 2017
| fails = queue->ControlMutex.try_lock(); | ||
| } | ||
| { | ||
| // Unfreeze (we need lock n case of spurious wakeup) |
| } | ||
| // Unfreeze | ||
| } | ||
| // Try to get control of the queue a bunch of times |
Contributor
There was a problem hiding this comment.
Maybe add "must always fail" or "should always fail."
Contributor
|
Can this be merged? It's a simple test improvement with two acks. |
Member
|
utACK 8c2f4b8 |
sipa
added a commit
that referenced
this pull request
Oct 12, 2017
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
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jan 16, 2020
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_Randomran 3.4X faster.RemovesUse FastRandomContext for all tests #10321 replicated thisGetRand()and replaces it with a single deterministic FastRandomContext instance.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).