[WIP] build: remove need to test for endianness #29852

pull fanquake wants to merge 4 commits into bitcoin:master from fanquake:leveldb_cherrypicks changing 10 files +4 −87

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:
- https://github.com/bitcoin-core/leveldb-subtree/pull/40
- https://github.com/bitcoin-core/crc32c-subtree/pull/7
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

For detailed information about the code coverage, see the test coverage report.

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

Reviewers, this pull request conflicts with the following ones:
- #30905 (cmake: Switch to crc32c upstream build system by hebasto)
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 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: 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

Most likely fruitless but can’t hurt to ask.

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

Type	Reviewers
Concept ACK	theuni, hebasto

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

Remove leveldb::port::kLittleEndian.

Clang 10 includes the optimizations described in
https://bugs.llvm.org/show_bug.cgi?id=41761. This means that the
platform-independent implementations of {Decode,Encode}Fixed{32,64}()
compile to one instruction on the most recent Clang and GCC.

PiperOrigin-RevId: 306330166

847b168200

build: remove LEVELDB_IS_BIG_ENDIAN 4eb44fa9c1

crc32c: remove BYTE_ORDER_BIG_ENDIAN

Similar to 038755784d88ce7522ac2f98e8ba138010a64f82 from leveldb.

69c3841888

build: remove BYTE_ORDER_BIG_ENDIAN 83c78d5aac
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

Contributors
fanquake DrahtBot theuni hebasto maflcko sipsorcery

Labels
CI failed

[WIP] build: remove need to test for endianness #29852

Code Coverage

Reviews

Conflicts