Skip to content

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Jun 4, 2025

There are several reasons to prefer clang-cl over MSVC, such as improved security and performance.

Below are the benchmark results for the master branch @ 201b2b8:

Benchmark                     ,    Min(us)    ,    Avg(us)    ,    Max(us)    

ecdsa_verify                  ,    66.0       ,    71.0       ,   113.0    
ecdsa_sign                    ,    37.0       ,    37.1       ,    37.5    
ec_keygen                     ,    28.5       ,    28.9       ,    29.0    
ecdh                          ,    66.0       ,    66.2       ,    67.0    
ecdsa_recover                 ,    67.0       ,    74.9       ,   123.0    
schnorrsig_sign               ,    30.0       ,    30.3       ,    30.5    
schnorrsig_verify             ,    66.5       ,    70.6       ,   104.0    
ellswift_encode               ,    17.5       ,    17.9       ,    18.0    
ellswift_decode               ,    14.5       ,    15.3       ,    19.0    
ellswift_keygen               ,    55.0       ,    56.4       ,    63.5    
ellswift_ecdh                 ,    72.5       ,    73.5       ,    79.5  
Benchmark                     ,    Min(us)    ,    Avg(us)    ,    Max(us)    

ecdsa_verify                  ,    41.0       ,    47.5       ,   100.0    
ecdsa_sign                    ,    27.0       ,    27.2       ,    27.5    
ec_keygen                     ,    19.0       ,    19.3       ,    19.5    
ecdh                          ,    42.0       ,    42.4       ,    43.0    
ecdsa_recover                 ,    41.5       ,    45.7       ,    80.0    
schnorrsig_sign               ,    20.0       ,    20.5       ,    20.5    
schnorrsig_verify             ,    41.5       ,    45.5       ,    77.5    
ellswift_encode               ,    13.0       ,    13.0       ,    13.0    
ellswift_decode               ,    10.0       ,    10.4       ,    10.5    
ellswift_keygen               ,    38.5       ,    39.1       ,    41.5    
ellswift_ecdh                 ,    47.0       ,    48.5       ,    59.0    

On my local machine, the "Release" build configuration:

  • using MSVC:
> .\build-msvc\bin\Release\bench.exe
Benchmark                     ,    Min(us)    ,    Avg(us)    ,    Max(us)

ecdsa_verify                  ,    81.2       ,    90.6       ,   102.0
ecdsa_sign                    ,    46.5       ,    48.6       ,    52.9
ec_keygen                     ,    31.6       ,    34.8       ,    36.2
ecdh                          ,    73.0       ,    76.4       ,    79.5
schnorrsig_sign               ,    32.1       ,    34.4       ,    35.8
schnorrsig_verify             ,    74.6       ,    76.2       ,    79.8
ellswift_encode               ,    33.4       ,    34.0       ,    34.8
ellswift_decode               ,    14.9       ,    15.5       ,    17.1
ellswift_keygen               ,    64.5       ,    65.6       ,    67.1
ellswift_ecdh                 ,    78.3       ,    80.7       ,    90.1
  • using clang-cl:
> .\build-clangcl\bin\Release\bench.exe
Benchmark                     ,    Min(us)    ,    Avg(us)    ,    Max(us)

ecdsa_verify                  ,    40.3       ,    40.6       ,    40.9
ecdsa_sign                    ,    30.6       ,    30.9       ,    31.3
ec_keygen                     ,    21.2       ,    21.3       ,    21.5
ecdh                          ,    41.5       ,    42.4       ,    44.8
schnorrsig_sign               ,    22.5       ,    22.7       ,    22.8
schnorrsig_verify             ,    41.2       ,    41.4       ,    41.7
ellswift_encode               ,    20.3       ,    20.6       ,    20.8
ellswift_decode               ,     8.50      ,     8.64      ,     8.76
ellswift_keygen               ,    41.7       ,    42.0       ,    42.4
ellswift_ecdh                 ,    45.1       ,    45.5       ,    46.3

@real-or-random real-or-random added user-documentation user-facing documentation build labels Jun 5, 2025
@fanquake
Copy link
Member

fanquake commented Jun 5, 2025

Are benchmarks all that should be considered? From what I can see there's there's only a single clang-cl CI job in this repo; if we're going to change the recommended way to build the library, then we should also replace the majority of the MSVC jobs, with clang-cl, to reflect that preference?

It would also be interesting to elaborate on why the performance of MSVC is so much worse than Clang.

@hebasto
Copy link
Member Author

hebasto commented Jun 5, 2025

It would also be interesting to elaborate on why the performance of MSVC is so much worse than Clang.

Several factors may contribute to the performance gap:

  1. Different optimization strategies.
  2. Differences in inlining heuristics.
  3. A number of other compiler‑level details.

However, I haven't compared the generated assembly code.

@hebasto
Copy link
Member Author

hebasto commented Jun 5, 2025

It would also be interesting to elaborate on why the performance of MSVC is so much worse than Clang.

Another factor is that clang-cl uses native 128-bit integer types, whereas MSVC relies on int128_struct.

@hebasto
Copy link
Member Author

hebasto commented Jun 5, 2025

Although clang-cl supports inline assembly, as indicated by the configure summary:

secp256k1 configure summary
===========================
<snip>
Optional features:
  assembly ............................ x86_64

I observed no benchmark difference between -DSECP256K1_ASM=x86_64 and -DSECP256K1_ASM=OFF.

@hebasto
Copy link
Member Author

hebasto commented Jun 5, 2025

From what I can see there's there's only a single clang-cl CI job in this repo; if we're going to change the recommended way to build the library, then we should also replace the majority of the MSVC jobs, with clang-cl, to reflect that preference?

Fair enough. More clang-cl CI tasks have been added.

Comment on lines 607 to 608
- job_name: 'x64 (MSVC): Windows (clang-cl, shared)'
cmake_options: '-T ClangCL -DBUILD_SHARED_LIBS=ON'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming: Shouldn't this be: x64 (clang-cl): Windows (VS 2022, shared) then? It's not a MSVC build.

