std::endian
#28674
Using std::endian from C++20 allows us to drop some amount of own code, including infra in the build system (which means we don’t have to port and review it for CMake, only to delete it shortly after switching to C++20).
Note that C++23 would take this even further, with the introduction of std::byteswap.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See 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.
Reviewers, 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.
I’ve been experimenting with benchmark comparisons between pulls and master on my test coverage tool corecheck (https://corecheck.dev/bitcoin/bitcoin/pulls/28674).
It seems that this pull request is negatively impacting a bunch of benchmarks:
Note that the benchmarks are being run on ARM64 CPUs, but I tested locally on an x86 and the performance loss is roughly the same.
@aureleoules Thanks for testing!
I tested this by comparing the x86_64 asm output of a simple test file and confirmed that it compiled down to bswaps as necessary. I didn’t do the same for other arches. Perhaps others are missing the critical optimizations? :(
Could you say more about your compiler/flags? I would expect to see the nasty behavior you’re seeing:
Would you mind pasting the flags use for compile? Without -O2 or better, for example, I would expect these results.
-O2 on Ubuntu Jammy)
I’d presume it is corecheck/coverage-worker@7d47674/entrypoint.sh#L58 (default flags -O2 on Ubuntu Jammy)
Yes that is correct. Also the compiler installed is g++-11.
Here is the output of the configure script of the corecheck job if this helps:
01701686294337,checking for pkg-config... /usr/bin/pkg-config
11701686294338,checking pkg-config is at least version 0.9.0... yes
21701686294378,checking build system type... aarch64-unknown-linux-gnu
31701686294379,checking host system type... aarch64-unknown-linux-gnu
41701686294384,checking for a BSD-compatible install... /usr/bin/install -c
51701686294387,checking whether build environment is sane... yes
61701686294393,checking for a race-free mkdir -p... /usr/bin/mkdir -p
71701686294393,checking for gawk... no
81701686294393,checking for mawk... mawk
91701686294398,checking whether make sets $(MAKE)... yes
101701686294402,checking whether make supports nested variables... yes
111701686294406,checking whether to enable maintainer-specific portions of Makefiles... yes
121701686294406,checking whether make supports nested variables... (cached) yes
131701686294406,checking for g++... g++
141701686294456,checking whether the C++ compiler works... yes
151701686294456,checking for C++ compiler default output file name... a.out
161701686294490,checking for suffix of executables...
171701686294529,checking whether we are cross compiling... no
181701686294545,checking for suffix of object files... o
191701686294558,checking whether the compiler supports GNU C++... yes
201701686294573,checking whether g++ accepts -g... yes
211701686294631,checking for g++ option to enable C++11 features... none needed
221701686294636,checking whether make supports the include directive... yes (GNU style)
231701686294660,checking dependency style of g++... gcc3
241701686294749,checking whether g++ supports C++20 features with -std=c++20... yes
251701686294771,checking whether the compiler supports GNU Objective C++... no
261701686294795,checking whether g++ -std=c++20 accepts -g... no
271701686294818,checking dependency style of g++ -std=c++20... gcc3
281701686294820,checking how to print strings... printf
291701686294820,checking for gcc... gcc
301701686294854,checking whether the compiler supports GNU C... yes
311701686294867,checking whether gcc accepts -g... yes
321701686294912,checking for gcc option to enable C11 features... none needed
331701686294938,checking whether gcc understands -c and -o together... yes
341701686294961,checking dependency style of gcc... gcc3
351701686294964,checking for a sed that does not truncate output... /usr/bin/sed
361701686294966,checking for grep that handles long lines and -e... /usr/bin/grep
371701686294967,checking for egrep... /usr/bin/grep -E
381701686294968,checking for fgrep... /usr/bin/grep -F
391701686294972,checking for ld used by gcc... /usr/bin/ld
401701686294973,checking if the linker (/usr/bin/ld) is GNU ld... yes
411701686294975,checking for BSD- or MS-compatible name lister (nm)... /usr/bin/nm -B
421701686294991,checking the name lister (/usr/bin/nm -B) interface... BSD nm
431701686294991,checking whether ln -s works... yes
441701686294994,checking the maximum length of command line arguments... 1966080
451701686294996,checking how to convert aarch64-unknown-linux-gnu file names to aarch64-unknown-linux-gnu format... func_convert_file_noop
461701686294996,checking how to convert aarch64-unknown-linux-gnu file names to toolchain format... func_convert_file_noop
471701686294996,checking for /usr/bin/ld option to reload object files... -r
481701686294996,checking for objdump... objdump
491701686294997,checking how to recognize dependent libraries... pass_all
501701686294997,checking for dlltool... no
511701686294997,"checking how to associate runtime and link libraries... printf %s
52"
531701686294997,checking for ar... ar
541701686295017,checking for archiver [@FILE](/bitcoin-bitcoin/contributor/file/) support... @
551701686295017,checking for strip... strip
561701686295017,checking for ranlib... ranlib
571701686295067,checking command to parse /usr/bin/nm -B output from gcc object... ok
581701686295068,checking for sysroot... no
591701686295071,checking for a working dd... /usr/bin/dd
601701686295073,checking how to truncate binary pipes... /usr/bin/dd bs=4096 count=1
611701686295074,checking for mt... no
621701686295076,checking if : is a manifest tool... no
631701686295093,checking for stdio.h... yes
641701686295112,checking for stdlib.h... yes
651701686295133,checking for string.h... yes
661701686295155,checking for inttypes.h... yes
671701686295177,checking for stdint.h... yes
681701686295198,checking for strings.h... yes
691701686295220,checking for sys/stat.h... yes
701701686295242,checking for sys/types.h... yes
711701686295269,checking for unistd.h... yes
721701686295297,checking for dlfcn.h... yes
731701686295300,checking for objdir... .libs
741701686295360,checking if gcc supports -fno-rtti -fno-exceptions... no
751701686295360,checking for gcc option to produce PIC... -fPIC -DPIC
761701686295375,checking if gcc PIC flag -fPIC -DPIC works... yes
771701686295435,checking if gcc static flag -static works... yes
781701686295455,checking if gcc supports -c -o file.o... yes
791701686295455,checking if gcc supports -c -o file.o... (cached) yes
801701686295462,checking whether the gcc linker (/usr/bin/ld) supports shared libraries... yes
811701686295481,checking whether -lc should be explicitly linked in... no
821701686295520,checking dynamic linker characteristics... GNU/Linux ld.so
831701686295520,checking how to hardcode library paths into programs... immediate
841701686295521,checking whether stripping libraries is possible... yes
851701686295521,checking if libtool supports shared libraries... yes
861701686295521,checking whether to build shared libraries... yes
871701686295521,checking whether to build static libraries... yes
881701686295546,checking how to run the C++ preprocessor... g++ -std=c++20 -E
891701686295620,checking for ld used by g++ -std=c++20... /usr/bin/ld
901701686295622,checking if the linker (/usr/bin/ld) is GNU ld... yes
911701686295627,checking whether the g++ -std=c++20 linker (/usr/bin/ld) supports shared libraries... yes
921701686295680,checking for g++ -std=c++20 option to produce PIC... -fPIC -DPIC
931701686295699,checking if g++ -std=c++20 PIC flag -fPIC -DPIC works... yes
941701686295759,checking if g++ -std=c++20 static flag -static works... yes
951701686295783,checking if g++ -std=c++20 supports -c -o file.o... yes
961701686295783,checking if g++ -std=c++20 supports -c -o file.o... (cached) yes
971701686295783,checking whether the g++ -std=c++20 linker (/usr/bin/ld) supports shared libraries... yes
981701686295786,checking dynamic linker characteristics... (cached) GNU/Linux ld.so
991701686295786,checking how to hardcode library paths into programs... immediate
1001701686295787,checking for ar... /usr/bin/ar
1011701686295787,checking for gcov... /usr/bin/gcov
1021701686295787,checking for llvm-cov... no
1031701686295787,checking for lcov... /usr/bin/lcov
1041701686295787,checking for python3.9... no
1051701686295787,checking for python3.10... /usr/bin/python3.10
1061701686295788,checking for genhtml... /usr/bin/genhtml
1071701686295788,checking for git... /usr/bin/git
1081701686295788,checking for ccache... /usr/bin/ccache
1091701686295788,checking for xgettext... no
1101701686295788,checking for hexdump... /usr/bin/hexdump
1111701686295788,checking for objcopy... /usr/bin/objcopy
1121701686295789,checking for doxygen... no
1131701686295805,checking whether C++ compiler accepts -Werror... yes
1141701686295844,"checking whether the linker accepts -Wl,--fatal-warnings... yes"
1151701686295864,checking whether C++ compiler accepts -Wall... yes
1161701686295881,checking whether C++ compiler accepts -Wextra... yes
1171701686295892,checking whether C++ compiler accepts -Wgnu... no
1181701686295910,checking whether C++ compiler accepts -Wformat -Wformat-security... yes
1191701686295929,checking whether C++ compiler accepts -Wvla... yes
1201701686295942,checking whether C++ compiler accepts -Wshadow-field... no
1211701686295958,checking whether C++ compiler accepts -Wthread-safety... no
1221701686295973,checking whether C++ compiler accepts -Wloop-analysis... no
1231701686295991,checking whether C++ compiler accepts -Wredundant-decls... yes
1241701686296011,checking whether C++ compiler accepts -Wunused-member-function... no
1251701686296029,checking whether C++ compiler accepts -Wdate-time... yes
1261701686296050,checking whether C++ compiler accepts -Wconditional-uninitialized... no
1271701686296068,checking whether C++ compiler accepts -Wduplicated-branches... yes
1281701686296085,checking whether C++ compiler accepts -Wduplicated-cond... yes
1291701686296103,checking whether C++ compiler accepts -Wlogical-op... yes
1301701686296121,checking whether C++ compiler accepts -Woverloaded-virtual... yes
1311701686296138,checking whether C++ compiler accepts -Wsuggest-override... yes
1321701686296161,checking whether C++ compiler accepts -Wunreachable-code-loop-increment... no
1331701686296179,checking whether C++ compiler accepts -Wimplicit-fallthrough... yes
1341701686296195,checking whether C++ compiler accepts -Wdocumentation... no
1351701686296213,checking whether C++ compiler accepts -Wunused-parameter... yes
1361701686296226,checking whether C++ compiler accepts -Wself-assign... no
1371701686296245,checking whether C++ compiler accepts -fno-extended-identifiers... yes
1381701686296260,checking whether C++ compiler accepts -fstack-reuse=none... yes
1391701686296272,checking whether C++ compiler accepts -msse4.2... no
1401701686296283,checking whether C++ compiler accepts -msse4.1... no
1411701686296294,checking whether C++ compiler accepts -mavx -mavx2... no
1421701686296305,checking whether C++ compiler accepts -msse4 -msha... no
1431701686296317,checking whether C++ compiler accepts -mpclmul... no
1441701686296340,checking for SSE4.2 intrinsics... no
1451701686296356,checking for SSE4.1 intrinsics... no
1461701686296370,checking for AVX2 intrinsics... no
1471701686296385,checking for x86 SHA-NI intrinsics... no
1481701686296405,checking whether C++ compiler accepts -march=armv8-a+crc+crypto... yes
1491701686296423,checking whether C++ compiler accepts -march=armv8-a+crypto... yes
1501701686296645,checking for ARMv8 CRC32 intrinsics... yes
1511701686296869,checking for ARMv8 SHA-NI intrinsics... yes
1521701686296951,checking whether byte ordering is bigendian... no
1531701686296971,checking how to run the C preprocessor... gcc -E
1541701686297002,checking whether gcc is Clang... no
1551701686297044,"checking whether pthreads work with ""-pthread"" and ""-lpthread""... yes"
1561701686297076,checking for joinable pthread attribute... PTHREAD_CREATE_JOINABLE
1571701686297076,checking whether more special flags are required for pthreads... no
1581701686297108,checking for PTHREAD_PRIO_INHERIT... yes
1591701686297581,checking whether std::atomic can be used without link library... yes
1601701686297582,checking for special C compiler options needed for large files... no
1611701686297600,checking for _FILE_OFFSET_BITS value needed for large files... no
1621701686297633,checking for g++ -std=c++20 options needed to detect all undeclared functions... none needed
1631701686297679,checking whether strerror_r is declared... yes
1641701686297701,checking whether strerror_r returns char *... yes
1651701686297717,checking whether C++ compiler accepts -fPIC... yes
1661701686297734,checking whether C++ compiler accepts -Wstack-protector... yes
1671701686297750,checking whether C++ compiler accepts -fstack-protector-all... yes
1681701686297758,checking whether C++ compiler accepts -fcf-protection=full... no
1691701686297776,checking whether C++ compiler accepts -fstack-clash-protection... yes
1701701686297792,checking whether C++ compiler accepts -mbranch-protection=bti... yes
1711701686297801,checking whether C++ preprocessor accepts -D_FORTIFY_SOURCE=3... yes
1721701686297811,checking whether C++ preprocessor accepts -U_FORTIFY_SOURCE... yes
1731701686297835,"checking whether the linker accepts -Wl,--enable-reloc-section... no"
1741701686297859,"checking whether the linker accepts -Wl,--dynamicbase... no"
1751701686297882,"checking whether the linker accepts -Wl,--nxcompat... no"
1761701686297907,"checking whether the linker accepts -Wl,--high-entropy-va... no"
1771701686297947,"checking whether the linker accepts -Wl,-z,relro... yes"
1781701686297986,"checking whether the linker accepts -Wl,-z,now... yes"
1791701686298025,"checking whether the linker accepts -Wl,-z,separate-code... yes"
1801701686298064,checking whether the linker accepts -fPIE -pie... yes
1811701686298103,checking for sys/select.h... yes
1821701686298143,checking for sys/prctl.h... yes
1831701686298169,checking for sys/sysctl.h... no
1841701686298194,checking for vm/vm_param.h... no
1851701686298219,checking for sys/vmmeter.h... no
1861701686298244,checking for sys/resources.h... no
1871701686298271,checking whether getifaddrs is declared... yes
1881701686298317,checking whether ifaddrs funcs can be used without link library... yes
1891701686298345,checking whether freeifaddrs is declared... yes
1901701686298389,checking whether ifaddrs funcs can be used without link library... yes
1911701686298438,checking whether fork is declared... yes
1921701686298484,checking whether setsid is declared... yes
1931701686298530,checking whether pipe2 is declared... yes
1941701686298572,checking for timingsafe_bcmp... no
1951701686298587,checking for __builtin_clzl... yes
1961701686298601,checking for __builtin_clzll... yes
1971701686298625,checking for getmemoryinfo... yes
1981701686298648,checking for mallopt M_ARENA_MAX... yes
1991701686298668,checking for posix_fallocate... yes
2001701686298684,checking for default visibility attribute... yes
2011701686298699,checking for dllexport attribute... no
2021701686299371,checking for thread_local support... yes
2031701686299391,checking for gmtime_r... yes
2041701686299414,checking for Linux getrandom function... yes
2051701686299437,checking for getentropy via sys/random.h... yes
2061701686299454,checking for sysctl... no
2071701686299469,checking for sysctl KERN_ARND... no
2081701686299498,checking for if type char equals int8_t... no
2091701686299523,checking for fdatasync... yes
2101701686299541,checking for F_FULLFSYNC... no
2111701686299562,checking for O_CLOEXEC... yes
2121701686299579,checking for __builtin_prefetch... yes
2131701686299592,checking for _mm_prefetch... no
2141701686299612,checking for strong getauxval support in the system headers... yes
2151701686299659,checking for std::system... yes
2161701686299684,checking for ::_wsystem... no
2171701686299691,checking for Qt5Core >= 5.11.3... no
2181701686299693,configure: WARNING: Qt5Core >= 5.11.3 not found; bitcoin-qt frontend will not be built
2191701686299694,checking whether to build Bitcoin Core GUI... no
2201701686299710,checking for sqlite3 >= 3.7.17... yes
2211701686299710,checking whether to build wallet with support for sqlite... yes
2221701686299723,"checking whether Userspace, Statically Defined Tracing tracepoints are supported... no"
2231701686299768,checking for miniupnpc/miniupnpc.h... yes
2241701686299814,checking for miniupnpc/upnpcommands.h... yes
2251701686299861,checking for miniupnpc/upnperrors.h... yes
2261701686299905,checking for upnpDiscover in -lminiupnpc... yes
2271701686299914,checking whether miniUPnPc API version is supported... yes
2281701686299941,checking for natpmp.h... no
2291701686299966,checking for Boost headers >= 1.64.0 (106400)... yes
2301701686299986,checking whether C++ preprocessor accepts -DBOOST_NO_CXX98_FUNCTION_BASE... yes
2311701686306369,checking whether Boost.Process can be used... yes
2321701686306374,checking for libevent >= 2.1.8... yes
2331701686306379,checking for libevent_pthreads >= 2.1.8... yes
2341701686306408,checking if evhttp_connection_get_peer expects const char**... no
2351701686306413,checking for libmultiprocess... no
2361701686306415,checking whether to build bitcoind... yes
2371701686306415,checking whether to build bitcoin-cli... yes
2381701686306415,checking whether to build bitcoin-tx... yes
2391701686306415,checking whether to build bitcoin-wallet... yes
2401701686306415,checking whether to build bitcoin-util... yes
2411701686306415,checking whether to build experimental bitcoin-chainstate... no
2421701686306416,checking whether to build libraries... yes
2431701686306416,checking if ccache should be used... yes
2441701686306549,checking whether C compiler accepts -fdebug-prefix-map=A=B... yes
2451701686306560,checking whether C preprocessor accepts -fmacro-prefix-map=A=B... yes
2461701686306560,checking if wallet should be enabled... yes
2471701686306560,checking whether to build with support for UPnP... yes
2481701686306560,checking whether to build with support for NAT-PMP... no
2491701686306560,checking whether to build test_bitcoin... no
2501701686306560,checking whether to reduce exports... no
2511701686306587,checking that generated files are newer than configure... done
2521701686306588,configure: creating ./config.status
2531701686307056,config.status: creating libbitcoinconsensus.pc
2541701686307064,config.status: creating Makefile
2551701686307072,config.status: creating src/Makefile
2561701686307108,config.status: creating doc/man/Makefile
2571701686307119,config.status: creating share/setup.nsi
2581701686307130,config.status: creating share/qt/Info.plist
2591701686307141,config.status: creating test/config.ini
2601701686307151,config.status: creating contrib/devtools/split-debug.sh
2611701686307163,config.status: creating src/config/bitcoin-config.h
2621701686307168,config.status: src/config/bitcoin-config.h is unchanged
2631701686307209,config.status: executing depfiles commands
2641701686307261,config.status: executing libtool commands
2651701686307270,'"=== configuring in src/secp256k1 (/tmp/bitcoin/src/secp256k1)"
2661701686307274,configure: running /bin/bash ./configure --disable-option-checking '--prefix=/usr/local' '--enable-bench' '--disable-tests' '--disable-gui' '--disable-zmq' '--disable-fuzz' '--enable-fuzz-binary=no' 'BDB_LIBS=-L/tmp/bitcoin/depends/aarch64-unknown-linux-gnu/lib -ldb_cxx-4.8' 'BDB_CFLAGS=-I/tmp/bitcoin/depends/aarch64-unknown-linux-gnu/include' '--disable-shared' '--with-pic' '--enable-benchmark=no' '--enable-module-recovery' '--disable-module-ecdh' --cache-file=/dev/null --srcdir=.
2671701686307402,checking build system type... aarch64-unknown-linux-gnu
2681701686307402,checking host system type... aarch64-unknown-linux-gnu
2691701686307408,checking for a BSD-compatible install... /usr/bin/install -c
2701701686307410,checking whether build environment is sane... yes
2711701686307416,checking for a race-free mkdir -p... /usr/bin/mkdir -p
2721701686307416,checking for gawk... no
2731701686307416,checking for mawk... mawk
2741701686307421,checking whether make sets $(MAKE)... yes
2751701686307425,checking whether make supports nested variables... yes
2761701686307428,checking whether make supports nested variables... (cached) yes
2771701686307428,checking for gcc... gcc
2781701686307472,checking whether the C compiler works... yes
2791701686307472,checking for C compiler default output file name... a.out
2801701686307497,checking for suffix of executables...
2811701686307525,checking whether we are cross compiling... no
2821701686307540,checking for suffix of object files... o
2831701686307553,checking whether the compiler supports GNU C... yes
2841701686307566,checking whether gcc accepts -g... yes
2851701686307609,checking for gcc option to enable C11 features... none needed
2861701686307633,checking whether gcc understands -c and -o together... yes
2871701686307638,checking whether make supports the include directive... yes (GNU style)
2881701686307661,checking dependency style of gcc... gcc3
2891701686307683,checking dependency style of gcc... gcc3
2901701686307683,checking for ar... ar
2911701686307697,checking the archiver (ar) interface... ar
2921701686307699,checking how to print strings... printf
2931701686307702,checking for a sed that does not truncate output... /usr/bin/sed
2941701686307704,checking for grep that handles long lines and -e... /usr/bin/grep
2951701686307705,checking for egrep... /usr/bin/grep -E
2961701686307706,checking for fgrep... /usr/bin/grep -F
2971701686307709,checking for ld used by gcc... /usr/bin/ld
2981701686307711,checking if the linker (/usr/bin/ld) is GNU ld... yes
2991701686307712,checking for BSD- or MS-compatible name lister (nm)... /usr/bin/nm -B
3001701686307728,checking the name lister (/usr/bin/nm -B) interface... BSD nm
3011701686307728,checking whether ln -s works... yes
3021701686307731,checking the maximum length of command line arguments... 1966080
3031701686307733,checking how to convert aarch64-unknown-linux-gnu file names to aarch64-unknown-linux-gnu format... func_convert_file_noop
3041701686307733,checking how to convert aarch64-unknown-linux-gnu file names to toolchain format... func_convert_file_noop
3051701686307733,checking for /usr/bin/ld option to reload object files... -r
3061701686307734,checking for objdump... objdump
3071701686307734,checking how to recognize dependent libraries... pass_all
3081701686307734,checking for dlltool... no
3091701686307734,"checking how to associate runtime and link libraries... printf %s
310"
3111701686307754,checking for archiver [@FILE](/bitcoin-bitcoin/contributor/file/) support... @
3121701686307754,checking for strip... strip
3131701686307754,checking for ranlib... ranlib
3141701686307803,checking command to parse /usr/bin/nm -B output from gcc object... ok
3151701686307804,checking for sysroot... no
3161701686307807,checking for a working dd... /usr/bin/dd
3171701686307810,checking how to truncate binary pipes... /usr/bin/dd bs=4096 count=1
3181701686307810,checking for mt... no
3191701686307812,checking if : is a manifest tool... no
3201701686307829,checking for stdio.h... yes
3211701686307850,checking for stdlib.h... yes
3221701686307872,checking for string.h... yes
3231701686307894,checking for inttypes.h... yes
3241701686307917,checking for stdint.h... yes
3251701686307940,checking for strings.h... yes
3261701686307962,checking for sys/stat.h... yes
3271701686307984,checking for sys/types.h... yes
3281701686308010,checking for unistd.h... yes
3291701686308037,checking for dlfcn.h... yes
3301701686308040,checking for objdir... .libs
3311701686308098,checking if gcc supports -fno-rtti -fno-exceptions... no
3321701686308098,checking for gcc option to produce PIC... -fPIC -DPIC
3331701686308113,checking if gcc PIC flag -fPIC -DPIC works... yes
3341701686308172,checking if gcc static flag -static works... yes
3351701686308191,checking if gcc supports -c -o file.o... yes
3361701686308191,checking if gcc supports -c -o file.o... (cached) yes
3371701686308198,checking whether the gcc linker (/usr/bin/ld) supports shared libraries... yes
3381701686308236,checking dynamic linker characteristics... GNU/Linux ld.so
3391701686308236,checking how to hardcode library paths into programs... immediate
3401701686308237,checking whether stripping libraries is possible... yes
3411701686308237,checking if libtool supports shared libraries... yes
3421701686308237,checking whether to build shared libraries... no
3431701686308237,checking whether to build static libraries... yes
3441701686308250,checking if gcc supports -Werror... yes
3451701686308262,checking if gcc supports -std=c89 -pedantic -Wno-long-long -Wnested-externs -Wshadow -Wstrict-prototypes -Wundef... yes
3461701686308274,checking if gcc supports -Wno-overlength-strings... yes
3471701686308287,checking if gcc supports -Wall... yes
3481701686308299,checking if gcc supports -Wno-unused-function... yes
3491701686308312,checking if gcc supports -Wextra... yes
3501701686308324,checking if gcc supports -Wcast-align... yes
3511701686308337,checking if gcc supports -Wcast-align=strict... yes
3521701686308356,checking if gcc supports -Wconditional-uninitialized... no
3531701686308372,checking if gcc supports -Wreserved-identifier... no
3541701686308383,checking if gcc supports -fvisibility=hidden... yes
3551701686308397,checking for valgrind support...
3561701686308415,checking for x86_64 assembly availability... no
3571701686308434,checking that generated files are newer than configure... done
3581701686308434,configure: creating ./config.status
3591701686308749,config.status: creating Makefile
3601701686308758,config.status: creating libsecp256k1.pc
3611701686308766,config.status: executing depfiles commands
3621701686308777,config.status: executing libtool commands
3631701686308784,Build Options:
3641701686308784, with external callbacks = no
3651701686308784, with benchmarks = no
3661701686308784, with tests = no
3671701686308784, with ctime tests = no
3681701686308784, with coverage = no
3691701686308784, with examples = no
3701701686308784, module ecdh = no
3711701686308784, module recovery = yes
3721701686308784, module extrakeys = yes
3731701686308784, module schnorrsig = yes
3741701686308784, module ellswift = yes
3751701686308784, asm = no
3761701686308784, ecmult window size = 15
3771701686308784, ecmult gen prec. bits = 4
3781701686308784, valgrind = no
3791701686308784, CC = gcc
3801701686308784, CPPFLAGS =
3811701686308784, SECP_CFLAGS = -O2 -std=c89 -pedantic -Wno-long-long -Wnested-externs -Wshadow -Wstrict-prototypes -Wundef -Wno-overlength-strings -Wall -Wno-unused-function -Wextra -Wcast-align -Wcast-align=strict -fvisibility=hidden
3821701686308784, CFLAGS = -g -O2
3831701686308784, LDFLAGS =
3841701686308812,Options used to compile and link:
3851701686308812, external signer = yes
3861701686308812, multiprocess = no
3871701686308812, with libs = yes
3881701686308812, with wallet = yes
3891701686308812, with sqlite = yes
3901701686308812, with bdb = yes
3911701686308812, with gui / qt = no
3921701686308812, with zmq = no
3931701686308812, with test = no
3941701686308812, with fuzz binary = no
3951701686308812, with bench = yes
3961701686308812, with upnp = yes
3971701686308812, with natpmp = no
3981701686308812, use asm = yes
3991701686308812, USDT tracing = no
4001701686308812, sanitizers =
4011701686308812, debug enabled = no
4021701686308812, gprof enabled = no
4031701686308812, werror = no
4041701686308812, LTO = no
4051701686308812, target os = linux-gnu
4061701686308812, build os = linux-gnu
4071701686308812, CC = /usr/bin/ccache gcc
4081701686308812, CFLAGS = -pthread -g -O2
4091701686308812, CPPFLAGS = -fmacro-prefix-map=$(abs_top_srcdir)=. -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -DHAVE_BUILD_INFO
4101701686308812, CXX = /usr/bin/ccache g++ -std=c++20
4111701686308812, CXXFLAGS = -fdebug-prefix-map=$(abs_top_srcdir)=. -Wstack-protector -fstack-protector-all -fstack-clash-protection -mbranch-protection=bti -Wall -Wextra -Wformat -Wformat-security -Wvla -Wredundant-decls -Wdate-time -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wno-unused-parameter -fno-extended-identifiers -fstack-reuse=none -g -O2
4121701686308812," LDFLAGS = -lpthread -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -pie "
4131701686308812, AR = /usr/bin/ar
4141701686308812, ARFLAGS = cr
Same bench result here, compiling with clang, both master and this pull. My command:
0$ grep 'make_bitcoin_core=' ~/.bashrc
1alias make_bitcoin_core=" ./autogen.sh && CC=clang CXX=clang++ ./configure -q --enable-c++20 --enable-experimental-util-chainstate --with-experimental-kernel-lib && make clean && make -j $(nproc)"
Yes that is correct. Also the compiler installed is g++-11.
It would be useful to have compiler/version used/C{XX} flags/configure options (even if just mentioning it’s the default) displayed next to the benchmarking information, as I assume this will always be one of the first questions asked in relation to benchmark output.
Rebased on master/the current C++20 PR, which should improve the CI here.
@aureleoules does the SonarCloud output always run on the latest changes? I’ve dropped all the inline usage from the constexpr functions in endian.h, but https://corecheck.dev/bitcoin/bitcoin/pulls/28674 still shows all the output about using inline with constexpr?
Maybe something was being cached, as this seems to have updated now.
Maybe something was being cached, as this seems to have updated now.
Yes it may take some time for the sonarcloud worker to finish, as well as sonarcloud to refresh the results. I’ll improve the UX for that 😅
Hmm, playing around with this code on godbolt, it hardly ever compiles down to bswaps. I’m not sure what changed from when I was initially investigating. Will have a look.
Ok, interesting, the problem is pretty easy to see here: https://godbolt.org/z/d5EGs8Ybs
GCC and MSVC both do the same thing: the optimization is done on the small static function, but it doesn’t carry through when inlined. clang’s output looks as expected. That explains why my test cases look good, but real world performance took a nosedive.
Also interesting, switching the function to a macro doesn’t change anything. @aureleoules How hard would it be to switch to using clang for a test-run to compare?
I wonder if this is something GCC would be interested in taking a look at. clang can obviously do the right thing.
clang can obviously do the right thing.
Are you sure? When I tried yesterday, it did not (https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1841445629)
clang can obviously do the right thing.
Are you sure?
Absolutely not! I’m making this up as I go. Edit: See the godbolt link for comparison though.
When I tried yesterday, it did not (#28674 (comment))
Thanks, I guess that’s the test I just asked for.
Ok, I’ll keep poking. But it seems like this approach likely won’t work :(
Ok, I’ll keep poking. But it seems like this approach likely won’t work :(
What about hiding the function inside a compilation unit, where they are turned into bswap, and then rely on LTO to replace the call internal_bswap_64 with bswap? (Haven’t tried this)
- use of std::endian means that we no longer have to rely on configure checks
and compiler macros for compile-time endianness checks.
- countl_zero replaces the need for __builtin_clz*
- trust the compiler to optimize bswaps and drop platform-dependent
implementations
118- while (x) {
119- x >>= 1;
120- ++ret;
121- }
122- return ret;
123+ return (sizeof(x) * 8 )- std::countl_zero(x);
Could split this up from the other changes?
nit: Also missing clang-format on this line.
Yes, there are essentially 3 changes here:
std::endianstd::countl_zeroThe first two are useful regardless, though obviously much less so without doing the third.
I’ll update this to break up the commits.
As an alternative to what’s here, I’m going to try switching our byteswaps to compiler builtins for gcc/clang/msvc. That still leaves us with compiler-specific behavior, but that would at least be an improvement over the current required autoconf/cmake tests.
CountBits in two places, if this is going to basically become a std lib call, we could just use it directly where needed, and remove our CountBits unit/fuzz tests, so that we aren’t nearly unit testing / fuzzing the standard library.
std::counl_zero directly, we want kinda the opposite: (sizeof(x) * 8 )- std::countl_zero(x);
I worked on a branch last week that left me very confused: https://github.com/theuni/bitcoin/commits/pr28674-rebase2/
It’s the last commit (which I would expect to be a no-op) which causes the slowdown. For whatever reason, the built-in functions like be64toh are much faster than my hand-written internal ones which implement the same code that glibc does in its headers! I’m still scratching my head trying to work out what’s happening.
But for now, now that we have c++20, I’ll go ahead and PR the first two parts.
I think I’d prefer to leave this with its comment. Note that we don’t use
std::counl_zerodirectly, we want kinda the opposite:(sizeof(x) * 8 )- std::countl_zero(x);
Actually, as it turns out, std::bit_width is exactly what we want (and it’s even described in terms of countl_zero). I’ll see if it works to use your suggestion with that function instead.
Ok, I’ll keep poking. But it seems like this approach likely won’t work :(
What about hiding the function inside a compilation unit, where they are turned into
bswap, and then rely on LTO to replace thecall internal_bswap_64withbswap? (Haven’t tried this)
Hah, that’s fun. Too fun in fact, so I’m not going to try because it would make things even more complicated if it worked :p Because even if it worked, we couldn’t possibly require LTO as a means of achieving basic performance.
Hmm. I have something working, but as usual, MSVC is the odd case. Looking at the current code, it’s not clear to me how MSVC ends up doing byteswaps because we don’t have any detection for it. But I also doubt that it’s finding the functions in <byteswap.h>.
So I think it’s possible that MSVC is always taking the slow path? If that’s the case, fixing this here would give it a big speedup. @hebasto could you check with MSVC? It should be easy to check with master with something like:
0diff --git a/src/compat/byteswap.h b/src/compat/byteswap.h
1index 9ee71ef267d..49e17c68828 100644
2--- a/src/compat/byteswap.h
3+++ b/src/compat/byteswap.h
4@@ -43,6 +43,7 @@ inline uint32_t bswap_32(uint32_t x)
5 #if HAVE_DECL_BSWAP_64 == 0
6 inline uint64_t bswap_64(uint64_t x)
7 {
8+#error using slow path
9 return (((x & 0xff00000000000000ull) >> 56)
10 | ((x & 0x00ff000000000000ull) >> 40)
11 | ((x & 0x0000ff0000000000ull) >> 24)
Thanks! https://github.com/theuni/bitcoin/commit/bbc72033128b6496dd3a92432eadbc4e75aaffe2 ready for PR if it indeed provides a speedup. I don’t see how it couldn’t :)
107-#if HAVE_BUILTIN_CLZL
108- if (sizeof(unsigned long) >= sizeof(uint64_t)) {
109- return x ? 8 * sizeof(unsigned long) - __builtin_clzl(x) : 0;
110- }
111-#endif
112-#if HAVE_BUILTIN_CLZLL