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

Expose NIOThreadPool.numberOfThreads publicly. #2676

Merged
merged 2 commits into from Mar 11, 2024

Conversation

gmilos
Copy link
Contributor

@gmilos gmilos commented Mar 9, 2024

Motivation:

numberOfThreads is a required parameter to the public constructor, it's therefore known to the caller. Any use case that wants to optimise its logic for the parallelism offered by the particular NIOThreadPool needs to carry the number of threads separately from the NIOThreadPool itself. We should exposes numberOfThreads publicly from NIOThreadPool to avoid this complexity and possible bugs.

This is especially true for NIOThreadPool.singleton where the decision on the number of threads used is achieved through some complex and dynamic logic. It's therefore best not to second guess the result of those calculations.

Modifications:

Flip the visibility of numberOfThreads in NIOThreadPool to public.

Result:

Some complexity gets removed from client packages.

Motivation:

`numberOfThreads` is a required parameter to the public constructor, it's therefore known to the caller. Any use case that wants to optimise its logic for the parallelism offered by the particular `NIOThreadPool` needs to carry the number of threads separately from the `NIOThreadPool` itself. We should exposes `numberOfThreads` publicly from `NIOThreadPool` to avoid this complexity and possible bugs.

This is especially true for `NIOThreadPool.singleton` where the decision on the number of threads used is achieved through some complex and dynamic logic. It's therefore best not to second guess the result of those calculations.

Modifications:

Flip the visibility of `numberOfThreads` in `NIOThreadPool` to `public`.

Result:

Some complexity gets removed from client packages.
@gmilos gmilos requested review from Lukasa and FranzBusch March 9, 2024 21:54
@glbrntt glbrntt added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Mar 11, 2024
@Lukasa Lukasa enabled auto-merge (squash) March 11, 2024 09:55
@Lukasa Lukasa merged commit 887b2c1 into apple:main Mar 11, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants