[update-checkout] lower the default number of parallel processes

[charles-zablit]

This patch lowers the default number of parallel processes from os.cpu_count() * 2 to os.cpu_count().

This has the side effect of fixing an issue on Windows on systems with >= 16 CPU cores:

The implementation of multiprocessing.Pool in Python uses WaitForMultipleObjects on Windows. The maximum value of processes this function can take is capped by MAXIMUM_WAIT_OBJECTS, which is 64.

This avoids Pool to throw on Windows on systems with a high number of CPU cores (a 16 cores CPU will encounter this error).

rdar://159749646

[charles-zablit]

@swift-ci please test

[charles-zablit]

@swift-ci please smoke test

[charles-zablit]

@swift-ci please smoke test

[compnerd]

This is misleading. WFMO is limited to MAXIMUM_WAIT_OBJECTS. You can use RegisterWaitForObject or an IOCP. This is just a QoI issue, not a windows specific issue. Did you compare this to the implementation in CPython? You should bind it to the version if they are using a specific implementation.

[charles-zablit]

Did you compare this to the implementation in CPython?

concurrent/futures documents this limit: https://github.com/python/cpython/blob/424e2ab95a0b225effa4ec1057be467bf696f75e/Lib/concurrent/futures/process.py#L121-L122

multiprocessing seems to be using WaitForMultipleObjectsEx: https://github.com/python/cpython/blob/424e2ab95a0b225effa4ec1057be467bf696f75e/Modules/_multiprocessing/semaphore.c#L154

This is just a QoI issue, not a windows specific issue.

I have only encountered this issue on Windows and my fix is specifically tailored towards Windows.

This is misleading.

I'm happy to rephrase the comment with something more generic like: "Cap the number of processes to 60 to stay below the limit allowed by some platforms."

The end goal of this patch is to improve the Qol of update-checkout. Refactoring the code to support more than 60 processes does not seem like it's worth it, compared to just capping the value.

[compnerd]

Sorry, I meant that this is a QoI issue with python. I think that the comment would be better phrased as:

Workaround a limitation of Python's implementation of threading. The CPython implementation uses `WaitForSingleObject` which is bound by `MAXIMUM_WAIT_OBJECTS` rather than using `RegisterWaitForObject` or an I/O completion port. In order to ensure that we do not exceed that limit, bound the number of parallel jobs to 60.

I think that additionally, we should use something like:

import win32event
kMaximumThreads = win32event.MAXIMUM_WAIT_OBJECTS - 4

rather than the embedded 60.

[charles-zablit]

Sorry, I meant that this is a QoI issue with python.

I see, thanks for the clarification! I changed the comment to your suggestion.

I think that additionally, we should use something like:

import win32event
kMaximumThreads = win32event.MAXIMUM_WAIT_OBJECTS - 4

rather than the embedded 60.

Since win32event is not bundled with Python, I think it's best not to add a mandatory dependency to update-checkout. This would require adapting the CI script on Windows. I think it's fine to hardcode 60, as there are far fewer than 60 repositories anyways.

It's possible that a user has a version of Windows where MAXIMUM_WAIT_OBJECTS is set to something lower than 60 but that seems like a very rare edge case.

[compnerd]

We do seem to be growing the number of repositories that we need to checkout to build, so it feels like we should just grow this dependency if we want to go with update-checkout instead of repo.

[charles-zablit]

Let's see the pros/cons of both approaches:

A. Hardcoding 60:

Pros

• Small patch

Cons

• Might yield slower clone time (if we have more than 60 repos, we currently have 50). This might not even be true as llvm-project is the longest to clone and is almost always the limiting factor when cloning all the repositories.

B. Using win32events

Pros

• No hardcoded value. Can be updated if MAXIMUM_WAIT_OBJECTS changes value.

Cons

• Need to install a dependency to run update-checkout. This requires editing the CI scripts for all platforms.

---

If I did not miss anything, I think the cons vastly overweight the pros in approach B. It's not clear to me why requiring a dependency to run update-checkout is worth (eventually) cloning more than 60 repositories in parallel.

[al45tair]

I've just hit this problem myself, so I have some thoughts here :-)

Let's not add a dependency on win32event here. That just adds a lot of extra work for what is a fairly minor fix.

I agree with @adrian-prantl that 60 is in any case way more than we need. I suspect it's pointless going much higher than 16 at a time — the performance gains from being more parallel than that likely tail off very quickly, and additionally trying to simultaneously clone 60 repos might even be seen as abusive (though GitHub is big and will probably cope).

[adrian-prantl]

Even 60 seems to be very high to me, but generally limiting makes sense.

[charles-zablit]

@swift-ci please test

[charles-zablit]

@swift-ci please smoke test macOS

[charles-zablit]

@swift-ci please test macOS

[AnthonyLatsis]

@swift-ci please test macOS

[charles-zablit]

Hi @compnerd, is there anything else I should change on this PR in order to merge it? Regarding #84081 (comment), I still think we should avoid adding another dependecy to building Swift on Windows, with the win32event module, especially if its only purpose is to avoid hardcoding MAXIMUM_WAIT_OBJECTS.

[compnerd]

Hi @compnerd, is there anything else I should change on this PR in order to merge it? Regarding #84081 (comment), I still think we should avoid adding another dependecy to building Swift on Windows, with the win32event module, especially if its only purpose is to avoid hardcoding MAXIMUM_WAIT_OBJECTS.

I think that it is a reasonable dependency to add; if we are going to grow our own homebrew tools, then we need to start considering the cost of the additional dependencies to get the things to work properly. The better solution IMO is to just switch to repo.

[charles-zablit]

I think that it is a reasonable dependency to add;

My concern is that update-checkout does not have any dependencies at the moment. Adding an external dependency would mean adding a requirements.txt file or equivalent, needing to create a virtual environment, adding extra steps in the CI, etc.

I don't think this extra complexity is worth it for this simple Windows specific patch.

[al45tair]

Even 60 seems to be very high to me, but generally limiting makes sense.

Agreed, let's set it to a smaller number. 60 at once is unnecessary and unlikely to genuinely be any faster than (say) working 16 at a time.

[charles-zablit]

Since the maximum number of parallel processes should depend on the number of CPU cores, I have lowered the default value to cpu_count() instead of cpu_count() * 2.

[al45tair]

Since the maximum number of parallel processes should depend on the number of CPU cores, I have lowered the default value to cpu_count() instead of cpu_count() * 2.

cpu_count() * 2 is good for machines with small core counts. I think we just want min(cpu_count() * 2, 16) or similar, to stop beefier machines from over-doing things.

[charles-zablit]

@swift-ci please smoke test

[charles-zablit]

@swift-ci please smoke test