Fix BSD cpu count by wmoxam · Pull Request #4466 · crystal-lang/crystal

wmoxam · 2017-05-26T13:52:09Z

System.cpu_count always returns -1 on BSD since fetching the number of CPUs via sysconf is not supported.

Instead use sysctl to fetch cpu count.

See: http://man.openbsd.org/sysctl.3 https://www.freebsd.org/cgi/man.cgi?query=sysctl&sektion=3

bew · 2017-05-26T13:57:47Z

Maybe this should leave under Crystal::System as it's a function with plateform dependent implementation?

wmoxam · 2017-05-26T14:12:08Z

@bew There is precedence for conditional platform dependant code elsewhere, ex: src/process/executable_path.cr

bcardiff · 2017-05-26T14:34:17Z

@wmoxam from CI there are two files where the formatter is not happy.

Error: formatting 'src/lib_c/amd64-unknown-openbsd/c/sysctl.cr' produced changes
Error: formatting 'src/lib_c/x86_64-portbld-freebsd/c/sysctl.cr' produced changes

To clarify @bew comment, #4450 is the first PR trying to move all the platform specifics in it's own namespace and offer a better abstraction. If you want to can take the stab here in this same PR. But if not a refactor will follow probably.

wmoxam · 2017-05-26T14:51:02Z

Okay good to know. Thanks @bcardiff

wmoxam · 2017-05-26T15:49:21Z

I'll refactor to match #4450

bcardiff · 2017-05-30T15:28:40Z

@wmoxam that PR is now merged. You can base the refactor with that final structure.
Having if flag to select which file to include and also use a skip_file macro in each file.

wmoxam · 2017-05-30T17:01:49Z

Thanks @bcardiff

I'll have it ready later this week

See: http://man.openbsd.org/sysctl.3 https://www.freebsd.org/cgi/man.cgi?query=sysctl&sektion=3

…x-bsd-cpu-count

RX14 · 2017-06-05T09:34:10Z

src/crystal/system/system.cr

@@ -0,0 +1,24 @@
+# :nodoc


We don't want to :nodoc: Crystal as we loose documenting macros and Crystal::VERSION. The :nodoc: should be placed on Crystal::System only.

RX14 · 2017-06-05T09:35:15Z

src/crystal/system/system.cr

+  # :nodoc
+  module System
+    # :nodoc
+    module System


I think that the methods in this module could go in just Crysatal::System itself.

Like replace src/system.cr with this file?

no keep the files the same but place the methods in Crystal::System not Crystal::System::System. It seems redundant and these are "misc" methods which can just go in Crystal::System itself.

IMO these are not "misc" methods, they are "System" (aka local computer system) utility methods for querying a local system's properties.

My understanding of the structure is that Crystal::System is simply a wrapper namespace for platform specific implementations.

Crystal::System::System is meant to contain platform specific implementations of the "System" methods.

It's confusing for sure since we're using the word 'System" for two different things. Perhaps a there is a different name that is more suitable?

What about Crystal::System::Inspect?

@bcardiff If we changed that name we'd have to change the name of System to Inspect which would make little sense. I'd rather have bit of duplication than a nonsensical name.

It's true there is no much value for some inspection method to be in it's own module for now.
Let's go with @RX14 proposal. So,

src/crystal/system.cr with the if flags to include src/crystal/{platform}/system.cr/etc.

src/crystal/{platform}/system.cr/etc. with the actual calls to lib C.

Leave src/system.cr with require "crystal/system" and delagation to Crystal::System.hostname.

::System is the public api with documentation.
::Crystal::System is :nodoc:. We can refactor later if many method end up there.

Otherwise it will affect documentation elsewhere

wmoxam · 2017-06-13T03:43:52Z

@RX14 @bcardiff How does this look now?

RX14 · 2017-06-13T08:57:52Z

src/crystal/system.cr

+  # :nodoc
+  module System
+    # Returns the hostname
+    # def.self.hostname


Typo: . instead of

RX14 · 2017-06-13T08:58:42Z

src/crystal/system.cr

+  require "./system/unix/sysctl_cpucount"
+{% else %}
+	# TODO: restrict on flag?(:unix) after crystal > 0.22.0 is released
+    require "./system/unix/sysconf_cpucount"


The indentation on these two lines is off.

Weird that the formatter didn't pick up on this

@wmoxam it's because the formatter doesn't format macro code

RX14 · 2017-06-13T09:04:30Z

src/crystal/system/unix/hostname.cr

