cryptonote_basic: faster and more readable is_valid_decomposed_amount

[jeffro256]

No description provided.

[0xFFFC0000]

Left a comment on while loop.

[0xFFFC0000]

Thanks @jeffro256 for this PR. It is great. I have a few comments that we can talk about.

a. It would be great if we could add a small benchmark, and test included in this PR.
b. While a while loop is simple and easy to read. I believe we can test the algorithm performance:

inline // better to have FORCEINLINE here
int c_t_z(uint64_t n) {
    int zeros = 0;
    if (n % 10000000000000000ULL == 0) { zeros += 16; n /= 10000000000000000ULL; }
    if (n % 100000000 == 0) { zeros += 8; n /= 100000000; }
    if (n % 10000 == 0) { zeros += 4; n /= 10000; }
    if (n % 100 == 0) { zeros += 2; n /= 100; }
    if (n % 10 == 0) { zeros += 1; }
    return zeros;
}
.
.
.
amount >>= zeros;

I am not sure how something like this would perform in comparison with the while loop. But worth looking at. Code size-wise, of course while version wins, but performance-wise I would look into the if implementation [1].

I will ping you on Matrix, to talk about these, even maybe we can work together on this.

Overall everything is clean, and ready to merge after making sure that we are not missing something.

1. https://godbolt.org/z/E3fxaf8Wh

[jeffro256]

Add a performance test suite and used fast power decomposition.

Results:

Perf test applied to master:

test_is_valid_decomposed_amount (3 calls) - OK: 5724 ms/call
Tests finished. Elapsed time: 17 sec

PR:

test_is_valid_decomposed_amount (3 calls) - OK: 2672 ms/call
Tests finished. Elapsed time: 8 sec

[0xFFFC0000]

Add a performance test suite and used fast power decomposition.

Results:

Perf test applied to master:

test_is_valid_decomposed_amount (3 calls) - OK: 5724 ms/call
Tests finished. Elapsed time: 17 sec

PR:

test_is_valid_decomposed_amount (3 calls) - OK: 2672 ms/call
Tests finished. Elapsed time: 8 sec

Just out of curiosity, do you have perf numbers for loop version too?

Overall it is now very clean. Thanks for considering my suggestion :)

[jeffro256]

Was just about to test that when you commented lol

[jeffro256]

I'm reverting back to the loop after performance testing since the compiler apparently was able to beat me. Using a simple while made the code easier to read and speeds it up almost 2x compared to the unrolled version (which itself is 2x faster than the lookup table).

With loop:

test_is_valid_decomposed_amount (3 calls) - OK: 1506 ms/call
Tests finished. Elapsed time: 4 sec

[0xFFFC0000]

Great to have the performance test and numbers too. I am fine with loop version too.

Overall, everything is good. Running the test locally in few hours, and will approve once the local run finish.

[0xFFFC0000]

Test are running correctly too. Thanks.