-
Notifications
You must be signed in to change notification settings - Fork 714
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Tests] Improve coverage for merkleblock and bloom code #2711
[Tests] Improve coverage for merkleblock and bloom code #2711
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can backport bitcoin#7934 and bitcoin#13176 (which are both direct cherry-picks) to not have to change the nHits
variable value on the rolling_bloom test case (bloom_tests.cpp
line 490 and 518).
Incorporates feedback suggested by @sipa, @promag, @TheBlueMatt.
This patch changes the implementation from one that stores 16 2-bit integers in one uint32_t's, to one that stores the first bit of 64 2-bit integers in one uint64_t and the second bit in another. This allows for 450x faster refreshing and 2.2x faster average speed.
Replaces the slow modulo operation with a much faster 32bit multiplication & shift. This works because the hash should be uniformly distributed between 0 and 2^32-1. This speeds up the benchmark by a factor of about 1.3: RollingBloom, 5, 1500000, 3.73733, 4.97569e-07, 4.99002e-07, 4.98372e-07 # before RollingBloom, 5, 1500000, 2.86842, 3.81630e-07, 3.83730e-07, 3.82473e-07 # FastMod Be aware that this changes the position of the bits that are toggled, so this should probably not be used for CBloomFilter which is serialized.
also rename the global `insecure_rand_ctx` to `g_insecure_rand_ctx`
752362f
to
f8c8a0a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
diff utACK f8c8a0a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK f8c8a0a
ad5717d Lint: Add lint-includes.sh (Fuzzbawls) faba3f6 [tests] Use FastRandomContext in scheduler_tests.cpp (practicalswift) 2034bf6 Remove unused boost includes in reverselock_tests.cpp (Fuzzbawls) 3256d16 bench: Use non-throwing ParseDouble() (Fuzzbawls) 3c984d1 Remove duplicate includes (Fuzzbawls) b54e396 Declare TorReply parsing functions in torcontrol_tests (Fuzzbawls) Pull request description: This brings in the `lint-includes.sh` shell script from upstream (first introduced in bitcoin#11878) to automatically check for duplicate includes and expanded in bitcoin#13301 and bitcoin#13385 to check for the inclusion of `.cpp` files and the introduction of new boost includes, respectively. The check for enforcing bracket include syntax (bitcoin#13230) is also included, but currently disabled, as we have yet to systematically switch to that syntax preference. Three other upstream PRs are backported here as they are directly related to the removal of some boost dependencies and are very straight forward: - bitcoin#10547 - bitcoin#13291 - bitcoin#13383 **NOTE: #2711 removes the dependency on `boost/tuple/tuple.hpp`, so it makes sense to merge that first. submitting this now so the general concept/functionality can be reviewed prior to that PR being merged** ACKs for top commit: furszy: good ACK ad5717d random-zebra: utACK ad5717d and merging... Tree-SHA512: d9d32d6d5122dac52c9601cda75612d87c4e027c6f196a2382206b227fcfd2bb61d4f72df7cbf5e572d94150ad8ca6db6301bd99b0da647b9627fe342b66873f
Based on the following upstream PRs:
Before:
After: