8000 doc: Recommend clang-cl when building on Windows by hebasto · Pull Request #1681 · bitcoin-core/secp256k1 · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

doc: Recommend clang-cl when building on Windows #1681

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

doc: Recommend clang-cl when building on Windows #1681

wants to merge 2 commits into from

Conversation

Copy link
Member
@hebasto hebasto commented Jun 4, 2025 •
edited
Loading

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 < 8000 /span> 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.

real-or-random
real-or-random reviewed Jun 5, 2025
View reviewed changes
.github/workflows/ci.yml Outdated
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 •
edited
Loading

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
real-or-random
real-or-random reviewed Jun 5, 2025
View reviewed changes
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 •
edited
Loading

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 from 9243e21 to 0a393dc Compare June 5, 2025 20:02
< 9BDF /div>
@hebasto hebasto force-pushed the 250604-clang-cl branch from 0a393dc to db0c5f9 Compare June 5, 2025 20:03
@real-or-random
Copy link
Contributor

@sipa What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@real-or-random real-or-random real-or-random left review comments

Assignees
No one assigned
Labels
build user-documentation user-facing documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hebasto @fanquake @real-or-random
0