Skip to content
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

Use multiprocessing start method spawn by default #854

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

simu
Copy link
Member

@simu simu commented Sep 22, 2023

This is already the default on macOS, and doesn't break anything on Linux. We'll need this at the latest when we want to switch to gojsonnet, since that library doesn't work correctly with multiprocessing start method fork.

Additionally, we reduce the memory usage of Commodore by using start method spawn because the multiprocessing child processes are created with smaller initial memory working sets. However, this causes some execution time overhead because more data needs to be copied to the new processes. Locally, we measured an increase in runtime of approximately 4 seconds when using spawn compared to fork.

Checklist

  • Keep pull requests small so they can be easily reviewed.
  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency, internal
    as they show up in the changelog

This is already the default on macOS, and doesn't break anything on
Linux. We'll need this at the latest when we want to switch to
gojsonnet, since that library doesn't work correctly with
multiprocessing start method `fork`.

Additionally, we reduce the memory usage of Commodore by using start
method `spawn` because the multiprocessing child processes are created
with smaller initial memory working sets. However, this causes some
execution time overhead because more data needs to be copied to the new
processes. Locally, we measured an increase in runtime of approximately
4 seconds when using `spawn` compared to `fork`.
@simu simu added the change PR with a minor change which affects users label Sep 22, 2023
@simu simu marked this pull request as ready for review September 22, 2023 12:43
@simu simu requested a review from a team as a code owner September 22, 2023 12:43
@simu simu merged commit 9021d13 into master Sep 22, 2023
20 checks passed
@simu simu deleted the feat/multiprocessing-spawn branch September 22, 2023 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change PR with a minor change which affects users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants