feat: add function num_cpus (formerly nproc)

[kesselb]

Returns the number of processors which are currently online.

The idea, to add a feature similar to nproc, comes from nextcloud/server#37921.

We use the number of processors to limit the parallel generation of thumbnails. Unfortunately, we forgot to implement a check if access to /proc/cpuinfo is allowed and /proc is not available for FreeBSD.

Our use case is probably not the best, but it is complicated to get the number of processors without shell_exec.

nproc is likely not a good name. The actual nproc implementation covers much more corner cases¹.

I was curious if I could add such a functionality with my limited c knowledge 🙈
If you like the idea and there is a chance, this patch could be accepted, I'm happy to spend more time on it.

Footnotes

1. https://github.com/coreutils/gnulib/blob/master/lib/nproc.c ↩

[devnexen]

Do you have an use case in mind ?

[kesselb]

Do you have an use case in mind ?

Thank you @devnexen 👍
I updated the pull request description.

[devnexen]

I see. I m not against personally, I ll leave the decision to Mate on this one.

[bukka]

This is not really good way to inform about errors or being unsupported by the platform. It should probably error somehow if there is an error (either warning or exception) and similarly inform user that the function is not support or even not expose it at all if it is not supported.

[kesselb]

This is not really good way to inform about errors or being unsupported by the platform.

I understand. Do you think we can keep the default 0 now that windows is also supported? I assume the implementation should work for most OSs.

either warning or exception

What exception would fit here?

even not expose it at all if it is not supported.

Similar to sys_getloadavg.
We would expose the function for every OS with _SC_NPROCESSORS_ONLN and for windows.
The disadvantage is that a function_exists call is necessary before using the function.

[iluuu1994]

Returning NULL might also be an option to allow cpu_cores() ?? 1 (or maybe implement it yourself for the given architecture). The signature will also make it more obvious that this can fail. A warning is hard to catch and handle (unless the return value is also distinguishable).

[kesselb]

Thanks 👍

NULL works for me and num_cpus() ?? 1 is very clean.

[kesselb]

Renamed the function to cpu_cores to avoid confusion with the nproc (because nproc is more advancend).

[iluuu1994]

Nit: We might want to incorporate num, cpu_cores sounds like it might provide more information, like cpu_num_cores. But admittedly it doesn't sound as nice.

[kesselb]

True. I found https://docs.rs/num_cpus/latest/num_cpus/ and thought it was a good name.

[kesselb]

Renamed the function to num_cpus so it's easier to guess that we just return a number.

[kocsismate]

My personal opinion: I would love to see convenient OO APIs like what ext/random has instead of creating functions scattered here and there. The way I can imagine this is adding a ReflectionServer (or something similar) class into ext/reflection. How does it sound to you?

[iluuu1994]

@kocsismate Unless there's some common configuration/parameter these functions would share, I don't see the point in hiding this function behind an object. A namespace is a different story, but I'm not ready for the bike-shedding 😜

[kocsismate]

@iluuu1994 I don't see it as "hiding", but grouping them together, so that the complete list of available functions for "server reflection" could be seen at once when someone starts to write (new ReflectionServer())->.

[iluuu1994]

@kocsismate Grouping is what namespaces are for. 🙂 (new \ReflectionServer())->numCores() is less readable than \Cpu\num_cores() (or whatever namespace) and requires an unnecessary allocation. ReflectionServer::numCores() would be slightly better, but I still don't see why this should be associated with a class. Autocomplete works fine for namespaces.

[bukka]

In terms of naming I agree that just single function is fine here (we don't really need OO API for this IMHO). The name num_cpus seems also fine to me.

[kesselb]

Hey 👋

Addressed the code review suggestions, had a brief look at Solaris/OpenBSD compatibility and rebased.

[bukka]

Just NIT: the preprocesor macros are not usually indented in our code so if you could remove indent for them, it would be great.

[kesselb]

Sure, they are much easier to spot when not indented.

[bukka]

@kocsismate Are you still have an objection against the naming? Just checking if we need an RFC for this basically?

@kesselb If there are no objections from @kocsismate, then it would be good to drop email to internals to see if there any objections in the next two weeks. If not, we could still get it to 8.3, otherwise the RFC would be required which would mean waiting for 8.4 due to discussion and voting period being both 2 weeks and feature freeze on July 18th.

[kocsismate]

Are you still have an objection against the naming? Just checking if we need an RFC for this basically?

To be honest, I'd be much happier if there was some structuring of the functionality which we add. I agree that the bike shedding can be extremely frustrating, but at least sometimes it improve things.

Unfortunately, I'm still pretty much obsessed with my ReflectionServer idea, probably because I like the OO API of PHP much more than the procedural one: I always refer to ext/random as the best example for adding a new API.

So the very least, it would make sense to think about the possible extensions of num_cpus? What kind of further info do we plan to provide about the environment in the future? If we want to stop here then ok, let's add a simple function no matter in which namespace. If we plan to add further related functionality then let's go for either a class (possibly with static methods) or with a separate namespace so that the choice is in line with our long term plans.

As far as I noticed, PHP is evolving towards more and more object oriented APIs (ext/random, resource to object migrations, stop adding dual APIs with the preference of OO), so I think this topic is worth having a discussion at least internally.