Skip to content

Workaround Python multiprocessing pool limitation to fix parallel test suite on Windows. #19216

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

Merged
merged 1 commit into from
Apr 20, 2023

Conversation

juj
Copy link
Collaborator

@juj juj commented Apr 20, 2023

Even latest Python 3.11 still has an issue that it cannot spawn a multiprocessing pool with > 61 processes on Windows. python/cpython#89240

Cap worker count to 61 on Windows to work around.

@juj juj enabled auto-merge (squash) April 20, 2023 18:04
@juj juj force-pushed the fix_windows_parallel_testsuite branch from 5b5ed9c to 6ea5d01 Compare April 20, 2023 18:07
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

How about just making get_num_cores return a value capped to 61 on windows? That seems simpler, and more future-proof.

@juj
Copy link
Collaborator Author

juj commented Apr 20, 2023

I thought about that at first, but that would pessimize other places that call get_num_cores(), e.g.

the manual multiprocessing spawner that I wrote to avoid multiprocessing.Pool() bugs at https://github.com/emscripten-core/emscripten/pull/19216/files#diff-4b34c94c6049a1b42f714f19c01fd70da33949b91e332e0ec0fb41711d75c537L200 .

Looks like also e.g. ninja builder invokes that

def run_ninja(build_dir):
  diagnostics.warning('experimental', 'ninja support is experimental')
  cmd = ['ninja', '-C', build_dir, f'-j{shared.get_num_cores()}']

and it probably does not have a problem with parallelizing to > 61 cores.

@juj juj merged commit 2bf4b38 into emscripten-core:main Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants