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

Add num_cpus to System module #4449

Merged
merged 2 commits into from May 24, 2017
Merged

Add num_cpus to System module #4449

merged 2 commits into from May 24, 2017

Conversation

miketheman
Copy link
Contributor

Signed-off-by: Mike Fiedler miketheman@gmail.com

@miketheman
Add num_cpus to System module
1b9499d 
Signed-off-by: Mike Fiedler <miketheman@gmail.com>
@miketheman
Copy link
Contributor Author

Resolves #4226

Wanted to minimize conditional logic or using flags.

@ysbaddaden
Copy link
Contributor

Maybe a more explicit naming?

@miketheman
Copy link
Contributor Author

Sure, open to suggestions. num_cpus_active, num_cpus_online

@bew
Copy link
Contributor

bew commented May 23, 2017

cpu_count ?

@RX14
Copy link
Contributor

RX14 commented May 23, 2017

I find the "cpu" terminology quite confusing, as it's actually the maximum number of concurrent threads the hardware can process at the same time - multicore and hyperthreading abstracts cpu_count away from the number of sockets or even the number of physical cores. However, referring to cpus as the number of threads the hardware can support seems to be pretty common terminology so I think cpu_count is the best so far.

@bararchy
Copy link
Contributor

Maybe cores ?

@drhuffman12
Copy link

Should we plan for big.LITTLE core flags too? Maybe in a separate PR?

@luislavena
Copy link
Contributor

Options core_count, available_cores, online_cores?

@mverzilli
Copy link

A quick and dirty research on other langs:

  • Java: availableProcessors
  • Python: cpu_count
  • C#: ProcessorCount
  • Go: NumCPU
  • Ruby: nprocessors
  • Haskell: getNumProcessors
  • Rust: num_cpus
  • Erlang: logical_processors_available

@bararchy
Copy link
Contributor

It seems that cpu and processors are the used terms

@ysbaddaden
Copy link
Contributor

It looks like cpu_count has the most votes? I like core but cpu is a little more explicit.

@bararchy
Copy link
Contributor

And if that's the choice then cpu > processors

mverzilli
mverzilli reviewed May 23, 2017
View reviewed changes
src/system.cr Outdated
@@ -18,4 +18,13 @@ module System
{len, len}
end
end

# Returns number of online CPUs
Copy link

Choose a reason for hiding this comment

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

May I suggest that we further detail what this method does? The current explanation will force anyone who doesn't know the exact meaning of "online CPUs" to review the LibC documentation. Ideally it'd be nice if we can save them the leap. I checked the docs in some other langs and they seem to use something like "number of [CPUs | Cores | Processors] available to the [program | process | runtime]"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly!

@RX14
Copy link
Contributor

RX14 commented May 23, 2017

I like calling them logical processors - it makes it clear what the number represents - but it might be too verbose.

@bew bew mentioned this pull request May 23, 2017
Merged
@miketheman
Copy link
Contributor Author

Thanks for the feedback everyone! I've added a commit incorporating the name change and docs update.

RX14
RX14 requested changes May 24, 2017
View reviewed changes
src/system.cr Outdated
@@ -18,4 +18,13 @@ module System
{len, len}
end
end

# Returns the number of processors which are currently online (i.e., available).
Copy link
Contributor

Choose a reason for hiding this comment

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

"Returns the number of logical processors available to the system"?

Copy link

Choose a reason for hiding this comment

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

@miketheman just apply @RX14's suggestion and I'll merge this :).

@miketheman
Change method name, add docs
0b276be 
As part of code review, update the name to be more in line with the
functionality provided, as well as update the initial docs.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
@miketheman
Copy link
Contributor Author

miketheman commented May 24, 2017 •
edited

@RX14 @mverzilli Done!

@mverzilli mverzilli added this to the Next milestone May 24, 2017
@mverzilli mverzilli merged commit 0e73cf4 into crystal-lang:master May 24, 2017
@mverzilli
Copy link

Thanks @miketheman for the contribution and everyone else for reviewing ❤️

@miketheman miketheman deleted the miketheman/numcpus branch August 4, 2017 12:25
@asterite asterite mentioned this pull request Sep 11, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mverzilli mverzilli mverzilli left review comments

@RX14 RX14 RX14 requested changes

Assignees
No one assigned
Labels
kind:feature status:accepted topic:stdlib
Projects
None yet
Milestone
0.23.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@miketheman @ysbaddaden @bew @RX14 @bararchy @drhuffman12 @luislavena @mverzilli