-
Notifications
You must be signed in to change notification settings - Fork 38.7k
ci: Update Windows task image up to visualstudio2022
#25460
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.
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
Conversation
|
Looks like it is now? |
|
Please revert fa72e0b as well |
Still getting "Agent is not responding!" error ( |
|
nm, forgot to update |
Dependency changes: - boost-* 1.78.0#0 -> 1.79.0#0
visualstudio2022
| << : *FILTER_TEMPLATE | ||
| windows_container: | ||
| cpu: 4 | ||
| cpu: 6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason to change this? With 6 cpu it finished 1 minute earlier compared to master ( https://cirrus-ci.com/task/5018066990923776 ), so it seems like a random fluke.
Also, if someone pushes several pull requests at the same time, the scheduler will hit the total cpu limit earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, looking at the graph, the CPU usage density is less, indicating that the 5th and 6th cpu may be idle most of the time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason to change this?
On master, the time required to get the task done with no caches is very close to 2 hours limit.
With 6 cpu it finished 1 minute earlier compared to master ( https://cirrus-ci.com/task/5018066990923776 ), so it seems like a random fluke.
Such comparison is not correct, because vcpkg_binary cache was invalidated for this PR. Functional tests are more then 10 minutes faster now.
Also, looking at the graph, the CPU usage density is less, indicating that the 5th and 6th cpu may be idle most of the time
Will 5 cpus be OK? UPDATE: Odd CPU number does not work (
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On master, the time required to get the task done with no caches is very close to 2 hours limit.
Ok, no objection then.
Is there a reason why vs22 needs more resources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why vs22 needs more resources?
On master, with cleared caches:
- time: 01:56:44
- peak memory usage: 5.08 GB
On master + 156bc89 commit, with cleared caches:
- time: 01:58:25
- peak memory usage: 5.12 GB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, there is no significant difference in resources at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be best to revert this. Looks like now this won't be scheduled until 4 hours later. If the CPU was reduced, so that it was scheduled earlier, it would take 15 minutes longer to run, but still finish earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be best to revert this. Looks like now this won't be scheduled until 4 hours later. If the CPU was reduced, so that it was scheduled earlier, it would take 15 minutes longer to run, but still finish earlier.
Ok, But a similar situation with ARM task been observed right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be best to revert this.
|
too bad that this doesn't fix #25472. Though, I guess it can't hurt to bump now, given that we'll have to do it anyway at some point. However, I think our windows docs should match the windows CI. |
I find myself doing that everytime I do a build. Maybe this PR is a good time to udate the default? |
|
ACK 3dabd28. |
Currently, the time it takes to get the "Win64 native" task done with all of the caches been invalidated is very close to the 2 hours limit. This task is the only one which runs on Windows Community Cluster, therefore this change should not affect other CI tasks.
Added Visual Studio 2022.
| - '%x64_NATIVE_TOOLS%' | ||
| - cd %CIRRUS_WORKING_DIR% | ||
| - ccache --zero-stats | ||
| - ccache --zero-stats --max-size=%CCACHE_SIZE% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is CCACHE_SIZE set at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 6 in 0dd3477
| CCACHE_SIZE: "200M" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On master:
Summary:
Hits: 239 / 239 (100.0 %)
Direct: 239 / 239 (100.0 %)
Preprocessed: 0 / 0
Misses: 0
Direct: 0
Preprocessed: 0
Uncacheable: 10
Primary storage:
Hits: 478 / 478 (100.0 %)
Misses: 0
Cache size (GB): 1.35 / 5.00 (27.06 %)
Use the -v/--verbose option for more details.
With this PR:
Summary:
Hits: 239 / 239 (100.0 %)
Direct: 239 / 239 (100.0 %)
Preprocessed: 0 / 0
Misses: 0
Direct: 0
Preprocessed: 0
Uncacheable: 10
Primary storage:
Hits: 478 / 478 (100.0 %)
Misses: 0
Cache size (GB): 0.07 / 0.20 (32.93 %)
Use the -v/--verbose option for more details.
Compare the "Cache size" string.
|
Updated 3dabd28 -> 05b2d9f (pr25460.01 -> pr25460.02, diff):
|
|
reACK 05b2d9f. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
jarolrod
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 05b2d9f
Changes look good to me, makes sense to update as visual studio 2019 will "will go out of support in October 2022". PR description could state some of the motivation for the update.
PR description has been just added. |
…64 native" task" b1d2fb4 Revert "ci: Increase CPU number for "Win64 native" task" (Hennadii Stepanov) Pull request description: This reverts commit 849cf96. It seems this should [improve](bitcoin/bitcoin#25460 (comment)) the total CI throughput. ACKs for top commit: MarcoFalke: review ACK b1d2fb4 Tree-SHA512: 833665bbea00b695dd1c600c6a47943d47b1a2a3560886b2056544cf62542d6f91b1f2e157ad082b02b67922f1fff14029da35b1a9d85339c2bf526f41b41836
…22.09.27` 281e7c7 ci: Bump vcpkg to the latest version `2022.09.27` (Hennadii Stepanov) Pull request description: Dependency changes in [vcpkg](https://github.com/microsoft/vcpkg) (2022.06.16.1 - [2022.09.27](https://github.com/microsoft/vcpkg/releases/tag/2022.09.27)): - boost 1.79.0#0 -> 1.80.0#0 - sqlite3 3.37.2#1 -> 3.39.2#0 - zeromq 4.3.4#5 -> 4.3.4#6 The recent update was in bitcoin/bitcoin#25460. ACKs for top commit: sipsorcery: tACK 281e7c7. Tree-SHA512: 624e2506eb16fb37d1ca0a71caa12e64f8709c0ddd280e3d1e0f6a8fa2a3667b0f8a2f52d553e096c9f8cd50e4e220e23a23fdb97076d7bcdfab0951e94909a1


Besides upgrading Visual Studio, which seems inevitable, this PR also:
2022.04.12#24847)ccache