Add missing byteswap functions for MSVC #29036

1. 
   theuni commented at 6:56 pm on December 8, 2023: member

   This should give a speedup across the board for MSVC builds.

   While working on modernizing our byteswapping code for c++20, we noticed that MSVC uses our hand-written byteswap functions, as opposed to using libc/compiler versions like almost all other platforms.

   aureleoules did some great benchmarks in #28674 which show that these hand-written byteswaps often compile down to a slow mess. hebasto confirmed that we’re indeed hitting these paths for MSVC.

   Quick tests with godbolt show that MSVC’s provided _byteswap_* indeed speed things up.

2. Add missing byteswap functions for MSVC

   This should give a significant speedup across the board.

   bbc7203312
3. 
   DrahtBot commented at 6:56 pm on December 8, 2023: 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	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:

   • #28674 ([POC] C++20 std::endian by fanquake)

   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.

4. theuni marked this as a draft on Dec 8, 2023
5. 
   theuni commented at 6:59 pm on December 8, 2023: member

   Converted this to draft because I haven’t actually tested the compile for MSVC (relying on c-i :( ), and because we’ll need solid benchmarks before making any decisions.
6. 
   hebasto commented at 7:05 pm on December 8, 2023: member

   Concept ACK on removing another MSVC-specific performance pessimization from the code.
7. 
   hebasto commented at 1:12 pm on December 9, 2023: member

   Converted this to draft because I haven’t actually tested the compile for MSVC (relying on c-i :( )

   It compiles OK.

   and because we’ll need solid benchmarks before making any decisions.

   What subset of benchmarks we might consider representative enough?

   For now, I’m observing very tiny (a couple of %) improvements in some of them.

   Also we should remember that MSVC build uses no optimisation (see e94ae810a55da16fce6040714677b9d50781bebd).

8. 
   fanquake commented at 2:16 pm on December 12, 2023: member

   cc @sipsorcery.

   Also we should remember that MSVC build uses no optimisation

   Could rebase on master now that #29045 has gone in.

9. 
   sipsorcery commented at 9:26 pm on December 12, 2023: member

   Builds fine for me on Windows 11 and msvc v19.38.33130 x64 (Visual Studio 2022).

   I ran bench_bitcoin.exe but not sure what differences I should see.

10. 
    maflcko commented at 9:41 pm on December 12, 2023: member

    See #28674 (comment) for benches that could make a difference.

    But this requires a rebase on master first, see

    Could rebase on master now that #29045 has gone in.

11. DrahtBot added the label CI failed on Jan 16, 2024
12. 
    theuni commented at 7:50 pm on January 17, 2024: member

    Obsoleted by #29263.
13. fanquake closed this on Jan 17, 2024

    ---

Contributors
theuni DrahtBot hebasto fanquake sipsorcery maflcko

Labels
CI failed