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

1. 
   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
2. 
   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

   No conflicts as of last run.

3. 
   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.
4. 
   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?
5. 
   hebasto commented at 2:24 pm on April 11, 2024: member

   Concept ACK.
6. 
   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

7. DrahtBot added the label CI failed on Apr 11, 2024
8. 
   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.

9. 
   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.

10. 
    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.

11. 
    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.

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

    What benchmarks might be appropiate for testing changes like these?
13. 
    maflcko commented at 10:52 am on April 16, 2024: member

    What benchmarks might be appropiate for testing changes like these?

    Microbenchmarks + IBD?

14. 
    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.
15. 
    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

16. 
    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

    

17. 
    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 :)

18. 
    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.
19. 
    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)?
20. fanquake force-pushed on May 22, 2024
21. DrahtBot added the label Needs rebase on Aug 28, 2024
22. 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
23. build: remove LEVELDB_IS_BIG_ENDIAN 4eb44fa9c1
24. crc32c: remove BYTE_ORDER_BIG_ENDIAN

    Similar to 038755784d88ce7522ac2f98e8ba138010a64f82 from leveldb.

    69c3841888
25. build: remove BYTE_ORDER_BIG_ENDIAN 83c78d5aac
26. fanquake force-pushed on Aug 28, 2024
27. 
    fanquake commented at 2:35 pm on August 28, 2024: member

    Rebased, and switched the changes to CMake.
28. DrahtBot removed the label Needs rebase on Aug 28, 2024

Contributors
fanquake DrahtBot theuni hebasto maflcko sipsorcery

Labels
CI failed