ext/pcntl: pcntl_getqos_class/pcntl_setqos_class addition. #13945

devnexen · 2024-04-11T20:24:23Z

Introducting macOs Quality Of Service through those two calls. on macOs arm64/M*, there is no concept of individual cores, thus the old thread policy for cpu affinity does not work here. Instead, the user can apply to the current process the level of
performance/energy consumption they wish from the highest
QOS_CLASS_USER_INTERACTIVE to QOS_CLASS_BACKGROUND.

Girgias · 2024-04-14T11:05:30Z

ext/pcntl/pcntl.c

+		RETURN_THROWS();
+	}
+
+	RETURN_TRUE;


Why not just return NULL/void?

to give a success status at least ?

Is it really that useful if the function can only ever return true? I know a lot of other functions in pcntl only return true nowadays, but that's because they used to return false for ZPP failures or cases that have been promoted to Errors.

Girgias · 2024-04-14T11:06:17Z

ext/pcntl/pcntl.c

+	if (UNEXPECTED(pthread_get_qos_class_np(pthread_self(), &qos_class, NULL) != 0))
+	{
+		// unlikely unless an external tool set the QOS class with a wrong value
+		PCNTL_G(last_error) = errno;
+		zend_value_error("invalid QOS class %u", qos_class);
+		RETURN_THROWS();
+	}


How can this happen? Also this doesn't look like a ValueError as no user input is taken.

Girgias · 2024-04-14T11:07:19Z

ext/pcntl/pcntl.c

+	if (pthread_set_qos_class_self_np((qos_class_t)qos_class, 0) != 0)
+	{
+		PCNTL_G(last_error) = errno;
+		zend_argument_value_error(1, "must be between QOS_CLASS_USER_INTERACTIVE and QOS_CLASS_BLACKGROUND");


Is that flag a bitmask? Or just individual settings, in the latter case I'd list them all rather than making the user guess the "underlying" int value of the constants.

individual. also the list had not been updated since a long time (and no reason to do so anyway) so yes .. I ll update.

Girgias

Last wording nit, but looks good to me otherwise :)

Girgias · 2024-04-14T12:13:01Z

ext/pcntl/pcntl.c

+	if (pthread_set_qos_class_self_np((qos_class_t)qos_class, 0) != 0)
+	{
+		PCNTL_G(last_error) = errno;
+		zend_argument_value_error(1, "must be between QOS_CLASS_USER_INTERACTIVE, QOS_CLASS_USER_INITIATED, QOS_CLASS_DEFAULT, QOS_CLASS_UTILITY and QOS_CLASS_BACKGROUND");


Suggested change

zend_argument_value_error(1, "must be between QOS_CLASS_USER_INTERACTIVE, QOS_CLASS_USER_INITIATED, QOS_CLASS_DEFAULT, QOS_CLASS_UTILITY and QOS_CLASS_BACKGROUND");

zend_argument_value_error(1, "must be one of QOS_CLASS_USER_INTERACTIVE, QOS_CLASS_USER_INITIATED, QOS_CLASS_DEFAULT, QOS_CLASS_UTILITY, or QOS_CLASS_BACKGROUND");

nielsdos · 2024-04-14T12:48:17Z

I'm gonna be "that guy", and ask: should we start introducing enums instead of constants for new functions? I see a bunch of QOS constants being added here, but using enums instead would provide better type safety, and an improved DX because the function "documents itself" (i.e. the IDE and SA tools can help you better).
Using enums isn't always viable of course if it's just adding more constants of a kind that already existed, but the QOS ones look entirely new.

Girgias · 2024-04-14T12:56:51Z

Actually, that's a good point :D

devnexen · 2024-04-14T12:58:31Z

You want something like

enum QosClass {
     case UserInitiated;
}

like zig like to do ?
Why not generalising for other (e.g. pgsql has a bunch of constants which are enums) for php 9 ?

nielsdos · 2024-04-14T13:03:16Z

Indeed something like that.

Why not generalising for other (e.g. pgsql has a bunch of constants which are enums) for php 9 ?

That could be PHP9 material.
The point I'm trying to make is that for new code we could use enums and keep the constants for old code. Then in a future PHP version convert the old constants to enums. We could even have a transition period in the future where we already introduce the enum but still allow the constant to be passed. Something like function foo(int|MyEnum $option).

devnexen · 2024-04-14T13:21:31Z

Ok I ll it a go a bit later.

nielsdos

I like this API much better, some remarks.

nielsdos · 2024-04-14T16:21:56Z

ext/pcntl/pcntl.c

@@ -53,6 +53,7 @@ typedef cpuset_t cpu_set_t;

 #if defined(HAVE_PTHREAD_SET_QOS_CLASS_SELF_NP)
 #include <pthread/qos.h>
+PHPAPI zend_class_entry *QosClass_ce;


Why is this exposed via PHPAPI? Do we expect other extensions to reuse this enum?

I do not think no :)

