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

Faster process pool #1369

Merged
merged 2 commits into from
Jun 13, 2024
Merged

Faster process pool #1369

merged 2 commits into from
Jun 13, 2024

Conversation

101arrowz
Copy link
Collaborator

Improves the performance of the process pool by replacing the polling-based approach with Python's built-in process pool executor, which reduces synchronization overhead and reuses the Python interpreter when possible instead of spinning up a new instance for every single task.

@nilfm99
Copy link
Collaborator

nilfm99 commented Jun 11, 2024

I think the build failures are on Github's side, not ours. See actions/runner-images#6131 and https://www.githubstatus.com/

@101arrowz
Copy link
Collaborator Author

The build failures seem to be due to an internal error with GitHub Actions, but I noticed there's a test for Windows support. As mentioned in the diff, neither the old implementation nor the new one works with kernels that do not support POSIX fork(), NT included. However, the old implementation would silently use a spawn-based method (and might thereby fail while pickling the arguments), while the new one fails fast due to an assertion. In the specific situation that all arguments are pickleable and the code is running on Windows, however, the old code would work fine while the new version does not.

@nilfm99 nilfm99 self-requested a review June 11, 2024 20:35
@nilfm99
Copy link
Collaborator

nilfm99 commented Jun 11, 2024

I think this should be okay, the python wrapper is secondary to the C library IMO, and Linux/Mac is our main focus. Tagging @christosbampis, @krasuluk and @li-zhi who are the main users of the python library in case they have objections.

@nilfm99 nilfm99 merged commit fc6e403 into Netflix:master Jun 13, 2024
8 checks passed
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.

2 participants