1333059 - GetCPUCount: _SC_NPROCESSORS_ONLN may not be what you want

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Lars T Hansen [:lth]]

On my ARM dev board with Ubuntu 14, calling sysconf with _SC_NPROCESSORS_ONLN returns "1" even if there are four cores on the systems.  (Presumably only one is needed to run my three-liner test program and the others are all sleeping.)  The result is that the performance is not very good; for example, baseline-compiling AngryBots from the shell takes 2.6s instead of the 1.4s that I see if I call sysconf with _SC_NPROCESSORS_CONF, which returns "4".

I don't know if this affects Android, of course, so no conclusions there yet.  And I don't even know if it affects Firefox on Linux generally; presumably many cores may concurrently be active when we compute this number.  But then the result of the computation may be a little random.

Needs investigation.

Comment 1

[Lars T Hansen [:lth]]

Rumors about similar issues on Android, also need followup:
http://stackoverflow.com/questions/19585312/detecting-number-of-cpu-cores-on-android-from-native-code

Interesting comments on the issue here, around line 677, apparently _SC_NPROCESSORS_CONF mostly works but is not ideal in all environments:
https://fossies.org/linux/mono/mono/utils/mono-proclib.c

Comment 2

[Luke Wagner [:luke]]

It seems both constants are used in the tree but the official NSPR PR_GetNumberOfProcessors(), which we return for navigator.hardwareConcurrency, uses _SC_NPROCESSORS_CONF.

That Mono comment is really interesting, in particular their recommendation at the end.  Since we're client-side and probably not having to worry about that explicit-CPU-affinity case, it sounds like _CONF is good enough.  If we did use sched_getaffinity(), though, it means that when we wanted to test scalability using taskset, we wouldn't also have to use --thread-count=N to manually override the number of CPUs.

Comment 3

[Lars T Hansen [:lth]]

Attachment: bug1333059-processor-count.patch

Keeping it simple: on ARM/ARM64, if we have _SC_NPROCESSORS_CONF, use it; otherwise fall back to existing code.

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Please see https://bugzilla.mozilla.org/show_bug.cgi?id=663970, which fixed the exact same issue in NSPR, 5.5 years ago.

Comment 5

[Lars T Hansen [:lth]]

Yes, I'm aware of the NSPR code (see also comment 2), but we do not use NSPR when building the JS shell stand-alone.  (That's my excuse.  I don't know what everyone else's excuses are :)

But it's perhaps an indication that what we should do here ismove our GetCPUCount into our (tiny) NSPR shim so that we get the corrected function from NSPR when we link against Firefox.  Luke, opinions?

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8829473 [details] [diff] [review]
bug1333059-processor-count.patch

Review of attachment 8829473 [details] [diff] [review]:
-----------------------------------------------------------------

This patch seems like a strict improvement over trunk, so landing as-is sounds like a fine first step.  But yes it does seem like a good idea to try to instead reuse PR_GetNumberOfProcessors() (or, alternatively, move PR_GetNumberOfProcessors() into MFBT so we can easily remove the other N ad hoc sysconf() queries).  Perhaps that could be filed as a separate bug?

::: js/src/jsgc.cpp
@@ +3213,2 @@
>      if (ncpus == 0) {
> +#if (defined(JS_CODEGEN_ARM) || defined(JS_CODEGEN_ARM64)) && defined(_SC_NPROCESSORS_CONF)

nit: can you put this as an #elif after the XP_WIN (to keep the two sysconf()s next to each other)?

Comment 7

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b70259d3610aa2ccd0f428043a19bbae83cd28de
Bug 1333059 - use sysconf(_SC_NPROCESSORS_CONF) for numcores on ARM systems that have it.  r=luke

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b70259d3610a
use sysconf(_SC_NPROCESSORS_CONF) for numcores on ARM systems that have it.  r=luke

Comment 9

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #6)
> Comment on attachment 8829473 [details] [diff] [review]
> bug1333059-processor-count.patch
> 
> Review of attachment 8829473 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> it does seem like a good idea to try
> to instead reuse PR_GetNumberOfProcessors() (or, alternatively, move
> PR_GetNumberOfProcessors() into MFBT so we can easily remove the other N ad
> hoc sysconf() queries).  Perhaps that could be filed as a separate bug?

Mm, that could be bug 507718 maybe.  A bit quiet over there the last five or six years.  There are other candidates.

How about we move the core count functionality into js/src/threading so that when that directory is promoted to MFBT (cf jimb's comment on dev-platform) by whatever mechanism that requies, the core count functionality moves along with it?

Comment 10

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=80e3b537b6c7

Comment 11

[Lars T Hansen [:lth]]

Also relevant is bug 956899, of course.

Comment 12

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=471fad02e1e0

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b70259d3610a