nielsdos · 2024-04-14T16:23:20Z

ext/pcntl/pcntl.c

 	if (pthread_set_qos_class_self_np((qos_class_t)qos_class, 0) != 0)
 	{
 		PCNTL_G(last_error) = errno;
-		zend_argument_value_error(1, "must be one of QOS_CLASS_USER_INTERACTIVE, QOS_CLASS_USER_INITIATED, QOS_CLASS_DEFAULT, QOS_CLASS_UTILITY and QOS_CLASS_BACKGROUND");
+		zend_argument_value_error(1, "must be one of QosClass enum entries : ::UserInteractive, ::UserInitiated, ::Default, ::Utility and ::Background");


Suggested change

zend_argument_value_error(1, "must be one of QosClass enum entries : ::UserInteractive, ::UserInitiated, ::Default, ::Utility and ::Background");

zend_argument_value_error(1, "must be one of QosClass enum entries : ::UserInteractive, ::UserInitiated, ::Default, ::Utility or ::Background");

nielsdos · 2024-04-14T16:23:54Z

ext/pcntl/php_pcntl.h

@@ -58,4 +58,6 @@ ZEND_TSRMLS_CACHE_EXTERN()
 ZEND_EXTERN_MODULE_GLOBALS(pcntl)
 #define PCNTL_G(v) ZEND_MODULE_GLOBALS_ACCESSOR(pcntl, v)

+extern PHPAPI zend_class_entry *QosClass_ce;


nielsdos · 2024-04-14T16:25:46Z

ext/pcntl/pcntl.c

+		RETURN_THROWS();
+	}
+
+	ZVAL_COPY_OR_DUP(return_value, zend_enum_fetch_case_value(qos_obj));


zend_enum_fetch_case_value returns the backed value, but you want the object instead.

Introducting macOs Quality Of Service through those two calls. on macOs arm64/M*, there is no concept of individual cores, thus the old thread policy for cpu affinity does not work here. Instead, the user can apply to the current process the level of performance/energy consumption they wish from the highest QOS_CLASS_USER_INTERACTIVE to QOS_CLASS_BACKGROUND.

Girgias · 2024-04-14T21:09:44Z

ext/pcntl/pcntl.stub.php

+enum QosClass: int
+{
+	case UserInteractive = 0x21;
+	case UserInitiated = 0x19;
+	case Default = 0x15;
+	case Utility = 0x11;
+	case Background = 0x09;
+	case Unspecified = 0x00;
+}


I don't think it should be a backed enum, exposing those values to userland is not really helpful IMHO. Also what is this Unspecified case about?

Girgias · 2024-04-14T21:10:12Z

ext/pcntl/pcntl.c

+		PCNTL_G(last_error) = errno;
+		zend_argument_value_error(1, "must be one of QosClass enum entries : ::UserInteractive, ::UserInitiated, ::Default, ::Utility or ::Background");
+		RETURN_THROWS();


This should never happen if we are requiring an enum.

Girgias

Makes sense to me

nielsdos · 2024-04-15T15:32:24Z

ext/pcntl/pcntl.c

