build: Increase MS Visual Studio minimum version #25472

pull hebasto wants to merge 5 commits into bitcoin:master from hebasto:220625-desig changing 11 files +246 −257

hebasto commented at 12:01 PM on June 25, 2022: member

Visual Studio 2022 with /std:c++20 supports designated initializers.
hebasto cross-referenced this on Jun 25, 2022 from issue ci: Update Windows task image up to `visualstudio2022` by hebasto
hebasto force-pushed on Jun 25, 2022
hebasto cross-referenced this on Jun 25, 2022 from issue Use designated initializers by maflcko
DrahtBot added the label Refactoring on Jun 25, 2022
hebasto force-pushed on Jun 25, 2022
hebasto force-pushed on Jun 25, 2022
DrahtBot commented at 3:26 PM on June 25, 2022: contributor


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.
hebasto force-pushed on Jun 25, 2022
DrahtBot cross-referenced this on Jun 25, 2022 from issue sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es by vasild
hebasto commented at 5:18 PM on June 25, 2022: member

Still observing a memory usage spike up to 25.38 GB.
ajtowns commented at 6:07 AM on June 27, 2022: contributor

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?
hebasto force-pushed on Jun 27, 2022
DrahtBot added the label Needs rebase on Jun 27, 2022
fanquake requested review from sipsorcery on Jun 28, 2022
sipsorcery approved
sipsorcery commented at 6:28 PM on June 28, 2022: member

Builds and passes tests for me.
maflcko commented at 6:44 PM on June 28, 2022: member

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 commented at 8:17 AM on June 29, 2022: member

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 commented at 8:27 AM on June 29, 2022: member

So it has nothing to do with designated initializers, but with compiling qt code with the c++20 option?
hebasto commented at 8:39 AM on June 29, 2022: member

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 force-pushed on Jun 29, 2022
hebasto force-pushed on Jun 29, 2022
DrahtBot removed the label Needs rebase on Jun 29, 2022
laanwj commented at 12:00 PM on June 29, 2022: member

Concept ACK
maflcko commented at 1:30 PM on June 29, 2022: member

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 commented at 1:32 PM on June 29, 2022: member

@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 commented at 2:34 PM on June 29, 2022: member
"Curiouser and curiouser!"

Just removing other CI tasks make "Win64 native [vs2022]" pass:
- https://cirrus-ci.com/task/5080247971348480 -- failed
- https://cirrus-ci.com/task/5782767731474432 -- passed
maflcko commented at 2:37 PM on June 29, 2022: member

Maybe it incorrectly uses a previous ccache?
DrahtBot cross-referenced this on Jun 29, 2022 from issue refactor: Move inbound eviction logic to its own translation unit by dergoegge
DrahtBot cross-referenced this on Jun 29, 2022 from issue refactor: Introduce EvictionManager by dergoegge
hebasto commented at 10:38 PM on June 29, 2022: member
I have disabled ccache:
- https://github.com/hebasto/bitcoin/commit/ac6794d3935fd7cd192e7becff062d82c1d17cd5 -- failed
- https://github.com/hebasto/bitcoin/commit/b205decd008c21ecfc11a23784d328103c46d505 -- passed
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 cross-referenced this on Jun 30, 2022 from issue net, test: Virtualise CConnman and add initial PeerManager unit tests by dergoegge
DrahtBot cross-referenced this on Jun 30, 2022 from issue net processing: Move CNode::nServices and CNode::nLocalServices to Peer by dergoegge
hebasto force-pushed on Jun 30, 2022
hebasto commented at 12:31 AM on July 1, 2022: member

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 force-pushed on Jul 1, 2022
hebasto marked this as ready for review on Jul 1, 2022
DrahtBot cross-referenced this on Jul 1, 2022 from issue Implement BIP 370 PSBTv2 by achow101
sipsorcery commented at 8:22 PM on July 2, 2022: member

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 commented at 8:06 PM on July 5, 2022: member

tACK 1240e827770dfe2c10badd9e42730430abb44ee5.
maflcko added this to the milestone 24.0 on Jul 6, 2022
maflcko commented at 6:46 AM on July 6, 2022: member

Concept ACK 1240e827770dfe2c10badd9e42730430abb44ee5
DrahtBot added the label Needs rebase on Jul 7, 2022

rpc, refactor: Add `getblock_prevout`

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`

01d95a3964

rpc, refactor: Add `decodepsbt_inputs`

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`

0c432cbbfa

rpc, refactor: Add `decodepsbt_outputs`

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`

555f9dd5d3

build: Increase MS Visual Studio minimum version

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

88ec5d40dc

refactor: Drop no longer needed `util/designator.h` 630c1711b4
hebasto force-pushed on Jul 7, 2022
hebasto commented at 7:08 PM on July 7, 2022: member

Rebased 1240e827770dfe2c10badd9e42730430abb44ee5 -> 630c1711b47ce50805f4dd2883777a100f7e5339 (pr25472.06 -> pr25472.07) due to the conflict with #25500.
sipsorcery commented at 7:54 PM on July 7, 2022: member

reACK 630c1711b47ce50805f4dd2883777a100f7e5339.
DrahtBot removed the label Needs rebase on Jul 7, 2022
DrahtBot cross-referenced this on Jul 8, 2022 from issue refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge
DrahtBot cross-referenced this on Jul 10, 2022 from issue rpc: add missing description in gettxout help text by MarnixCroes
fanquake merged this on Jul 13, 2022
fanquake closed this on Jul 13, 2022
hebasto deleted the branch on Jul 13, 2022
sidhujag referenced this in commit e692850c7c on Jul 13, 2022
DrahtBot cross-referenced this on Jul 13, 2022 from issue BIP324: Handshake prerequisites by dhruv
hebasto cross-referenced this on Feb 9, 2023 from issue build: Add CMake-based build system (1 of N) by hebasto
theuni commented at 7:16 PM on February 13, 2023: member

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 commented at 7:41 PM on February 13, 2023: member

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 locked this on Feb 13, 2024

Contributors

Labels

Milestone
24.0

Linked (view graph)

#24531 Use designated initializers #25460 ci: Update Windows task image up to `visualstudio2022`#27060 build: Add CMake-based build system (1 of N)

build: Increase MS Visual Studio minimum version #25472

Conflicts