+        raise Errno.new("Could not get hostname")
+      end
+      len = LibC.strlen(buffer)
+      {len, len}


Are you sure this string doesn't contain utf8?

I didn't implement this, I only moved it 😅

RX14 · 2017-06-13T09:10:31Z

src/crystal/system/unix/sysctl_cpucount.cr

+    if LibC.sysctl(mib, 2, pointerof(ncpus), pointerof(size), nil, 0) == 0
+      ncpus
+    else
+      -1


Don't we want to raise here?

Possibly? I was just matching the behaviour of the existing sysconf version

Is that the behaviour of other standard Crystal libs, or do they just rely on return value?

RX14 · 2017-06-13T09:10:59Z

src/crystal/system/unix/sysconf_cpucount.cr

+
+module Crystal::System
+  def self.cpu_count
+    LibC.sysconf(LibC::SC_NPROCESSORS_ONLN)


Similarly, we should check the return value and raise.

I think this could be part of a separate PR as the return value isn't checked elsewhere. Ex: Process.times

ysbaddaden · 2017-06-13T14:38:08Z

If changes to the API are required it may later in another pull request, but this shouldn't delay this patch which fixes issues on *BSD.

RX14 · 2017-06-13T14:45:19Z

I forgot that this was just a straight copy of the existing behaviour... Returning -1 would be something to change in the future though.

Sija · 2017-06-13T20:25:56Z

Build #4466 errored on osx with xcode8.2, again... Would it be a good time to merge #4528?

bcardiff

LGTM. Should be merged as a single commit.

Wesley Moxam added 2 commits May 26, 2017 09:50

Use sysctl to return the number of CPUs on BSD systems

675442d

See: http://man.openbsd.org/sysctl.3 https://www.freebsd.org/cgi/man.cgi?query=sysctl&sektion=3

Avoid duplication of sysctl definition

5cab9c8

mverzilli added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib labels May 26, 2017

Whoops, missed adding the require

d77e265

Use standard formatting

6919126

Wesley Moxam and others added 6 commits June 4, 2017 20:09

Use sysctl to return the number of CPUs on BSD systems

6e19fbd

See: http://man.openbsd.org/sysctl.3 https://www.freebsd.org/cgi/man.cgi?query=sysctl&sektion=3

Avoid duplication of sysctl definition

0f3e387

Whoops, missed adding the require

c526950

Use standard formatting

71bf419

Adds Crystal::System::System namespace

f3f87ce

Merge branch 'fix-bsd-cpu-count' of github.com:wmoxam/crystal into fi…

910e945

…x-bsd-cpu-count

wmoxam changed the base branch from master to collections_type_restrictions June 5, 2017 00:51

wmoxam changed the base branch from collections_type_restrictions to master June 5, 2017 00:51

wmoxam and others added 2 commits June 5, 2017 01:02

fix line length

df3b5b3

Fix formatting

cdba538

RX14 requested changes Jun 5, 2017

View reviewed changes

Wesley Moxam and others added 2 commits June 5, 2017 15:58

Remove extra nodoc declaration

b334f68

Otherwise it will affect documentation elsewhere

Re-org for less confusing naming

1b80956

RX14 requested changes Jun 13, 2017

View reviewed changes

Fixes formatting

a67cf46

RX14 approved these changes Jun 13, 2017

View reviewed changes

bcardiff approved these changes Jun 14, 2017

View reviewed changes

mverzilli merged commit 11e76e4 into crystal-lang:master Jun 15, 2017

mverzilli added this to the Next milestone Jun 15, 2017

wmoxam deleted the fix-bsd-cpu-count branch June 16, 2017 04:24

wmoxam mentioned this pull request Aug 1, 2017

Refactor platform specific process path #4770

Closed

Uh oh!

Conversation

wmoxam commented May 26, 2017

Uh oh!

bew commented May 26, 2017

Uh oh!

wmoxam commented May 26, 2017

Uh oh!

bcardiff commented May 26, 2017

Uh oh!

wmoxam commented May 26, 2017

Uh oh!

wmoxam commented May 26, 2017

Uh oh!

bcardiff commented May 30, 2017

Uh oh!

wmoxam commented May 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RX14 Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wmoxam commented Jun 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ysbaddaden commented Jun 13, 2017

Uh oh!

RX14 commented Jun 13, 2017

Uh oh!

Sija commented Jun 13, 2017

Uh oh!

bcardiff left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

wmoxam commented May 30, 2017 •

edited

Loading

RX14 Jun 5, 2017 •

edited

Loading