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

feat: Add a new method to get the number of CPU cores to use for parallelism #127

Merged
merged 4 commits into from
Jul 25, 2024

Conversation

theofidry
Copy link
Owner

@theofidry theofidry commented Jul 24, 2024

I think the current primitives offered by the library are fine. What is not is that people use $counter->getCount() with the expectation that it is the number of cores they should use for parallelism (and it is the usage I've defaulted to as well, as this is how it was used in the original code that this library was taken from). But as highlighted in #125, it is not. A non exhaustive list:

  • It does not account for the fact that the current process reserves one core.
  • It does not account for a user defined limit (e.g. like mentioned in Detect available CPUs on Kubernetes #125 with Kubernetes).
  • It does not account for the current system load.

Seeing uv_available_parallelism, I've decide to introduce a new API that can address those points and make it easier for consumers to get the appropriate number of cores for the purpose of knowing the number of processes they can launch for parallelism, which is the main usage of this library.

I will add more parameters in the future to address the points mentioned above, but they will be added as default parameters hence will not break the API.

@theofidry
Copy link
Owner Author

/cc @Wirone

Copy link

@Wirone Wirone left a comment

Choose a reason for hiding this comment

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

Little suggestions from my side. Thank you very much for addressing this 🙂🍻!

README.md Outdated Show resolved Hide resolved
src/CpuCoreCounter.php Outdated Show resolved Hide resolved
tests/CpuCoreCounterTest.php Outdated Show resolved Hide resolved
@theofidry theofidry merged commit 6c6c0f7 into main Jul 25, 2024
20 checks passed
@theofidry theofidry deleted the fix/kubernetes branch July 25, 2024 20:54
@theofidry theofidry mentioned this pull request Jul 26, 2024
1 task
theofidry added a commit that referenced this pull request Jul 29, 2024
I would like to make cases like PHP-CS-Fixer/PHP-CS-Fixer#8129 easier to debug. There is 3 things to check:

- If each respective finder behaves as expected, this is the job of the `diagnose.php`.
- Which finder result got picked: the introduce `CpuCoreFinder::trace()` should answer that.
- A way to debug `CpuCoreFinder::getAvailableForParallelisation()`. Since #127 is not released yet, the signature of `::getAvailableForParallelisation()` to return a result value object which could capture the resolved parameters to be able to figure out with which values the calculation was done.
@sebastianbergmann
Copy link

Finally able to confirm this:

./tools/php-cs-fixer fix
PHP CS Fixer 3.64.0 (209f776) Space Sets by Fabien Potencier, Dariusz Ruminski and contributors.
PHP runtime: 8.3.11
Running analysis on 6 cores with 10 files per process.

Thank you!, @theofidry.

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