-
Notifications
You must be signed in to change notification settings - Fork 290
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
Change subprocesses to run at normal priority (#42715) #1555
base: main
Are you sure you want to change the base?
Conversation
I expected only a small effect in vcpkg-tool CI, but ... I see that Windows end-to-end test for this PR took only 15 min, vs. 25 min in previous runs. FTR this might increase memory pressure, with much more active cores as described in microsoft/vcpkg#42715. |
My experience is that ci times are not a reliable measurement because you never know under which conditions it was run. So local benchmarks would be more reliable. |
True, but these test aren't massive workloads anyways. |
That is true, but this can be solved by adjusting VCPKG_MAX_CONCURRENCY. On the other hand, there is currently no way of telling vcpkg to run build tools at normal priority, which is a huge waste on CI resources. In comparison, tools like cmake/ninja always build at normal priority, letting the user figure out how many cores to use without running out of mem, so I think it makes sense for vcpkg to do the same. On a side note, zipping (the post build packaging phase) was ridiculously slow for llvm when run in low priority, that may be a 30 min vs 5 min difference (I will probably submit a benchmark when I have time). |
This was done intentionally in ras0219-msft/vcpkg@a24ccdf @ras0219-msft Do you remember why this was set to IDLE_PRIORITY_CLASS? That seems wrong. I could imagine below normal but not idle. This might explain why we have seen such variable perf in CI. |
Is the problem fixed if you use BELOW_NORMAL rather than NORMAL? I can see an argument of not wanting to make interactive stuff on the system stuttery during a build being a reason to choose lower than normal, but idle may be triggering other forms of throttling like you describe in your issue filing. |
I think this is kind of intended. Otherwise machines become completely unresponsive. I don't see the same problem with 24 AMD cores and Win11 building LLVM |
How does Windows (10 vs. 11) handle power vs. efficiency cores for these scheduling levels? |
I have a win11 machine with 32 AMD cores in my workplace and it also has this issue. |
I think that is a trade-off that should be left for the users to decide. Currently we already have VCPKG_MAX_CONCURRENCY for that. |
I think setting process priority to idle was just a workaround for poor memory management by Windows. Also, this was changed to idle back in 2017 so I would expect this to be fixed in Windows by now. High memory usage is just a problem if you run out of swap. My MacBook sits constantly at 90% memory usage and still runs flawlessly. Even if idle priority would still be necessary, I don't think locking users into slow builds is a good idea. At the bare minimum there should be an escape hatch. |
@BillyONeal What do you think? If you merge this, you are the hero that saves users millions of dollars 😉! |
Now they'll never merge this cause it means that Microsoft would loose millions of dollars |
This looks like the right change to me, but will bring up to the team. |
That is the case. We want builds to yield to normal other work to keep the system responsive, though of course we don't want to leave perfectly good cores idle. Is the behavior you observe affected by power management settings or something similar? It's hard to evaluate the risk vs. benefit of this change without a better understanding of why it's happening. |
Couple more questions:
Could you try adding a call to
|
this was also my observation, fully related to the same problem |
@cenit Would you be willing to try the |
yes tomorrow I can test. Can you format your code as a patch file so that I can apply it without any possible mistake? |
ok perfect, i'll let you know |
OS Build: 19045.5247 Building ffmpeg with currently released vcpkg: Building ffmpeg with noQoS vcpkg (#1570): Building ffmpeg with normal-priority vcpkg (#1555) Building ffmpeg with currently released vcpkg under wsl2: The two PR are improving the situation only a little, maybe nothing if we consider errors in this very non-scientific measurement. |
in any case something changed in the windows kernel energy heuristic (please note all my data is related to win10, we still didn't move to 11), because i perfectly remember that the usual picture in the task manager was with only the efficiency cores taken by the compiler... |
@BillyONeal I tried. Didn't work either. Would you mind trying a demo that I wrote to see if the problem can be reproduced on your side? https://github.com/Binary-Song/vcpkg_test_project |
RE: #1555 (comment) @cenit it looks like neither change fixes it in your case, but you also aren't seeing the 'things are only running on E cores' reported by @Binary-Song . @Binary-Song No repro for me, sorry: |
in microsoft/vcpkg#39935 i reported exactly that problem, that the build was happening only in efficiency cores. Maybe building ffmpeg now for the test was not the best choice due to the strange toolchain? i picked it because i needed it and it was big enough -.-" (but the title of my old issue clearly states vcpkg+cmake+ninja, so clearly i am stupid enough in not testing that... will do it tomorrow) |
@cenit Stupid question: Is the vcpkg directory where builds are happening excluded from Defender in your tests?
I agree that sucks but unfortunately given that this PR doesn't fix it that argues against taking it. @Binary-Song I think adding a switch to do this might be acceptable but fixing one system and breaking responsiveness for everyone else doesn't seem like the correct tradeoff to me. |
@BillyONeal Yeah you are right. I tried this on the win 11 machine today and did not repro either. This is likely to be an OS bug that got fixed later. I will do some VM experiments about this. Adding a switch might be a reasonable solution if this is indeed an OS bug. |
on the laptop considered, there is CS and no way to change any settings. Anyway, here there are the results when building llvm, and there is a difference wrt ffmpeg Building llvm with currently released vcpkg: Building llvm with noQoS vcpkg (#1570): Building llvm with normal-priority vcpkg (#1555): As you can see, only the normal-priority version really made a difference (and this is expected imho, since EcoQoS is not in the win10 api), but only in this case (previous experiment with ffmpeg port was much more dubious in results) Can you explain why the llvm toolchain "sees" the different behavior while the ffmpeg one does not? Do we call external tools in vcpkg in different parts and so all of them should be upgraded to see the cpu usage improved also for ffmpeg? |
Given that it's clear we are unlikely to merge this in this form, I've set the PR to 'draft' status. We would be happy to accept adding an opt-in switch to turn on such behavior, but as proposed we are worried about breaking the responsiveness goals for which we intentionally run at lower priorities given that current versions of Windows do not seem to experience an issue. |
@BillyONeal How about adding an env var |
@BillyONeal Also, do I need to consider non-Windows? |
@Binary-Song please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
1 similar comment
@Binary-Song please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
@microsoft-github-policy-service agree |
Fixes microsoft/vcpkg#42715
The priority was changed to NORMAL_PRIORITY_CLASS so vcpkg can perform its tasks (building, zipping, etc) much faster.