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.

Sign up for GitHub

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

Jump to bottom

Use cgroups aware processor count by default #348

Merged
merged 1 commit into from
Aug 8, 2024
Merged

Use cgroups aware processor count by default #348

merged 1 commit into from
Aug 8, 2024

Conversation

y-yagi
Copy link
Contributor

@y-yagi y-yagi commented Aug 8, 2024

In containerized environments, a number of CPU cores isn't the same as the available CPUs. In this case, we need to consider cgroups.

concurrent-ruby now has the method to get that since v1.3.1. I think it's better to use this for setting more container environment friendly default value. Ref: ruby-concurrency/concurrent-ruby#1038

I keep Parallel.processor_count as is to keep supporting setting processor count via env(PARALLEL_PROCESSOR_COUNT).

@y-yagi y-yagi mentioned this pull request Aug 8, 2024
Closed
4 tasks
grosser
grosser reviewed Aug 8, 2024
View reviewed changes
lib/parallel.rb Outdated
def processor_count
require 'etc'
@processor_count ||= Integer(ENV['PARALLEL_PROCESSOR_COUNT'] || Etc.nprocessors)
require 'concurrent-ruby'
Copy link
Owner

Choose a reason for hiding this comment

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

how about this: no new dependency or version conflicts with other concurrent-ruby usage
... and maybe a readme entry for "if you want x add concurrent-ruby"

begin
  require 'concurrent-ruby'
  Concurrent.available_processor_count.to_i
rescue LoadError
  require 'etc'
  Etc.nprocessors
end

Copy link
Owner

Choose a reason for hiding this comment

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

maybe also use .floor instead of .to_i to make this more explicit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review.

how about this: no new dependency or version conflicts with other concurrent-ruby

This makes sense to me. I've updated my fix to use this approach.

@y-yagi y-yagi force-pushed the use_available_processor_count branch from f7bfe1b to 2e13dfd Compare August 8, 2024 07:26
@y-yagi
Use cgroups aware processor count by default
39f5630 
In containerized environments, a number of CPU cores isn't the
same as the available CPUs. In this case, we need to consider cgroups.

`concurrent-ruby` now has the method to get that since v1.3.1. I think it's better
to use this for setting more container environment friendly default value.
Ref: ruby-concurrency/concurrent-ruby#1038

I keep `Parallel.processor_count` as is to keep supporting setting
processor count via env(`PARALLEL_PROCESSOR_COUNT`).
@y-yagi y-yagi force-pushed the use_available_processor_count branch from 2e13dfd to 39f5630 Compare August 8, 2024 07:31
grosser
grosser reviewed Aug 8, 2024
View reviewed changes
lib/parallel.rb
def processor_count
require 'etc'
@processor_count ||= Integer(ENV['PARALLEL_PROCESSOR_COUNT'] || Etc.nprocessors)
@processor_count ||= Integer(ENV['PARALLEL_PROCESSOR_COUNT'] || available_processor_count)
Copy link
Owner

Choose a reason for hiding this comment

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

nice side-effect that if somehow etc is broken you can set env var now :)

grosser
grosser reviewed Aug 8, 2024
View reviewed changes
lib/parallel.rb
def available_processor_count
require 'concurrent-ruby'
Concurrent.available_processor_count.floor
rescue LoadError, NoMethodError
Copy link
Owner

Choose a reason for hiding this comment

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

NoMethodError is a nice touch 👍

@grosser grosser merged commit 21fdad7 into grosser:master Aug 8, 2024
6 checks passed
@grosser
Copy link
Owner

grosser commented Aug 8, 2024

1.26.0

@ahorek
Copy link

ahorek commented Aug 8, 2024

this change causes "negative array size" errors because

> Concurrent.available_processor_count.floor
 => -1
> Concurrent.physical_processor_count
 => 6
> Concurrent.processor_count
 => 12
> Etc.nprocessors
 => 12
> Concurrent.cpu_quota.floor
 => -1

according to docs
cpu.cfs_quota_us = -1: Unlimited CPU time.
cpu.cfs_quota_us = 0: No CPU time (tasks are blocked).
cpu.cfs_quota_us > 0: Limited CPU time as specified.

it looks like a bug in ruby-concurrency/concurrent-ruby#1038

@grosser
Copy link
Owner

grosser commented Aug 8, 2024 via email

@ahorek
Copy link

ahorek commented Aug 8, 2024

You can fall back to using Etc.nprocessors if Concurrent.available_processor_count returns an invalid value, such as -1 (negative) or 0.

grosser added a commit that referenced this pull request Aug 8, 2024
@grosser
Revert "Merge pull request #348 from y-yagi/use_available_processor_c…
c316cc5 
…ount"

This reverts commit 21fdad7, reversing
changes made to 9644d13.

see #349
@grosser
Copy link
Owner

grosser commented Aug 8, 2024

@y-yagi any idea why we'd get negative numbers ?
... and thoughts on if we should discard them to use Etc or what they mean ?

more feedback in #349
... revert, yanked 1.26.0 (and released 1.26.1 so local caches get purged too)

@y-yagi
Copy link
Contributor Author

y-yagi commented Aug 8, 2024

@ahorek
Thanks for your feedback!

@grosser
As @ahorek described, I think this is a bug of concurrent-ruby. I will try to fix it first.

@y-yagi y-yagi deleted the use_available_processor_count branch August 8, 2024 23:19
y-yagi added a commit to y-yagi/concurrent-ruby that referenced this pull request Aug 9, 2024
@y-yagi
Fix the return value of Concurrent.available_processor_count when `…
ddf62f6 
…cpu.cfs_quota_us` is -1

I tried to use `Concurrent.available_processor_count` in `parallel`
gem, but we got some feedback `Concurrent.available_processor_count`
returned a negative value.
grosser/parallel#348 (comment)
grosser/parallel#349 (comment)

According to the https://docs.kernel.org/scheduler/sched-bwc.html#management,
The default value of `cpu.cfs_quota_us` is -1. In that case,
cgroup does not adhere to any CPU time restrictions.

This PR adds the case of `cpu.cfs_quota_us` is -1 to `#cpu_quota`
to return processor count from `Concurrent.available_processor_count`
in that case.
@y-yagi y-yagi mentioned this pull request Aug 9, 2024
Merged
y-yagi added a commit to y-yagi/concurrent-ruby that referenced this pull request Aug 10, 2024
@y-yagi
Fix the return value of Concurrent.available_processor_count when `…
7306b26 
…cpu.cfs_quota_us` is -1

I tried to use `Concurrent.available_processor_count` in `parallel`
gem, but we got some feedback `Concurrent.available_processor_count`
returned a negative value.
grosser/parallel#348 (comment)
grosser/parallel#349 (comment)

According to the https://docs.kernel.org/scheduler/sched-bwc.html#management,
The default value of `cpu.cfs_quota_us` is -1. In that case,
cgroup does not adhere to any CPU time restrictions.

This PR adds the case of `cpu.cfs_quota_us` is -1 to `#cpu_quota`
to return processor count from `Concurrent.available_processor_count`
in that case.
y-yagi added a commit to y-yagi/concurrent-ruby that referenced this pull request Aug 10, 2024
@y-yagi
Fix the return value of Concurrent.available_processor_count when `…
7fea31d 
…cpu.cfs_quota_us` is -1

I tried to use `Concurrent.available_processor_count` in `parallel`
gem, but we got some feedback `Concurrent.available_processor_count`
returned a negative value.
grosser/parallel#348 (comment)
grosser/parallel#349 (comment)

According to the https://docs.kernel.org/scheduler/sched-bwc.html#management,
The default value of `cpu.cfs_quota_us` is -1. In that case,
cgroup does not adhere to any CPU time restrictions.

This PR adds the case of `cpu.cfs_quota_us` is -1 to `#cpu_quota`
to return processor count from `Concurrent.available_processor_count`
in that case.
eregon pushed a commit to ruby-concurrency/concurrent-ruby that referenced this pull request Aug 10, 2024
@y-yagi @eregon
Fix the return value of Concurrent.available_processor_count when `…
98d0f16 
…cpu.cfs_quota_us` is -1

I tried to use `Concurrent.available_processor_count` in `parallel`
gem, but we got some feedback `Concurrent.available_processor_count`
returned a negative value.
grosser/parallel#348 (comment)
grosser/parallel#349 (comment)

According to the https://docs.kernel.org/scheduler/sched-bwc.html#management,
The default value of `cpu.cfs_quota_us` is -1. In that case,
cgroup does not adhere to any CPU time restrictions.

This PR adds the case of `cpu.cfs_quota_us` is -1 to `#cpu_quota`
to return processor count from `Concurrent.available_processor_count`
in that case.
@eregon
Copy link

eregon commented Aug 10, 2024

Concurrent.available_processor_count is now fixed upstream and released: ruby-concurrency/concurrent-ruby#1060
Sorry for the bug!

grosser added a commit that referenced this pull request Aug 11, 2024
@grosser
Revert "Revert "Merge pull request #348 from y-yagi/use_available_pro…
fcae579 
…cessor_count""

This reverts commit c316cc5.
@grosser
Copy link
Owner

grosser commented Aug 11, 2024

re-released as v1.26.2 🤞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grosser grosser grosser left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@y-yagi @grosser @ahorek @eregon