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

Adding a memory limit option #225

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

gazugafan
Copy link
Contributor

Asynchronous processes currently spawn with PHP's default memory limit (whatever's configured in php.ini). In my case, I increase this limit as needed in certain scripts using ini_set. This lead to a problem where these scripts would spawn processes that run out of memory... even though the memory limit was increased in the script, the sub-processes would all get the smaller default memory limit from php.ini and silently crash. Not ideal and tough to debug!

This PR adds a memoryLimit method to the Pool class that lets you configure a memory limit for each process spawned by the pool. You can specify a specific memory limit (using bytes, KB, M, etc.). Or, if you leave the parameter out, it will calculate how much memory is remaining in the parent script and divide it by the concurrency value--spreading out the remaining memory to the number of processes expected to be running. If you don't call the memoryLimit method at all, nothing happens--making this feature backwards-compatible.

@gazugafan
Copy link
Contributor Author

Ah, the failing tests for PHP 7.4 are probably due to the mixed type params used in the memoryLimit and bytes methods. Probably best to just remove the types for PHP 7 compatibility?

@MarcHagen
Copy link

MarcHagen commented Aug 28, 2024

My to cents, maybe not for this PR, but consider dropping PHP 7.4 support and moving the package more towards stricter typing.
#223 already made the start for this.
Adding return types to more functions will further enhance static analysis and less unpredictable behaviours.
(Same argument and reasoning as in #175)

@Nielsvanpach
Copy link
Member

We're okay with dropping everything below 8.2 and tagging a new major release.
Feel free to submit a PR for this!

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