(We could rework the naming of the jobs in general, it's not very consistent, and it's also not very helpful because the long names are truncated too early in the sidebar on https://github.com/bitcoin-core/secp256k1/pull/1681/checks, but that's a separate issue.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Renamed.

@real-or-random
Copy link
Contributor

real-or-random commented Jun 5, 2025

if we're going to change the recommended way to build the library,

I doubt that MSVC was ever supposed to be a recommendation. (I wouldn't recommend MSVC to anyone...) It's just an example, and it's the default compiler in VS as far as I understand.

then we should also replace the majority of the MSVC jobs, with clang-cl, to reflect that preference?

But sure, I can't hurt to test more in clang-cl. Though we have a good clang coverage already, just not on Windows. But since we hardly use the C stdlib or syscalls, that's still a good coverage.

@hebasto hebasto force-pushed the 250604-clang-cl branch 2 times, most recently from a5d4cd3 to 9243e21 Compare June 5, 2025 16:15
README.md Outdated
Comment on lines 140 to 141
Using clang-cl is recommended, as it tends to produce better-performing binaries compared to MSVC.
Copy link
Contributor

@real-or-random real-or-random Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Using clang-cl is recommended, as it tends to produce better-performing binaries compared to MSVC.
Using clang-cl is recommended.

There are more reasons to prefer clang, e.g., the (forgotten) #1164 or just the fact its output more testing (though mostly on Linux). But it's difficult to explain in one or two sentences, and I think it's ok not to explain it here.

edit: I also suggest dropping the empty line after this sentence, but somehow the GitHub suggestion feature doesn't understand this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Updated, including dropping the empty line.

@real-or-random
Copy link
Contributor

Concept ACK

@hebasto hebasto force-pushed the 250604-clang-cl branch 2 times, most recently from 0a393dc to db0c5f9 Compare June 5, 2025 20:03
@real-or-random
Copy link
Contributor

@sipa What do you think?

Copy link
Contributor

@sipa sipa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

README.md Outdated
To build on Windows with Visual Studio, a proper [generator](https://cmake.org/cmake/help/latest/manual/cmake-generators.7.html#visual-studio-generators) must be specified for a new build tree.
Using clang-cl is recommended.
The following example assumes using of Visual Studio 2022 and CMake v3.21+.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks outdated now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Adjusted.

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK mod nit

README.md Outdated
The following example assumes using of Visual Studio 2022 and CMake v3.21+.
Using clang-cl is recommended.
The following example assumes using of Visual Studio 2022.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

I think this is more grammatical:

Suggested change
The following example assumes using of Visual Studio 2022.
The following example assumes using Visual Studio 2022.

But I'd just simplify to:

Suggested change
The following example assumes using of Visual Studio 2022.
The following example assumes Visual Studio 2022.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Picked the correct wording you suggested.

Additionally, I dropped the mention of 'a proper generator must be specified', as the default generator now works just fine.

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 7379124

@real-or-random real-or-random merged commit 03fb60a into bitcoin-core:master Sep 2, 2025
119 checks passed
@hebasto hebasto deleted the 250604-clang-cl branch September 3, 2025 09:51
vmta added a commit to umkoin/umkoin that referenced this pull request Sep 21, 2025
36e76952c Merge bitcoin-core/secp256k1#1738: check-abi: remove support for obsolete CMake library output location (src/libsecp256k1.so)
4985ac0f8 Merge bitcoin-core/secp256k1#1737: doc: mention ctx requirement for `_ellswift_create` (not secp256k1_context_static)
7ebaa134a check-abi: remove support for obsolete CMake library output location (src/libsecp256k1.so)
806de38bf doc: mention ctx requirement for `_ellswift_create` (not secp256k1_context_static)
03fb60ad2 Merge bitcoin-core/secp256k1#1681: doc: Recommend clang-cl when building on Windows
d93380fb3 Merge bitcoin-core/secp256k1#1731: schnorrsig: Securely clear buf containing k or its negation
8113671f8 Merge bitcoin-core/secp256k1#1729: hash: Use size_t instead of int for RFC6979 outlen copy
325d65a8c Rename and clear var containing k or -k
960ba5f9c Use size_t instead of int for RFC6979 outlen copy
737912430 ci: Add more tests for clang-cl
7379a5bed doc: Recommend clang-cl when building on Windows
f36afb8b3 Merge bitcoin-core/secp256k1#1725: tests: refactor tagged hash verification
5153cf1c9 tests: refactor tagged hash tests
d2dcf5209 Merge bitcoin-core/secp256k1#1726: docs: fix broken link to Tromer's cache.pdf paper
489a43d1b docs: fix broken link to eprint cache.pdf paper
d59971414 Merge bitcoin-core/secp256k1#1722: docs: Exclude modules' `bench_impl.h` headers from coverage report
0458def51 doc: Add `--gcov-ignore-parse-errors=all` option to `gcovr` invocations
1aecce593 doc: Add `--merge-mode-functions=separate` option to `gcovr` invocations
106a7cbf4 doc: Exclude modules' `bench_impl.h` headers from coverage report
a9e955d3e autotools, docs: Adjust help string for `--enable-coverage` option
e523e4f90 Merge bitcoin-core/secp256k1#1720: chore(ci): Fix typo in Dockerfile comment
24ba8ff16 chore(ci): Fix typo in Dockerfile comment
74b8068c5 Merge bitcoin-core/secp256k1#1717: test: update wycheproof test vectors
c25c3c8a8 test: update wycheproof test vectors
20e3b4474 Merge bitcoin-core/secp256k1#1688: cmake: Avoid contaminating parent project's cache with `BUILD_SHARED_LIBS`
2c076d907 Merge bitcoin-core/secp256k1#1711: tests: update Wycheproof
7b07b2295 cmake: Avoid contaminating parent project's cache with BUILD_SHARED_LIBS
5433648ca Fix typos and spellings
9ea54c69b tests: update Wycheproof files
b9313c6e1 Merge bitcoin-core/secp256k1#1708: release cleanup: bump version after 0.7.0
a660a4976 Merge bitcoin-core/secp256k1#1707: release: Prepare for 0.7.0
7ab8b0cc0 release cleanup: bump version after 0.7.0
a3e742d94 release: Prepare for 0.7.0
f67b0ac1a ci: Don't hardcode ABI version
020ee6049 Merge bitcoin-core/secp256k1#1706: musig/tests: initialize keypair
cde413089 musig/tests: initialize keypair
6037833c9 Merge bitcoin-core/secp256k1#1702: changelog: update
40b4a0652 changelog: update
5e74086dc Merge bitcoin-core/secp256k1#1705: musig/test: Remove dead code
7c3380423 Merge bitcoin-core/secp256k1#1696: build: Refactor visibility logic and add override
8d967a602 musig/test: Remove dead code
983711cd6 musig/tests: Refactor vectors_signverify
73a695958 Merge bitcoin-core/secp256k1#1704: cmake: Make `secp256k1_objs` inherit interface defines from `secp256k1`
bf082221f cmake: Make `secp256k1_objs` inherit interface defines from `secp256k1`
c82d84bb8 build: add CMake option for disabling symbol visibility attributes
ce7923874 build: Add SECP256K1_NO_API_VISIBILITY_ATTRIBUTES
e5297f6d7 build: Refactor visibility logic
cbbbf3bd6 Merge bitcoin-core/secp256k1#1699: ci: enable musig module for native macOS arm64 job
943479a7a Merge bitcoin-core/secp256k1#1694: Revert "cmake: configure libsecp256k1.pc during install"
3352f9d66 ci: enable musig module for native macOS arm64 job
ad60ef7ea Merge bitcoin-core/secp256k1#1689: ci: Convert `arm64` Cirrus tasks to GHA jobs
c49877909 Merge bitcoin-core/secp256k1#1687: cmake: support the use of launchers in ctest -S scripts
44b205e9e Revert "cmake: configure libsecp256k1.pc during install"
0dfe387db cmake: support the use of launchers in ctest -S scripts
89096c234 Merge bitcoin-core/secp256k1#1692: cmake: configure libsecp256k1.pc during install
7106dce6f cmake: configure libsecp256k1.pc during install
29e73f4ba Merge bitcoin-core/secp256k1#1685: cmake: Emulate Libtool's behavior on FreeBSD
746e36b14 Merge bitcoin-core/secp256k1#1678: cmake: add a helper for linking into static libs
a28c2ffa5 Merge bitcoin-core/secp256k1#1683: README: add link to musig example
2a9d37473 Merge bitcoin-core/secp256k1#1690: ci: Bump GCC snapshot major version to 16
add146e10 ci: Bump GCC snapshot major version to 16
004f57fcd ci: Move Valgrind build for `arm64` from Cirrus to GHA
5fafdfc30 ci: Move `gcc-snapshot` build for `arm64` from Cirrus to GHA
e814b79a8 ci: Switch `arm64_debian` from QEMU to native `arm64` Docker image
bcf77346b ci: Add `arm64` architecture to `docker_cache` job
b77aae922 ci: Rename Docker image tag to reflect architecture
145ae3e28 cmake: add a helper for linking into static libs
819210974 README: add link to musig example, generalize module enabling hint
95db29b14 Merge bitcoin-core/secp256k1#1679: cmake: Use `PUBLIC_HEADER` target property in installation logic
37dd422b5 cmake: Emulate Libtool's behavior on FreeBSD
f24b838be Merge bitcoin-core/secp256k1#1680: doc: Promote "Building with CMake" to standard procedure
3f31ac43e doc: Promote "Building with CMake" to standard procedure
6f67151ee cmake: Use `PUBLIC_HEADER` target property
c32715b2a cmake, move-only: Move module option processing to `src/CMakeLists.txt`
201b2b8f0 Merge bitcoin-core/secp256k1#1675: cmake: Bump minimum required CMake version to 3.22
3af71987a cmake: Bump minimum required CMake version to 3.22
92394476e Merge bitcoin-core/secp256k1#1673: Assert field magnitude at control-flow join
3a4f448cb Assert field magnitude at control-flow join

git-subtree-dir: src/secp256k1
git-subtree-split: 36e76952cbf1cf54ddd2d8756cc31a486e2ba1d9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build user-documentation user-facing documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants