Page MenuHomePhabricator

Speed up OP_REVERSEBYTES test significantly
ClosedPublic

Authored by tobias_ruck on Apr 14 2020, 19:53.

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
Summary

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.

Test Plan

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 10216
Build 18257: Default Diff Build & Tests
Build 18256: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Apr 14 2020, 19:53

Actually use exhaustive test cases in curated flags case

deadalnix requested changes to this revision.Apr 14 2020, 20:28

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

This revision now requires changes to proceed.Apr 14 2020, 20:28
deadalnix requested changes to this revision.Apr 15 2020, 15:37

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.

This revision now requires changes to proceed.Apr 15 2020, 15:37

Made the test less functional and more loopy.

deadalnix requested changes to this revision.Apr 16 2020, 13:12
deadalnix added inline comments.
src/test/op_reversebytes_tests.cpp
88 ↗(On Diff #18855)

This is not a good name. You can also drop the reverse test case thing and just pass two values.

137 ↗(On Diff #18855)

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 ↗(On Diff #18855)

Just create a test case with all the flag instead of woving this all over the place.

This revision now requires changes to proceed.Apr 16 2020, 13:12

I'm not sure if this is what you want but it does look cleaner now, so maybe.

deadalnix requested changes to this revision.Apr 17 2020, 00:26

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.

This revision now requires changes to proceed.Apr 17 2020, 00:26

ok ok i admit this is cleaner now :rolling_eyes:

This revision is now accepted and ready to land.Apr 18 2020, 07:30