Closed Bug 1008453 Opened 10 years ago Closed 8 years ago

support for navigator.hardwareConcurrency

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: cabanier, Assigned: bzbarsky)

Details

(Keywords: dev-doc-complete, Whiteboard: [games:p1][platform-rel-Games])

Attachments

(3 files, 5 obsolete files)

See proposal: http://wiki.whatwg.org/wiki/NavigatorCores
Will be implement in blink (got LGTM's) and likely WebKit (https://bugs.webkit.org/show_bug.cgi?id=132588)
Assignee: nobody → cabanier
Try run: https://tbpl.mozilla.org/?tree=Try&rev=b37924907c24

This implementation matches WebKit
Attachment #8420479 - Flags: review?(bzbarsky)
Attachment #8420479 - Attachment is obsolete: true
Attachment #8420479 - Flags: review?(bzbarsky)
Comment on attachment 8420482 [details] [diff] [review]
Implementation + test for navigator.hardwareConcurrency

Fixed indentation
Attachment #8420482 - Flags: review?(bzbarsky)
Should this have a runtime flag?
Flags: needinfo?(bzbarsky)
For a start, this needs an intent to implement.

If you want to ship it on by default, it also needs an intent to ship.
Flags: needinfo?(bzbarsky)
Comment on attachment 8420482 [details] [diff] [review]
Implementation + test for navigator.hardwareConcurrency

Since this is supposed to expose information about how many workers to start, I think Ben should be doing the actual review.

That said, why is this returning long and not unsigned long?
Attachment #8420482 - Flags: review?(bzbarsky) → review?(bent.mozilla)
Attachment #8420482 - Attachment is obsolete: true
Attachment #8420482 - Flags: review?(bent.mozilla)
Attachment #8420752 - Flags: review?(bent.mozilla)
Do you think it is better to make the max number a pref with default value 8 instead of a hard-coded number?
(In reply to Xidorn Quan from comment #8)
> Do you think it is better to make the max number a pref with default value 8
> instead of a hard-coded number?

I could certainly do that and that would make it easier to up it later.
(In reply to Xidorn Quan from comment #8)
Yeah, that and the default max value reported should be the same as the default hardware thread limit per origin, which is 20.

This number as per the spec represents "the system's total number of logical processors available to the user agent per origin", which is currently 20 in Firefox.
(In reply to Xidorn Quan from comment #8)
> Do you think it is better to make the max number a pref with default value 8
> instead of a hard-coded number?

What should the pref be called? navigator.maxHardwareConcurrency?
Flags: needinfo?(quanxunzhen)
(In reply to Rik Cabanier from comment #11)
> (In reply to Xidorn Quan from comment #8)
> > Do you think it is better to make the max number a pref with default value 8
> > instead of a hard-coded number?
> 
> What should the pref be called? navigator.maxHardwareConcurrency?

I have no idea, but dom.maxHardwareConcurrency might be more reasonable?
Flags: needinfo?(quanxunzhen) → needinfo?(bent.mozilla)
You can use the already-existing dom.workers.maxPerDomain preference.
Attachment #8420752 - Flags: review?(bent.mozilla)
(In reply to Xidorn Quan from comment #12)
> (In reply to Rik Cabanier from comment #11)
> > (In reply to Xidorn Quan from comment #8)
> > > Do you think it is better to make the max number a pref with default value 8
> > > instead of a hard-coded number?
> > 
> > What should the pref be called? navigator.maxHardwareConcurrency?
> 
> I have no idea, but dom.maxHardwareConcurrency might be more reasonable?

OK. I will update the patch so it takes the 2 runtime flags into account. I cleared the review flag for now.
Attachment #8420752 - Attachment is obsolete: true
Attachment #8420752 - Flags: review?(bent.mozilla)
Please refrain from landing this patch unless we decide that we want to implement and ship this on the dev-platform thread.
Attachment #8420752 - Flags: review?(bent.mozilla)
Comment on attachment 8422200 [details] [diff] [review]
Implementation + test for navigator.hardwareConcurrency

I won't land this until there is agreement that this is fine to have in the platform.
Attachment #8422200 - Flags: review?(bent.mozilla)
Comment on attachment 8422200 [details] [diff] [review]
Implementation + test for navigator.hardwareConcurrency

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

::: dom/base/Navigator.cpp
@@ +629,5 @@
> +
> +uint32_t Navigator::HardwareConcurrency()
> +{
> +  static uint32_t NumberOfCores = -1;
> +  if (NumberOfCores > 0) {

If NumberOfCores is unsigned and its initial value is "-1", won't this `NumberOfCores > 0` check always be true?

@@ +643,5 @@
> +
> +  if (maxWorkers > maxPerDomain) {
> +    maxWorkers = maxPerDomain;
> +  }
> +  

Trailing whitespace.
Comment on attachment 8422200 [details] [diff] [review]
Implementation + test for navigator.hardwareConcurrency

I'm trying to move some stuff out of Ben's review queue.  In this case, I think Ehsan would be a fine reviewer (IIRC he disliked the idea but that's an even better reason to have him review).  Note that Ehsan's AFK today and tomorrow.
Attachment #8422200 - Flags: review?(bent.mozilla) → review?(ehsan.akhgari)
Flags: needinfo?(bent.mozilla) → needinfo?(ehsan.akhgari)
Please see the extensive discussion on dev.platform.  I don't think that the concerns raised there were properly addressed, so I think a proper resolution for this bug is WONTFIX until the concerns there are dealt with.  FWIW I have a very hard time reconciling how those concerns may be addressed with the current shape of the API.
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(ehsan.akhgari)
Resolution: --- → WONTFIX
Attachment #8422200 - Flags: review?(ehsan.akhgari)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO on Thu/Fri) from comment #20)
> Please see the extensive discussion on dev.platform.  I don't think that the
> concerns raised there were properly addressed, so I think a proper
> resolution for this bug is WONTFIX until the concerns there are dealt with. 
> FWIW I have a very hard time reconciling how those concerns may be addressed
> with the current shape of the API.

It's unfortunate that you don't trust authors. We'll just see how this API ends up being used in Safari and Chrome and revisit if it turns out good/bad.
(In reply to Rik Cabanier from comment #21)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO on
> Thu/Fri) from comment #20)
> > Please see the extensive discussion on dev.platform.  I don't think that the
> > concerns raised there were properly addressed, so I think a proper
> > resolution for this bug is WONTFIX until the concerns there are dealt with. 
> > FWIW I have a very hard time reconciling how those concerns may be addressed
> > with the current shape of the API.
> 
> It's unfortunate that you don't trust authors.

It makes me sad that you're still trying to simply glance over the objections by putting them in this bucket, but I don't want to rehash the discussion here.

> We'll just see how this API
> ends up being used in Safari and Chrome and revisit if it turns out good/bad.

Yes, we can definitely re-evaluate in the future.  I would be interested to see how this will be used in real Web applications, and how they will deal with the problems of this API, but I'm still worried that the API will be used without understanding its limitations.
Emscripten has gained multithreading support with the SharedArrayBuffer specification, and there is a multithreaded port of Unity3D being developed. Unity3D can scale up a task pool to run physics, particles, skinning, asset loading, AI and other tasks in any number of threads. It would be very useful to use navigator.hardwareConcurrency support here to know how many workers make sense. Overcommitting the number of workers can be catastrophic to performance and smooth jitter free experience.

I am not sure if I was able to locate the correct original discussion that took place. Could someone summarize the pending objections that blocked implementing this? Were there other concerns than fingerprinting?
Well, a page can't anyhow know what other pages are running and how many workers they are using, or
what other software is running actively on the machine, so would this really help with
Unity3D in general case?
Actually nevermind, I think I found the discussion here: https://lists.mozilla.org/pipermail/dev-platform/2014-May/004596.html , and I don't wish to draw resources to discuss further here. Sorry for the noise!
Reopening based on newer m.d.p thread:
  https://groups.google.com/d/topic/mozilla.dev.platform/1FYb4cH6y7A/discussion
and +1 from sicking, roc, bkelly and jst.  Rik, by any chance would you be willing to rebase the patch and rerequest review?
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
It's been a while since I committed something but I'll give it a shot
Attachment #8422200 - Attachment is obsolete: true
Attachment #8684527 - Attachment is obsolete: true
(In reply to Luke Wagner [:luke] from comment #26)
> Reopening based on newer m.d.p thread:
>  
> https://groups.google.com/d/topic/mozilla.dev.platform/1FYb4cH6y7A/discussion
> and +1 from sicking, roc, bkelly and jst.  Rik, by any chance would you be
> willing to rebase the patch and rerequest review?

I refreshed the patch, but I no longer have try access :-(
Can someone else try to push it through? I'll fix bugs if they pop up
I'm under the impression that the people who are +1ing in the new thread don't want clamping. Also, isn't dom.maxHardwareConcurrency redundant when we have dom.workers.maxPerDomain?
(In reply to Eli Grey (:sephr) from comment #31)
> I'm under the impression that the people who are +1ing in the new thread
> don't want clamping. Also, isn't dom.maxHardwareConcurrency redundant when
> we have dom.workers.maxPerDomain?

I don't disagree. Chrome shipped with no clamping and I haven't heard of any problems with their approach.
Since this was so contentious, we should get everyone on board before we make this change.
I haven't seen any requests to clamp navigator.hardwareConcurrency either.
This patch uses PR_GetNumberOfProcessors() instead which is used all over and seems to ultimately get down to the same syscalls.  Also, an Atomic is used to avoid what would technically be a race on the static.
Attachment #8720478 - Flags: review?(bzbarsky)
Comment on attachment 8720478 [details] [diff] [review]
Implementation + test for navigator.hardwareConcurrency

I guess we decided to not do clamping?

>+    numberOfProcessors.compareExchange(0, PR_GetNumberOfProcessors());

Please document why you don't care if false is returned: that means you just lost the race, but the value is set up already anyway.

But why does this need to be atomic?  I don't think it does...  This function will only be called on the main thread.

You _do_ need to implement this for workers, right?  See what the IDL in https://wiki.whatwg.org/wiki/NavigatorCores#API looks like (though the "WorkerNavigator implements" bit will need to go in WorkerNavigator.webidl).  The WorkerNavigator bit _will_ need atomics.  Please add a worker test.

As far as the syscalls PR_GetNumberOfProcessors ends up at, it differs from Rik's patch in two ways:

1)  On Mac, it will use the deprecated HW_NCPU, which is documented as "number of cpus".  Rik's patch used "hw.logicalcpu" instead.  Note that there are also "hw.physicalcpu", "hw.physicalcpu_max", and "hw.logicalcpu_max" values.  The difference between the non-max and max ones is that the non-max ones are "the number in the current power management mode" and the max ones are the max it could be without rebooting.  I guess there's no hotplugging of CPUs based on power management on MacOS in general, but nothing even guarantees that HW_NCPU means "logical CPUS", though in practice it seems to.

2)  On Linux, it will try to return a number based on groveling /sys/devices/system/cpu/present and fall back to sysconf() only if this fails.  The idea is to return the number of actual CPUs, whereas sysconf() will only return the ones in /sys/devices/system/cpu/online, I guess?  Again, in practice I expect this to not matter for the moment on consumer hardware where hotplugging CPUs based on power management mode is not a thing, right?

It might be worth it to file an NSPR bug to have a PR_GetNumberOfLogicalProcessors function with better defined semantics in terms of its interaction with power management modes or something.

Anyway, r- for the worker thing.  That definitely needs to be addressed.
Attachment #8720478 - Flags: review?(bzbarsky) → review-
(Thanks for offering to help this patch along!)
Flags: needinfo?(bzbarsky)
I filed bug 1256504 on NSPR.
Assignee: cabanier → bzbarsky
Status: REOPENED → ASSIGNED
Comment on attachment 8730525 [details] [diff] [review]
Add support for navigator.hardwareConcurrency

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

::: dom/webidl/Navigator.webidl
@@ +480,5 @@
>  #endif
> +
> +[NoInterfaceObject, Exposed=(Window,Worker)]
> +interface NavigatorCPU {
> +    readonly attribute unsigned long hardwareConcurrency;

I think two space indentation is customary in this file.

::: dom/workers/RuntimeService.cpp
@@ +2463,5 @@
> +  // other worker has initialized numberOfProcessors, so we're good to go.
> +  if (!clampedHardwareConcurrency) {
> +    int32_t numberOfProcessors = PR_GetNumberOfProcessors();
> +    if (numberOfProcessors <= 0) {
> +      numberOfProcessors = 1; // Must be one there somewhere

lol
Attachment #8730525 - Flags: review?(khuey) → review+
Comment on attachment 8730525 [details] [diff] [review]
Add support for navigator.hardwareConcurrency

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

::: dom/webidl/Navigator.webidl
@@ +478,5 @@
>    DOMRequest mozPay(any jwts);
>  };
>  #endif
> +
> +[NoInterfaceObject, Exposed=(Window,Worker)]

Does the Exposed here actually do something?  If yes, why are we excluding ServiceWorker?
> Does the Exposed here actually do something?

Yes, it does.  It controls the exposure set of functions from this mixin.  Without this annotation the functions would not be exposed in workers (and our codegen would actually fail the IDL here, because WorkerNavigator _is_ exposed in workers, and it implementing (in the "implements" sense) something that's not exposed there makes no sense.

> If yes, why are we excluding ServiceWorker?

We aren't.  ServiceWorkerGlobalScope has [Global=(Worker,ServiceWorker)], so "Worker" is a set of global types that includes ServiceWorkerGlobalScope.  It also includes DedicatedWorkerGlobalScope and SharedWorkerGlobalScope, for similar reasons.
Flags: needinfo?(bzbarsky)
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=24080812&repo=mozilla-inbound
Flags: needinfo?(bzbarsky)
Ugh.  I had missed this in the try output yesterday because of the tier3 mess.  :(  Thank you for backing me out.

What a crappy test.  It assumes that no property on Navigator will ever have the value 1!  Too bad we're running the test on a single-processor machine.
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/mozilla-central/rev/d2b13daf01ed
Status: ASSIGNED → RESOLVED
Closed: 10 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Sheppy: NavigatorConcurrentHardware is not an interface, please use the term 'mixin'.
Flags: needinfo?(eshepherd)
(In reply to Jean-Yves Perrier [:teoli] from comment #49)
> Sheppy: NavigatorConcurrentHardware is not an interface, please use the term
> 'mixin'.

Whoops, yes, of course. I'll fix that today.
Flags: needinfo?(eshepherd)
I will point out however, that I based my text on other bases of Navigator, including NavigatorID and WorkerNavigator, both of which also are described as interfaces. Presumably these need updating too then?
I've fixed the language here and tweaked phrasing a bit to work better with the "mixin" terminology.

I admit I'm still getting used to the word. I was brought up referring to these as "base classes," "base interfaces," or "foundation interfaces." Things like that. I'd never heard the word "mixin" until a few months ago. :)
The Fetch API was the first spec to use the terminology. Older specs and others are using... periphrases. I never saw 'base classes', 'base interfaces' or 'foundation interfaces' in a Web spec. 

And yes, there are wrong occurrences of 'interfaces' elsewhere in the MDN.
Whiteboard: [games:p1] → [games:p1][platform-rel-Games]
You need to log in before you can comment on or make changes to this bug.