Currently, the test for OP_REVERSEBYTES takes ~40s to finish. The bottleneck is that both the stack item sizes and the flag set are checked exhaustively combinatorically. This diff still checks both exhaustively, but separately, and uses a curated set of flags/test cases for each, respectively. This reduces the test execution time to around ~1s.
Details
- Reviewers
deadalnix - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) Restricted Project - Commits
- rSTAGINGa546a8d17f10: Speed up OP_REVERSEBYTES test significantly
rABCa546a8d17f10: Speed up OP_REVERSEBYTES test significantly
ninja check
./src/test/test_bitcoin -t op_reversebytes_tests
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- master
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 10247 Build 18316: Default Diff Build & Tests Build 18315: arc lint + arc unit
Event Timeline
The whole structure you have here doesn't make a lot of sense. You iterate to build large data structure with which which you do nothing but iterate over. It does nothing but moving memor consumption from O(1) to O(n).
Just write several tests. One for different sizes. One for palidromes. etc...
This is quite evidently not sliced the right way.
src/test/op_reversebytes_tests.cpp | ||
---|---|---|
81 ↗ | (On Diff #18806) | This is a terrible function name. Does it check that the reverse of the test case that is passed also passes? No. What makes sense would be something like given A and B, test that the reverse of A is B, the reverse of B is A, the double reverse of A is A and the double reverse of B is B. |
96 ↗ | (On Diff #18806) | This only depends on the flags. So that belong is the flag grilling test not in some common function. |
113 ↗ | (On Diff #18806) | dito |
I see you like that functional programming thing, but you kinda went overboard with it.
You might want to give that good old for loop a chance. This will be completely impossible to debug when something goes south.
src/test/op_reversebytes_tests.cpp | ||
---|---|---|
126 ↗ | (On Diff #18814) | What about you define a set of flags, or even a function to get a selection of flags, and have the caller do something like for (uint32_t flags : GetWhateverFlags()) { // Do stuff here... } and then delete that thing. |
172 ↗ | (On Diff #18814) | Just use a good old for loop. Bonus point, you'll be able to allocate one vector instead of n, the code will be easier to debug and follow in general and it's not even a given that it'll be that much longer. For the many flag test, it's okay to use static test vectors, or to leverage GetTheThingYouWant() functions. Pick a pseudorandom set of flags, pick static and radom test vector, check that and do it again with a new set of flags. |
src/test/op_reversebytes_tests.cpp | ||
---|---|---|
88 | This is not a good name. You can also drop the reverse test case thing and just pass two values. | |
137 | Dude, just do for (int i = 0; i < 4096; i++) { uint32_t flags = lcg.next(); // Do stuff... } Just stop overcomplicating everything. This is not shorter, this is not faster (in fact, this is slower). Why does this exist? Also, bonus point for the Flag*Set* being a vector XD | |
171 | Just create a test case with all the flag instead of woving this all over the place. |
This is MUCH better.
src/test/op_reversebytes_tests.cpp | ||
---|---|---|
160 ↗ | (On Diff #18865) | This has the same structure as op_reversebytes_iota_random_flags and op_reversebytes_palindrome so you should probably merge them. The code is talking to you, you just got to pay attention. |
173 ↗ | (On Diff #18865) | Don't create function if you are going to only use them in one place. |
197 ↗ | (On Diff #18865) | You can merge that into op_reversebytes_manual_random_flags with op_reversebytes_empty_stack_random_flags and maybe other pieces that I'm missing. |