Recent compilers compile the two new functions to very efficient code on various platforms. In particular, already GCC >= 5 and clang >= 5 understand do this for the read function, which is the one critical for performance (called 16 times per SHA256 transform).
Fixes #1080.
Recent compilers compile the two new functions to very efficient code
on various platforms. In particular, already GCC >= 5 and clang >= 5
understand do this for the read function, which is the one critical
for performance (called 16 times per SHA256 transform).
Fixes #1080.
ACK 37d36927dff793e61df6d6f6a6c50e8fa2519e33
Glad you caught the int promotion issue when shifting. Sanity checked by searching for strings BE32, ENDIAN, chunk, ->buf to confirm nothing else needed to be changed. Benchmark anecdata on my machine looks reasonable:
0# 1093
1hash_sha256 , 0.197 , 0.198 , 0.202
2hash_hmac_sha256 , 0.786 , 0.788 , 0.798
3hash_rfc6979_hmac_sha256 , 4.29 , 4.30 , 4.31
4
5# master
6hash_sha256 , 0.212 , 0.214 , 0.216
7hash_hmac_sha256 , 0.841 , 0.846 , 0.854
8hash_rfc6979_hmac_sha256 , 4.30 , 4.50 , 4.64
312@@ -338,4 +313,20 @@ static SECP256K1_INLINE int secp256k1_ctz64_var(uint64_t x) {
313 #endif
314 }
315
316+/* Read a uint32_t in big endian */
317+SECP256K1_INLINE static uint32_t secp256k1_read_be32(const unsigned char* p) {
318+ return (uint32_t)p[0] << 24 |
like this:
0(((uint32_t)p[0]) << 24) |
1(((uint32_t)p[1]) << 16) |
2(((uint32_t)p[2]) << 8) |
3((uint32_t)p[3])
is much easier to read IMO
Yeah, I agree, it may be easier to review for some reviewers because they don’t need to to spend time looking up the rules. But given that it has been merged (i.e., review has been done), I don’t think it’s worth spending a PR on this. Feel free to open a PR if you disagree.
(Sorry, I hadn’t seen your comment earlier. I was spending some time testing this locally before pushing the merge. Your comment arrived after I had created the merge commit locally, but I believe before I pushed it. Then I didn’t check github again before pushing the merge.)