+	qos_class_t qos_class = QOS_CLASS_DEFAULT;
+	zend_string *entry = Z_STR_P(zend_enum_fetch_case_name(Z_OBJ_P(qos_obj)));
+
+	if (zend_string_equals_cstr(entry, "UserInteractive", sizeof("UserInteractive") - 1)) {


Please use zend_string_equals_literal for these instead, it's less error-prone.

nielsdos · 2024-04-15T15:35:10Z

ext/pcntl/pcntl.c

+
+static zend_object *qos_lval_to_zval(qos_class_t qos_class)
+{
+	const char *entryname;


If macOS ever introduces more qos types, then the fact this is uninitialized but used below will be UB

Despite apple has very little incentive to add more level of service, default is handled below.

Sorry, you are right. I missed the default.

nielsdos

LGTM, thanks!

TimWolla · 2024-07-09T18:18:04Z

ext/pcntl/pcntl.stub.php

@@ -1003,3 +1003,17 @@ function pcntl_setcpuaffinity(?int $process_id = null, array $cpu_ids = []): boo
 #ifdef HAVE_SCHED_GETCPU
 function pcntl_getcpu(): int {}
 #endif
+
+#ifdef HAVE_PTHREAD_SET_QOS_CLASS_SELF_NP
+enum QosClass


This should be named Pcntl\QosClass as per https://wiki.php.net/rfc/namespaces_in_bundled_extensions.

github-actions bot added Category: Build System Extension: pcntl labels Apr 11, 2024

devnexen force-pushed the pcntl_thread_qos_macos branch 2 times, most recently from a256774 to bdab6c1 Compare April 11, 2024 21:31

devnexen marked this pull request as ready for review April 11, 2024 21:59

devnexen requested a review from kocsismate as a code owner April 11, 2024 21:59

Girgias reviewed Apr 14, 2024

View reviewed changes

devnexen force-pushed the pcntl_thread_qos_macos branch from bdab6c1 to 1ec8a4a Compare April 14, 2024 11:25

Girgias approved these changes Apr 14, 2024

View reviewed changes

devnexen force-pushed the pcntl_thread_qos_macos branch from 1ec8a4a to 376bdd9 Compare April 14, 2024 12:18

nielsdos requested changes Apr 14, 2024

View reviewed changes

devnexen force-pushed the pcntl_thread_qos_macos branch from 85df17c to 8078a9b Compare April 14, 2024 16:44

Girgias reviewed Apr 14, 2024

View reviewed changes

changes to undef enums types.

ab885d8

Girgias approved these changes Apr 15, 2024

View reviewed changes

nielsdos requested changes Apr 15, 2024

View reviewed changes

use zend_string_equals_literal

540c027

nielsdos approved these changes Apr 15, 2024

View reviewed changes

devnexen closed this in d407266 Apr 15, 2024

TimWolla reviewed Jul 9, 2024

View reviewed changes

jrfnl mentioned this pull request Oct 5, 2024

PHP 8.4 | NewFunctions: detect more new functions PHPCompatibility/PHPCompatibility#1783

Merged

	zend_argument_value_error(1, "must be between QOS_CLASS_USER_INTERACTIVE, QOS_CLASS_USER_INITIATED, QOS_CLASS_DEFAULT, QOS_CLASS_UTILITY and QOS_CLASS_BACKGROUND");
	zend_argument_value_error(1, "must be one of QOS_CLASS_USER_INTERACTIVE, QOS_CLASS_USER_INITIATED, QOS_CLASS_DEFAULT, QOS_CLASS_UTILITY, or QOS_CLASS_BACKGROUND");

ext/pcntl: pcntl_getqos_class/pcntl_setqos_class addition. #13945

ext/pcntl: pcntl_getqos_class/pcntl_setqos_class addition. #13945

Uh oh!

Conversation

devnexen commented Apr 11, 2024

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!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nielsdos commented Apr 14, 2024

Uh oh!

Girgias commented Apr 14, 2024

Uh oh!

devnexen commented Apr 14, 2024

Uh oh!

nielsdos commented Apr 14, 2024

Uh oh!

devnexen commented Apr 14, 2024

Uh oh!

nielsdos left a comment

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!

Girgias left a comment

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!

nielsdos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!