Skip to content

build: Increase MS Visual Studio minimum version #25472

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
Merged
merged 5 commits into from
Jul 13, 2022
Merged

build: Increase MS Visual Studio minimum version #25472

merged 5 commits into from
Jul 13, 2022

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Jun 25, 2022 •
edited
Loading

Visual Studio 2022 with /std:c++20 supports designated initializers.

@hebasto hebasto mentioned this pull request Jun 25, 2022
Merged
@hebasto hebasto mentioned this pull request Jun 25, 2022
Merged
@hebasto hebasto force-pushed the 220625-desig branch 2 times, most recently from eae8a64 to ef556c7 Compare June 25, 2022 15:13
@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 25, 2022 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25578 (rpc: add missing description in gettxout help text by MarnixCroes)
  • #25572 (refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge)
  • #25514 (net processing: Move CNode::nServices and CNode::nLocalServices to Peer by dergoegge)
  • #25390 (sync: introduce a thread-safe smart container and use it to remove a bunch of "GlobalMutex"es by vasild)
  • #25268 (refactor: Introduce EvictionManager by dergoegge)
  • #23561 (BIP324: Handshake prerequisites by dhruv)
  • #21283 (Implement BIP 370 PSBTv2 by achow101)

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.

@DrahtBot DrahtBot mentioned this pull request Jun 25, 2022
Closed
@hebasto
Copy link
Member Author

hebasto commented Jun 25, 2022

Still observing a memory usage spike up to 25.38 GB.

@ajtowns
Copy link
Contributor

ajtowns commented Jun 27, 2022

Still observing a memory usage spike up to 25.38 GB.

Is that unique to cirrus, or something people can observe when compiling with vs2022 themselves?

@fanquake fanquake requested a review from sipsorcery June 28, 2022 17:21
sipsorcery
sipsorcery approved these changes Jun 28, 2022
View reviewed changes
Copy link
Contributor

@sipsorcery sipsorcery left a comment

Choose a reason for hiding this comment

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

Builds and passes tests for me.

@maflcko
Copy link
Member

maflcko commented Jun 28, 2022

Is that unique to cirrus, or something people can observe when compiling with vs2022 themselves?

I guess it would be nice to have a minimum working (breaking) example

@hebasto
Copy link
Member Author

hebasto commented Jun 29, 2022

Is that unique to cirrus, or something people can observe when compiling with vs2022 themselves?

I guess it would be nice to have a minimum working (breaking) example

FWIW, disabling of Qt code removes memory usage spikes.

@maflcko
Copy link
Member

maflcko commented Jun 29, 2022

So it has nothing to do with designated initializers, but with compiling qt code with the c++20 option?

@hebasto
Copy link
Member Author

hebasto commented Jun 29, 2022

So it has nothing to do with designated initializers, but with compiling qt code with the c++20 option?

Yes. See https://cirrus-ci.com/task/4621320997568512

@hebasto hebasto force-pushed the 220625-desig branch 2 times, most recently from 57761cf to ae7327a Compare June 29, 2022 10:38
@laanwj
Copy link
Member

laanwj commented Jun 29, 2022

Concept ACK

@maflcko
Copy link
Member

maflcko commented Jun 29, 2022

If you want to use this pull request for debugging faster, you can also delete all other tasks and assign the credits template to the windows build for immediate scheduling.

@hebasto
Copy link
Member Author

hebasto commented Jun 29, 2022

@MarcoFalke

you can also delete all other tasks

Of course, I did it in my private Cirrus account :)

and assign the credits template to the windows build for immediate scheduling.

Thank you!

@hebasto
Copy link
Member Author

hebasto commented Jun 29, 2022

"Curiouser and curiouser!"

Just removing other CI tasks make "Win64 native [vs2022]" pass:

@maflcko
Copy link
Member

maflcko commented Jun 29, 2022

Maybe it incorrectly uses a previous ccache?

This was referenced Jun 29, 2022
Merged
Closed
@hebasto
Copy link
Member Author

hebasto commented Jun 29, 2022

I have disabled ccache:

The getblock() function is a culprit, which creates a memory usage peak over 24 GB (verified a few times).

Another peak below 24 GB exists although.

@DrahtBot DrahtBot mentioned this pull request Jun 30, 2022
Merged
@hebasto
Copy link
Member Author

hebasto commented Jul 1, 2022 •
edited
Loading

