build: remove need to test for endianness

fanquake commented at 1:40 pm on April 11, 2024: member

We can cherry-pick one commit from upstream leveldb, make the same change in crc32c, and then ultimately drop our build infra for testing endianness.

Not for merging until subtrees are updated:

DrahtBot commented at 1:40 pm on April 11, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29852.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	theuni, hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

theuni commented at 2:18 pm on April 11, 2024: member

Concept ACK. Looking at the leveldb godbolt link, this is nicely optimized everywhere except MSVC. I’m ok with a possible regression there for the sake of the cleanup.

theuni commented at 2:20 pm on April 11, 2024: member

Want to upstream the crc32 patch to match the others we have sitting there?

hebasto commented at 2:24 pm on April 11, 2024: member

Concept ACK.

DrahtBot commented at 4:35 pm on April 11, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/23709865964

DrahtBot added the label CI failed on Apr 11, 2024

hebasto commented at 11:06 pm on April 14, 2024: member

Concept ACK. Looking at the leveldb godbolt link, this is nicely optimized everywhere except MSVC.

However, the changes in MSVC generated assembly code look quite significant.

I’m ok with a possible regression there for the sake of the cleanup.

I disagree. Before stacking another performance deterioration change on top of the pile of the currently unresolved performance issues in the MSVC builds, it would be nice to compare benchmarks in the first place.

fanquake commented at 10:49 am on April 15, 2024: member

However, the changes in MSVC generated assembly code look quite significant. Before stacking another performance deterioration change on top of the pile

Isn’t that because optimisations haven’t been turned on? Otherwise, can you provide a concrete example of what you’re talking about.

fanquake commented at 1:55 pm on April 15, 2024: member

Want to upstream the crc32 patch to match the others we have sitting there?

Sure. Opened a PR in our crc32c subtree fork: https://github.com/bitcoin-core/crc32c-subtree/pull/7, and one in Google upstream: https://github.com/google/crc32c/pull/64.

hebasto commented at 10:24 am on April 16, 2024: member

However, the changes in MSVC generated assembly code look quite significant. Before stacking another performance deterioration change on top of the pile

Isn’t that because optimisations haven’t been turned on? Otherwise, can you provide a concrete example of what you’re talking about.

https://godbolt.org/z/of4T8hM8j provides examples with the /O2 optimization flag.

hebasto commented at 10:29 am on April 16, 2024: member

What benchmarks might be appropiate for testing changes like these?

maflcko commented at 10:52 am on April 16, 2024: member

What benchmarks might be appropiate for testing changes like these?

Microbenchmarks + IBD?

theuni commented at 5:36 pm on April 17, 2024: member

@hebasto Is there a venue for reporting this to MSVC? They recently patted themselves on the back for detecting similar patterns. It’s a shame MSVC can’t detect something that (in 2024) seems so obvious.

hebasto commented at 6:38 pm on April 17, 2024: member

Is there a venue for reporting this to MSVC? They recently patted themselves on the back for detecting similar patterns. It’s a shame MSVC can’t detect something that (in 2024) seems so obvious.

cc @sipsorcery

sipsorcery commented at 8:00 pm on April 17, 2024: contributor

Is there a venue for reporting this to MSVC? They recently patted themselves on the back for detecting similar patterns. It’s a shame MSVC can’t detect something that (in 2024) seems so obvious.

cc @sipsorcery

Most likely fruitless but can’t hurt to ask.

https://x.com/sipsorcery/status/1780687316522266853

hebasto commented at 1:16 pm on April 19, 2024: member

We can cherry-pick one commit from upstream leveldb, make the same change in crc32c, and then ultimately drop our build infra for testing endianness.

The same goal, which is dropping “build infra for testing endianness”, might be achieved with an alternative approach, which essentially boils down to:

 0--- a/src/leveldb/util/coding.h
 1+++ b/src/leveldb/util/coding.h
 2@@ -62,7 +62,7 @@ char* EncodeVarint64(char* dst, uint64_t value);
 3 inline void EncodeFixed32(char* dst, uint32_t value) {
 4   uint8_t* const buffer = reinterpret_cast<uint8_t*>(dst);
 5 
 6-  if (port::kLittleEndian) {
 7+  if constexpr (std::endian::native == std::endian::little) {
 8     // Fast path for little-endian CPUs. All major compilers optimize this to a
 9     // single mov (x86_64) / str (ARM) instruction.
10     std::memcpy(buffer, &value, sizeof(uint32_t));

And no MSVC code degradation :)

fanquake commented at 1:27 pm on April 19, 2024: member

I don’t think that’s better. We have to keep all the redundant code, and now we are diverging from upstream for no reason.

fanquake commented at 1:36 pm on April 19, 2024: member

Also, you’re just moving the endianess testing into the code. The point is to drop all of this, and use generic code that doesn’t require any tests at all. The fact that MSVC fails to perform basic optimisations is annoying, but I don’t really see why it’s a blocker here. If we’d just pulled the subtree, and this change came in as part of that, I doubt anyone would have even noticed anything MSVC related (still haven’t seen any benchmarks showing any meaningful difference for this change as-is)?

fanquake force-pushed on May 22, 2024

DrahtBot added the label Needs rebase on Aug 28, 2024

fanquake force-pushed on Aug 28, 2024

fanquake commented at 2:35 pm on August 28, 2024: member

Rebased, and switched the changes to CMake.

DrahtBot removed the label Needs rebase on Aug 28, 2024

theuni commented at 7:12 pm on December 5, 2024: member

if constexpr (std::endian::native == std::endian::little) {

This is a c++20 feature unfortunately. So I don’t imagine either upstream accepting it any time soon.

I agree with @fanquake that we shouldn’t let MSVC (an unsupported and closed-source compiler) stand in the way of our progress. And this is a real barrier to us staying in sync with upstream. If we shipped msvc-built binaries that’d be one thing, but I don’t see that ever happening.

fanquake force-pushed on Dec 6, 2024

B5OFT commented at 12:45 pm on December 10, 2024: none

Utilize pre-existing functions or develop a utility function to manage endianess.

fanquake referenced this in commit 59669817c5 on Jan 15, 2025

maflcko commented at 3:37 pm on January 15, 2025: member

MSVC

Has someone reported the request to them? If not, it seems less likely they’ll fix it.

hebasto commented at 10:17 am on January 21, 2025: member

MSVC

Has someone reported the request to them? If not, it seems less likely they’ll fix it.

https://developercommunity.visualstudio.com/t/Missed-optimization-for-consecutive-byte/10831400 @theuni

… MSVC (an unsupported … compiler)…

Since when?

maflcko commented at 11:02 am on January 21, 2025: member

MSVC

Has someone reported the request to them? If not, it seems less likely they’ll fix it.

https://developercommunity.visualstudio.com/t/Missed-optimization-for-consecutive-byte/10831400

Thx. Could also add a link (https://github.com/google/leveldb/commit/201f52201f5dd9701e7a8ceaa0ec4d344e69e022) to the thread to give one example that the code is used in the real world by a real software project?

hebasto commented at 12:04 pm on January 21, 2025: member

MSVC

Has someone reported the request to them? If not, it seems less likely they’ll fix it.

https://developercommunity.visualstudio.com/t/Missed-optimization-for-consecutive-byte/10831400

Thx. Could also add a link (google/leveldb@201f522) to the thread to give one example that the code is used in the real world by a real software project?

https://developercommunity.visualstudio.com/t/Missed-optimization-for-consecutive-byte/10831400#T-N10831434

fanquake referenced this in commit 8fc7140846 on Jan 21, 2025

fanquake force-pushed on Jan 21, 2025

fanquake renamed this:
~~[WIP] build: remove need to test for endianness~~
build: remove need to test for endianness
on Jan 21, 2025

l0rinc commented at 8:36 pm on January 23, 2025: contributor

Microbenchmarks + IBD?

I ran a reindex-chainstate until 880k - to check for correctness and speed. There was no obvious difference between runs (2 master vs 2 this PR).

 0hyperfine \
 1--runs 2 \
 2--parameter-list COMMIT 523520f8279987cd528a9e2db6db13dc56641eff,72aa32fbae34c6fe151d3ab531974d6992dd065e \
 3--prepare 'rm -f /mnt/my_storage/BitcoinData/debug.log && git checkout {COMMIT} && git clean -fxd && git reset --hard \
 4&& cmake -B build -DCMAKE_BUILD_TYPE=Release -DBUILD_UTIL=OFF -DBUILD_TX=OFF -DBUILD_TESTS=OFF -DENABLE_WALLET=OFF -DINSTALL_MAN=OFF && cmake --build build -j$(nproc)' \
 5--cleanup 'mv /mnt/my_storage/BitcoinData/debug.log /mnt/my_storage/logs/debug-{COMMIT}.log' \
 6'COMMIT={COMMIT} ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=880000 -dbcache=30000 -reindex-chainstate -connect=0'
 7
 8Benchmark 1: COMMIT=523520f8279987cd528a9e2db6db13dc56641eff ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=880000 -dbcache=30000 -reindex-chainstate -connect=0
 9  Time (mean ± σ):     23763.799 s ± 230.232 s    [User: 37683.470 s, System: 693.079 s]
10  Range (min … max):   23601.001 s … 23926.598 s    2 runs
11 
12Benchmark 2: COMMIT=72aa32fbae34c6fe151d3ab531974d6992dd065e ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=880000 -dbcache=30000 -reindex-chainstate -connect=0
13  Time (mean ± σ):     23804.995 s ± 97.138 s    [User: 37743.594 s, System: 690.682 s]
14  Range (min … max):   23736.308 s … 23873.682 s    2 runs
15 
16Summary
17  COMMIT=523520f8279987cd528a9e2db6db13dc56641eff ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=880000 -dbcache=30000 -reindex-chainstate -connect=0 ran
18    1.00 ± 0.01 times faster than COMMIT=72aa32fbae34c6fe151d3ab531974d6992dd065e ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=880000 -dbcache=30000 -reindex-chainstate -connect=0

I did however struggle to run these tests in a simulated big endian env (tried the setup I documented at #31344 (comment)), I’m getting:

 096% tests passed, 6 tests failed out of 136
 1
 2Total Test time (real) = 283.13 sec
 3
 4The following tests FAILED:
 5          1 - util_test_runner (Failed)
 6          3 - univalue_test (Not Run)
 7          4 - univalue_object_test (Not Run)
 8          5 - secp256k1_noverify_tests (Not Run)
 9          6 - secp256k1_tests (Not Run)
10          7 - secp256k1_exhaustive_tests (Not Run)

Not sure if this passes for latest master (it did, ~2 months ago, on my previous laptop) - will experiment more, but these runs are extremely slow.

maflcko commented at 8:44 pm on January 23, 2025: member

Not sure if this passes for latest master (it did, ~2 months ago, on my previous laptop) - will experiment more, but these runs are extremely slow.

qemu is expected to be slow. For reference, there was a recent change (https://github.com/bitcoin/bitcoin/pull/31657), so this may now be easier to run on non-linux machines, but I haven’t tried. Running the s390x CI task should cover the changes here.

l0rinc commented at 3:53 pm on January 25, 2025: contributor

Running the s390x CI task should cover the changes here

I managed to run ctest with master and this branch (I have updated the instructions in #31344 (comment)).

But running functional/test_runner.py reveals a few failures that seem related to endianness changes, e.g. rpc_bind.py fails with:

AssertionError: not({(‘0100007f’, 19192)} == {(‘7f000001’, 19192)})

 0stdout:
 12025-01-25T15:47:08.065000Z TestFramework (INFO): PRNG seed is: 3770749326237847101
 22025-01-25T15:47:08.076000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20250125_164236/rpc_bind_266
 32025-01-25T15:47:08.079000Z TestFramework (INFO): Check for ipv6
 42025-01-25T15:47:08.080000Z TestFramework (INFO): Check for non-loopback interface
 52025-01-25T15:47:08.082000Z TestFramework (INFO): Bind test for ['127.0.0.1']
 62025-01-25T15:47:08.657000Z TestFramework (ERROR): Assertion failed
 7Traceback (most recent call last):
 8  File "/mnt/bitcoin/test/functional/test_framework/test_framework.py", line 135, in main
 9    self.run_test()
10  File "/mnt/bitcoin/build_dev_mode/test/functional/rpc_bind.py", line 99, in run_test
11    self._run_loopback_tests()
12  File "/mnt/bitcoin/build_dev_mode/test/functional/rpc_bind.py", line 110, in _run_loopback_tests
13    self.run_bind_test(['127.0.0.1'], '127.0.0.1', ['127.0.0.1'],
14  File "/mnt/bitcoin/build_dev_mode/test/functional/rpc_bind.py", line 45, in run_bind_test
15    assert_equal(set(get_bind_addrs(pid)), set(expected))
16  File "/mnt/bitcoin/test/functional/test_framework/util.py", line 77, in assert_equal
17    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
18AssertionError: not({('0100007f', 19192)} == {('7f000001', 19192)})
192025-01-25T15:47:08.717000Z TestFramework (INFO): Stopping nodes

crc32c: remove BYTE_ORDER_BIG_ENDIAN

Similar to 038755784d88ce7522ac2f98e8ba138010a64f82 from leveldb.

86fa5cc03a

build: remove BYTE_ORDER_BIG_ENDIAN 11bc8e9ef3

fanquake force-pushed on Feb 20, 2025

build: remove need to test for endianness #29852

Code Coverage & Benchmarks

Reviews

Conflicts