serialization: c++20 endian/byteswap/clz modernization #29263
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
theuni
force-pushed
the
cxx_20_endian2
branch
from
6d28845
to
60be777
Compare
|
Here's a godbolt test that shows how various compilers perform: https://gcc.godbolt.org/z/nTadqEP83 To test with/without builtins, comment/un-comment the From my tests:
|
theuni
force-pushed
the
cxx_20_endian2
branch
from
60be777
to
4221e05
Compare
|
Added a quick note about |
|
Concept ACK - I think moving to using the builtins is ok. With the eventual plan to migrate to @aureleoules everytime I visit the coverage/benchmarks for this PR, I get a 500 error. Can you take a look? |
|
@fanquake thanks for reaching out, I fixed the issue. |
|
@aureleoules Thanks! These numbers really don't make much sense to me and don't match what I've seen locally, but I'll work on trying to reproduce. |
|
From some more local tests, it looks like it's the clz changes slowing things down for whatever reason. Still investigating. |
theuni
force-pushed
the
cxx_20_endian2
branch
from
4221e05
to
2c1a73e
Compare
|
I've dropped the clz changes as a test and kept only the endian/byteswap change. Locally on my machine the benchmarks look the same before and after. If the corecheck benchmarks look better I'll update the title/description here. Edit: looks good. |
|
Aha, I finally tracked down the culprit! No wonder the benchmarks weren't making sense! The problem was the removal of this from #if defined(HAVE_CONFIG_H)
#include <config/bitcoin-config.h>
#endifTurns out, I'm going to put the clz changes back to match the PR title/description, but this time with the include fixed up. Hopefully after that the benchmarks will make sense and all will be good :) I suppose there might be other compilation units that were depending on the include indirectly as well. I'll figure out a way to check for that. |
theuni
force-pushed
the
cxx_20_endian2
branch
from
2c1a73e
to
6c84885
Compare
theuni
force-pushed
the
cxx_20_endian2
branch
from
6c84885
to
7ffc688
Compare
|
Converted to a draft while I'm still messing with this. I added a new commit to add the The benchmarks still show some regressions, though it's not nearly as bad as it was before. Will continue poking. |
theuni
force-pushed
the
cxx_20_endian2
branch
from
7ffc688
to
cfbf466
Compare
|
I've created #26972 to track ideas on how to detect those silent build issues, or at least make them easier to spot. |
|
I've pushed a commit which temporarily puts the includes back in the low-level headers for the sake of addressing the benchmarks first, setting aside the possibility of missing defines. @aureleoules's benchmarks show 2 small regressions, though I am unable to reproduce those locally. My tests show: This PR: Essentially no change. I'm curious if @maflcko sees the same? |
|
@theuni yeah the You can see ignored benchmarks here in the meantime: https://github.com/corecheck/frontend/blob/master/src/routes/%5Bowner%5D/%5Brepo%5D/pulls/%5Bnumber%5D/Benchmarks.svelte#L334 |
|
@aureleoules Thanks, that's helpful. |
|
Updated to split the last commit as suggested by @maflcko . I also replaced |
theuni
force-pushed
the
cxx_20_endian2
branch
from
7d87065
to
1d051e8
Compare
Guix builds (on x86_64)
|
There was a problem hiding this comment.
ACK 1d051e8 📛
Show signature
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 1d051e86098e721a47cebf7cfd73d82aa414c05e 📛
6Obp6pvdrMxN7MXEPLAuvZe0UCy9RzNtRu/VNpLvSGe6P6thBW12J6z7gacvZBa1MGOdmgJFjbeCHc8MjWTyDA==
Rather than a complicated set of tests to decide which bswap functions to use, always prefer the compiler built-ins when available. These builtins and fallbacks can all be removed once we're using c++23, which adds std::byteswap.
These replace our platform-specific mess in favor of c++20 endian detection via std::endian and internal byteswap functions when necessary. They no longer rely on autoconf detection.
theuni
force-pushed
the
cxx_20_endian2
branch
from
1d051e8
to
86b7f28
Compare
|
ACK 86b7f28 📘 Show signatureSignature: |
…n with c++20 concept ad7584d serialization: replace char-is-int8_t autoconf detection with c++20 concept (Cory Fields) Pull request description: Doesn't depend on #29263, but it's really only relevant after that one's merged. This removes the only remaining autoconf macro in our serialization code (after #29263), so it can now be used trivially and safely out-of-tree. ~Our code does not currently contain any concepts, but couldn't find any discussion or docs about avoiding them. I guess we'll see if this blows up our c-i.~ Edit: Ignore this. ajtowns pointed out that we're already using a few concepts. This was introduced in #13580. Please check my logic on this as I'm unable to test on a SmartOS system. Even better would be a confirmation from someone who can build there. ACKs for top commit: Empact: Code review ACK ad7584d Tree-SHA512: 1faf65c900700efb1cf3092c607a2230321b393cb2f029fbfb94bc8e50df1dabd7a9e4b91e3b34f0d2f3471aaf18ee7e56d91869db5c5f4bae84da95443e1120
…ESSTHANOREQUAL64, OP_GREATERTHAN64, OP_GREATERTHANOREQUAL64, OP_SCRIPTNUMTOLE64, OP_LE64TOSCRIPTNUM, OP_LE32TOLE64 Remove liquid args WIP Get simple OP_1 functional test case working Get tests for arithmetic and comparison opcodes working Get all functional tests passing Rename test case to Arithmetic64bitTest Rename file to feature_64bit_arithmetic_opcodes.py, add it to test_runner.py Get tests passing in feature_taproot.py Remove unused push_le4 Revert test fixture setup Cleanup Fix linting test: Add leaf_version parameter to taproot_tree_helper() Fix bug Fix bugs Fix compile Fix missing sigversion checks Fix htole64 -> htole64_internal due to bitcoin#29263
THis change mirrors changes from bitcoin#29263.
This change mirrors changes from bitcoin#29263.
8e17f00 build, msvc: Cleanup `bitcoin_config.h.in` (Hennadii Stepanov) Pull request description: This PR mirrors changes from #29263 into the MSVC build system. ACKs for top commit: fanquake: ACK 8e17f00 Tree-SHA512: b8e5cca015ff112c2969a60436524e97007ff2c559b3c12425d0549af694b16248311cc3e7c33f798bc095a679933641496836bb846eee6a2a377956ef53f56e
This change mirrors changes from bitcoin#29263.
This change mirrors changes from bitcoin#29263.
…ESSTHANOREQUAL64, OP_GREATERTHAN64, OP_GREATERTHANOREQUAL64, OP_SCRIPTNUMTOLE64, OP_LE64TOSCRIPTNUM, OP_LE32TOLE64 Remove liquid args WIP Get simple OP_1 functional test case working Get tests for arithmetic and comparison opcodes working Get all functional tests passing Rename test case to Arithmetic64bitTest Rename file to feature_64bit_arithmetic_opcodes.py, add it to test_runner.py Get tests passing in feature_taproot.py Remove unused push_le4 Revert test fixture setup Cleanup Fix linting test: Add leaf_version parameter to taproot_tree_helper() Fix bug Fix bugs Fix compile Fix missing sigversion checks Fix htole64 -> htole64_internal due to bitcoin#29263
33a454e fixup! cmake: Check system symbols (Hennadii Stepanov) 3cb2e65 fixup! cmake: Check system headers (Hennadii Stepanov) Pull request description: This PR backports build system changes from bitcoin#29263. ACKs for top commit: pablomartin4btc: ACK 33a454e Tree-SHA512: 1793c6504a7190134c0ce075e959d22c4a3640d54a4d141f5117975bed267952cc8c7da488426e48022eba1eb77d3353783d77a20907b0cfa183e0b68d824133
This change mirrors changes from bitcoin/bitcoin#29263.
…ESSTHANOREQUAL64, OP_GREATERTHAN64, OP_GREATERTHANOREQUAL64, OP_SCRIPTNUMTOLE64, OP_LE64TOSCRIPTNUM, OP_LE32TOLE64 Remove liquid args WIP Get simple OP_1 functional test case working Get tests for arithmetic and comparison opcodes working Get all functional tests passing Rename test case to Arithmetic64bitTest Rename file to feature_64bit_arithmetic_opcodes.py, add it to test_runner.py Get tests passing in feature_taproot.py Remove unused push_le4 Revert test fixture setup Cleanup Fix linting test: Add leaf_version parameter to taproot_tree_helper() Fix bug Fix bugs Fix compile Fix missing sigversion checks Fix htole64 -> htole64_internal due to bitcoin#29263
…ESSTHANOREQUAL64, OP_GREATERTHAN64, OP_GREATERTHANOREQUAL64, OP_SCRIPTNUMTOLE64, OP_LE64TOSCRIPTNUM, OP_LE32TOLE64 Remove liquid args WIP Get simple OP_1 functional test case working Get tests for arithmetic and comparison opcodes working Get all functional tests passing Rename test case to Arithmetic64bitTest Rename file to feature_64bit_arithmetic_opcodes.py, add it to test_runner.py Get tests passing in feature_taproot.py Remove unused push_le4 Revert test fixture setup Cleanup Fix linting test: Add leaf_version parameter to taproot_tree_helper() Fix bug Fix bugs Fix compile Fix missing sigversion checks Fix htole64 -> htole64_internal due to bitcoin#29263
…ESSTHANOREQUAL64, OP_GREATERTHAN64, OP_GREATERTHANOREQUAL64, OP_SCRIPTNUMTOLE64, OP_LE64TOSCRIPTNUM, OP_LE32TOLE64 Remove liquid args WIP Get simple OP_1 functional test case working Get tests for arithmetic and comparison opcodes working Get all functional tests passing Rename test case to Arithmetic64bitTest Rename file to feature_64bit_arithmetic_opcodes.py, add it to test_runner.py Get tests passing in feature_taproot.py Remove unused push_le4 Revert test fixture setup Cleanup Fix linting test: Add leaf_version parameter to taproot_tree_helper() Fix bug Fix bugs Fix compile Fix missing sigversion checks Fix htole64 -> htole64_internal due to bitcoin#29263
This replaces #28674, #29036, and #29057. Now ready for testing and review.
Replaces platform-specific endian and byteswap functions. This is especially useful for kernel, as it means that our deep serialization code no longer requires bitcoin-config.h.
I apologize for the size of the last commit, but it's hard to avoid making those changes at once.
All platforms now use our internal functions rather than libc or platform-specific ones, with the exception of MSVC.
Sadly, benchmarking showed that not all compilers are capable of detecting and optimizing byteswap functions, so compiler builtins are instead used where possible. However, they're now detected via macros rather than autoconf checks.
This matches how libc++ implements std::byteswap for c++23.
I suggest we move/rename
compat/endian.h, but I left that out of this PR to avoid bikeshedding.#29057 pointed out some irregularities in benchmarks. After messing with various compilers and configs for a few weeks with these changes, I'm of the opinion that we can't win on every platform every time, so we should take the code that makes sense going forward. That said, if any real-world slowdowns are caused here, we should obviously investigate.