Still observing a memory usage spike up to 25.38 GB.

It appears, that those spikes are caused by recursive usage of RPCResult. The applied refactoring has removed them.

@sipsorcery Is this bug familiar to you?

@hebasto hebasto marked this pull request as ready for review July 1, 2022 16:05
@DrahtBot DrahtBot mentioned this pull request Jul 1, 2022
Open
@sipsorcery
Copy link
Contributor

Still observing a memory usage spike up to 25.38 GB.

It appears, that those spikes are caused by recursive usage of RPCResult. The applied refactoring has removed them.

@sipsorcery Is this bug familiar to you?

Nope, I've never come across it. I only have 16GB though, so I'd guess even if I did somehow manage to replicate it my machine would crash or freeze first.

@sipsorcery
Copy link
Contributor

tACK 1240e82.

@maflcko maflcko added this to the 24.0 milestone Jul 6, 2022
@maflcko
Copy link
Member

maflcko commented Jul 6, 2022

Concept ACK 1240e82

hebasto added 5 commits July 7, 2022 19:59
@hebasto
rpc, refactor: Add getblock_prevout
01d95a3 
This change eliminates memory usage spike when compiling with Visual
Studio 2022 (at least in Cirrus CI environment).

Easy to review using
`git diff --color-moved-ws=allow-indentation-change --color-moved=dimmed-zebra`
@hebasto
rpc, refactor: Add decodepsbt_inputs
0c432cb 
This change eliminates memory usage spike when compiling with Visual
Studio 2022 (at least in Cirrus CI environment).

Easy to review using
`git diff --color-moved-ws=allow-indentation-change --color-moved=dimmed-zebra`
@hebasto
rpc, refactor: Add decodepsbt_outputs
555f9dd 
This change eliminates memory usage spike when compiling with Visual
Studio 2022 (at least in Cirrus CI environment).

Easy to review using
`git diff --color-moved-ws=allow-indentation-change --color-moved=dimmed-zebra`
@hebasto
build: Increase MS Visual Studio minimum version
88ec5d4 
Visual Studio 2022 with `/std:c++20` supports designated initializers.
@hebasto
refactor: Drop no longer needed util/designator.h
630c171
@hebasto
Copy link
Member Author

hebasto commented Jul 7, 2022

Rebased 1240e82 -> 630c171 (pr25472.06 -> pr25472.07) due to the conflict with #25500.

@sipsorcery
Copy link
Contributor

reACK 630c171.

This was referenced Jul 8, 2022
Closed
Closed
@fanquake fanquake merged commit c30b3e9 into bitcoin:master Jul 13, 2022
@hebasto hebasto deleted the 220625-desig branch July 13, 2022 15:32
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 13, 2022
@fanquake
Merge bitcoin#25472: build: Increase MS Visual Studio minimum version
e692850
@DrahtBot DrahtBot mentioned this pull request Jul 13, 2022
Closed
@hebasto hebasto mentioned this pull request Feb 9, 2023
Closed
@theuni
Copy link
Member

theuni commented Feb 13, 2023

I'm confused about this PR. If I'm reading correctly, MSVC now gets built as c++20 while other platforms are c++17. Seemingly so that we could drop our designated initializer code because it's non-standard for c++17.

That seems like a huge hammer for a small problem. I suspect I'm missing something?

@hebasto
Copy link
Member Author

hebasto commented Feb 13, 2023

I'm confused about this PR. If I'm reading correctly, MSVC now gets built as c++20 while other platforms are c++17. Seemingly so that we could drop our designated initializer code because it's non-standard for c++17.

That seems like a huge hammer for a small problem. I suspect I'm missing something?

I took a liberty to cross reference:

Designated initializers are supported since gcc 4.7 (Our minimum required is 8) and clang 3 (Our minimum required is 7). They work out of the box with C++17, and only msvc requires the C++20 flag to be set. I don't expect any of our msvc users will run into issues due to this. See also https://bitcoin.jonasschnelli.ch/ircmeetings/logs/bitcoin-core-dev/2022/bitcoin-core-dev.2022-03-10-19.00.log.html#l-114

@bitcoin bitcoin locked and limited conversation to collaborators Feb 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@sipsorcery sipsorcery sipsorcery approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
24.0
Development

Successfully merging this pull request may close these issues.
8 participants
@hebasto @DrahtBot @ajtowns @maflcko @laanwj @sipsorcery @theuni @fanquake