Skip to content

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Jun 23, 2022

Besides upgrading Visual Studio, which seems inevitable, this PR also:

@DrahtBot DrahtBot added the Tests label Jun 23, 2022
@hebasto hebasto closed this Jun 23, 2022
@maflcko
Copy link
Member

maflcko commented Jun 24, 2022

Looks like it is now?

@maflcko
Copy link
Member

maflcko commented Jun 24, 2022

Please revert fa72e0b as well

@hebasto
Copy link
Member Author

hebasto commented Jun 24, 2022

Looks like it is now?

Still getting "Agent is not responding!" error (

@maflcko maflcko added this to the 24.0 milestone Jun 24, 2022
@hebasto hebasto reopened this Jun 24, 2022
@hebasto
Copy link
Member Author

hebasto commented Jun 24, 2022

@sipsorcery

Could you look at linker error here? (fwiw, I have wiped all caches)

nm, forgot to update PlatformToolset :)

@hebasto
Copy link
Member Author

hebasto commented Jun 25, 2022

@MarcoFalke

Please revert fa72e0b as well

See #25472.

@hebasto hebasto changed the title ci: Update Windows task image up to visualstudio2022 ci: Update Windows task image up to visualstudio2022 Jun 25, 2022
@hebasto hebasto marked this pull request as ready for review June 25, 2022 12:26
<< : *FILTER_TEMPLATE
windows_container:
cpu: 4
cpu: 6
Copy link
Member

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.

Copy link
Member

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

Copy link
Member Author

@hebasto hebasto Jun 25, 2022

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 (

Copy link
Member

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?

Copy link
Member Author

@hebasto hebasto Jun 26, 2022

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

image

On master + 156bc89 commit, with cleared caches:

  • time: 01:58:25
  • peak memory usage: 5.12 GB

image

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

#25483

@maflcko
Copy link
Member

maflcko commented Jun 26, 2022

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.

@sipsorcery
Copy link
Contributor

@sipsorcery

Could you look at linker error here? (fwiw, I have wiped all caches)

nm, forgot to update PlatformToolset :)

I find myself doing that everytime I do a build. Maybe this PR is a good time to udate the default?

@sipsorcery
Copy link
Contributor

ACK 3dabd28.

hebasto added 3 commits June 26, 2022 11:07
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.
- '%x64_NATIVE_TOOLS%'
- cd %CIRRUS_WORKING_DIR%
- ccache --zero-stats
- ccache --zero-stats --max-size=%CCACHE_SIZE%
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

CCACHE_SIZE: "200M"

Copy link
Member Author

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.

@hebasto
Copy link
Member Author

hebasto commented Jun 26, 2022

Updated 3dabd28 -> 05b2d9f (pr25460.01 -> pr25460.02, diff):

However, I think our windows docs should match the windows CI.

nm, forgot to update PlatformToolset :)

I find myself doing that everytime I do a build. Maybe this PR is a good time to udate the default?

@sipsorcery
Copy link
Contributor

reACK 05b2d9f.

@DrahtBot
Copy link
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:

  • #25472 (build: Increase MS Visual Studio minimum version by hebasto)

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.

Copy link
Contributor

@jarolrod jarolrod left a 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.

@hebasto
Copy link
Member Author

hebasto commented Jun 27, 2022

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.

@maflcko maflcko merged commit fe5911e into bitcoin:master Jun 27, 2022
@hebasto hebasto deleted the 220623-vs2022 branch June 27, 2022 06:14
fanquake added a commit to bitcoin-core/gui that referenced this pull request Jun 27, 2022
…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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 27, 2022
maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Oct 2, 2022
…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
@bitcoin bitcoin locked and limited conversation to collaborators Jun 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants