Everything

php

Deprecate posix_times

php.internals

Daniil Gentili

2 years ago

Hi everyone,

I would like to submit an RFC (and related PR) to deprecate posix_times
for PHP 8.4, removing it for PHP 9.

There are multiple reasons to deprecate this function:

1)

The values which times(2) returns are expressed as the number of clock
ticks elapsed since the process was started: the frequency of the
aforementioned clock varies from platform to platform, and PHP does not
expose the sysconf(2) function, which is what should be used to fetch
the clock frequency, invoking `sysconf(_SC_CLK_TCK)`.

Thus, users have to deal with time values which **cannot be portably
converted to microseconds**: this has actually bitten us @ work, where I
recently found an ancient piece of code which incorrectly hardcoded the
(undocumented) clock frequency of 100hz used on (most) Linux platforms:
running the code on a Linux platform like ia64 [1], or any other POSIX
OS which uses a different clock frequency would have silently reported
an incorrect value, with no way to obtain the correct clock frequency
from PHP.

2)

Equivalent functionality is already provided by getrusage(), with a
*much higher resolution*:

*a.php:*

<?php

$t = microtime(true);

function slow2() {}
function slow() {
    slow2();
}

for ($x = 0; $x < 9000000; $x++) {
    slow();
}

$r = getrusage();
$stat = posix_times();

$res = [
    'utime_times' => $stat['utime'] * 10_000, // Assuming 100 hertz
clock on x86 linux
    'stime_times' => $stat['stime'] * 10_000, // Assuming 100 hertz
clock on x86 linux

    'utime_rusage' => $r['ru_utime.tv_usec'] + ($r['ru_utime.tv_sec'] *
1000_000),
    'stime_rusage' => $r['ru_stime.tv_usec'] + ($r['ru_stime.tv_sec'] *
1000_000),
];

var_dump($res);


*Result:*

array(4) {
  ["utime_times"]=>
  int(160000)
  ["stime_times"]=>
  int(0)
  ["utime_rusage"]=>
  int(163672)
  ["stime_rusage"]=>
  int(4994)
}


As you can see, with a system time of ~5 milliseconds, posix_times()
even returns 0, since the minimum duration that can be represented with
a 100 hertz clock (the default on most Linux platforms) is 10 milliseconds.


Waiting for feedback, kind regards,

Daniil Gentili.


[1]:
https://github.com/bminor/glibc/blob/69239bd7a216007692470aa9d5f3658024638742/sysdeps/unix/sysv/linux/ia64/getclktck.c

Christian Schneider

2 years ago

Am 17.10.2023 um 19:39 schrieb Daniil Gentili <daniil@daniil.it>:

> I would like to submit an RFC (and related PR) to deprecate posix_times for PHP 8.4, removing it for PHP 9.

My question here would be: Is there sufficient reason to remove this function and introduce a BC break, i.e. is the implementation code hard to maintain or does the function cause security issues?

Unless that is the case I'd leave it unchanged and just add a note to the documentation that people might want to consider using getrusage() instead.

Regards,
- Chris

Daniil Gentili

2 years ago

Hi,

As listed in my original thread, I think that offering posix_times
without offering sysconf would be like offering a version of the
microtime function, which returns a value divided by a *undocumented and
unexposed divider that changes in each patch version of PHP*.

The main reason I would like to see this deprecated is not the fact that
it's returning a less precise value compared to getrusage, but rather
because it's returning a value that cannot be interpreted in any way
from pure PHP.


Regards,

Daniil Gentili.

On 10/18/23 09:00, Christian Schneider wrote:

Tim Düsterhus

2 years ago

Hi

On 10/18/23 09:00, Christian Schneider wrote:

> My question here would be: Is there sufficient reason to remove this function and introduce a BC break, i.e. is the implementation code hard to maintain or does the function cause security issues?
>
> Unless that is the case I'd leave it unchanged and just add a note to the documentation that people might want to consider using getrusage() instead.

If a function cannot be safely or correctly used at all, then it
shouldn't exist, especially if there are working alternatives [1].

Expecting users to find a warning in the docs to learn that they should
"never" use a function is not good developer experience.

Best regards
Tim Düsterhus

Tim Düsterhus

2 years ago

Hi

On 10/17/23 19:39, Daniil Gentili wrote:

> I would like to submit an RFC (and related PR) to deprecate posix_times
> for PHP 8.4, removing it for PHP 9.
>
> […]
>
> Waiting for feedback, kind regards,

This deprecation appears to be sufficiently simple that it can likely be
included in the bulk deprecation RFC for PHP 8.4:

https://wiki.php.net/rfc/deprecations_php_8_4

best regards
Tim Düsterhus

Daniil Gentili

2 years ago

Hi,

> This deprecation appears to be sufficiently simple that it can likely be included in the bulk deprecation RFC for PHP 8.4:
>
> https://wiki.php.net/rfc/deprecations_php_8_4

That would be super nice, who should I ping before editing the page?

Regards,
Daniil Gentili

Tim Düsterhus

2 years ago

Hi

On 11/3/23 00:14, daniil@daniil.it wrote:

>> This deprecation appears to be sufficiently simple that it can likely be included in the bulk deprecation RFC for PHP 8.4:
>>
>> https://wiki.php.net/rfc/deprecations_php_8_4
>
>
> That would be super nice, who should I ping before editing the page?

I'd say you can just go ahead. Revision history is tracked.

Just make sure to refer to this ML thread somewhere and perhaps add a
short header line to indicate that you authored that specific section in
case anyone has questions. That's what I did for the stuff I added to
the 8.3 deprecation RFC and also the 'unserialize()'s 'S' tag' stub.

Best regards
Tim Düsterhus

Jakub Zelenka

2 years ago

Hi,

On Tue, Oct 17, 2023 at 6:39 PM Daniil Gentili <daniil@daniil.it> wrote:

> Hi everyone,
>
> I would like to submit an RFC (and related PR) to deprecate posix_times
> for PHP 8.4, removing it for PHP 9.
>

The times() is defined by POSIX and I don't see a reason why it couldn't be
exposed even though there is a better alternative. I think posix extension
should just expose posix defined functions and it is thus a low level
extension.

>
> There are multiple reasons to deprecate this function:
>
> 1)
>
> The values which times(2) returns are expressed as the number of clock
> ticks elapsed since the process was started: the frequency of the
> aforementioned clock varies from platform to platform, and PHP does not
> expose the sysconf(2) function, which is what should be used to fetch
> the clock frequency, invoking `sysconf(_SC_CLK_TCK)`.
>
>

I think the only thing that is needed is to expose _SC_CLK_TCK as
POSIX_SC_CLK_TCK and add a note to docs about it in the same way how this
is documented for the C counterpart.

Cheers

Jakub

Daniil Gentili

2 years ago

Hi, I would like to echo the sentiment expressed by Tim earlier in another subthread:

>If a function cannot be safely or correctly used at all, then it shouldn't exist, especially if there are working alternatives [1].
>
>Expecting users to find a warning in the docs to learn that they should "never" use a function is not good developer experience.

I am aware that the function is defined by the POSIX standard but

1) Not all POSIX functions are currently exposed by the POSIX extension, sysconf being one of the missing ones (with IMO no good reason to add it)
2) I am not aware of the historical reasons that lead to the addition of a duplicated, less capable function to the POSIX standard (I'm super duper sure there's some reason related to an implementation detail of some 30+ year-old platform :), but PHP isn't a POSIX-compliant OS, it is not PHP's task to expose the entirety of the POSIX standard functions: PHP's task is to provide a simple to use language that also exposes some *useful* functions from the POSIX standard, and posix_times does not fit the definition, in my opinion.

Regards,
Daniil Gentili.

Jakub Zelenka

2 years ago

On Thu, Nov 2, 2023 at 11:11 PM <daniil@daniil.it> wrote:

> 1) Not all POSIX functions are currently exposed by the POSIX extension,
> sysconf being one of the missing ones (with IMO no good reason to add it)
>

It is exposed:
https://github.com/php/php-src/blob/66e2aa725564c2221ace7a26628afe3957f28237/ext/posix/posix.c#L1232-L1241

Also we already define some SC constants so the only that is needed for
this is just adding a constant which is a very easy thing to do.

>
> 2) I am not aware of the historical reasons that lead to the addition of a
> duplicated, less capable function to the POSIX standard (I'm super duper
> sure there's some reason related to an implementation detail of some 30+
> year-old platform :), but PHP isn't a POSIX-compliant OS, it is not PHP's
> task to expose the entirety of the POSIX standard functions: PHP's task is
> to provide a simple to use language that also exposes some *useful*
> functions from the POSIX standard, and posix_times does not fit the
> definition, in my opinion.
>

But this is a posix extension which sole purpose is to expose POSIX
functions. The extension is available only on POSIX systems. We are not
talking about function in standard extension. In such case I would share
your sentiment but this is a completely different matter.

Cheers

Jakub

Daniil Gentili

2 years ago

Hi,

> It is exposed:
> https://github.com/php/php-src/blob/66e2aa725564c2221ace7a26628afe3957f28237/ext/posix/posix.c#L1232-L1241
>

Welp completely missed that, my bad, it was added in php 8.3, that's why I couldn't find it in the docs :)

Respectfully, I will still be editing the deprecations RFC anyway to see if the voting passes.

Kind regards,
Daniil Gentili.

Jakub Zelenka

2 years ago

On Fri, Nov 3, 2023 at 6:49 PM <daniil@daniil.it> wrote: added in php 8.3,
that's why I couldn't find it in the docs :)

>
> Respectfully, I will still be editing the deprecations RFC anyway to see
> if the voting passes.
>
>

Hmm, didn't you say this previously:

> The main reason I would like to see this deprecated is not the fact that
> it's returning a less precise value compared to getrusage, but rather
> because it's returning a value that cannot be interpreted in any way
> from pure PHP.

So if the constant is added, which is trivial, then the only reason left is
that it's returning slightly less precise value and maybe slightly harder
way to interpret it, right? If so, then I don't think this is enough to
break BC. It would be much better done using documentation where we should
also add a note about the new constant.

We should really realise that any deprecation and its subsequent change in
major version can have impact on users and adoption of new PHP versions.
There should always be a very good reason to do deprecate something.

Cheers

Jakub

Tim Düsterhus

2 years ago

Hi

On 11/3/23 21:51, Jakub Zelenka wrote:

>> The main reason I would like to see this deprecated is not the fact that
>> it's returning a less precise value compared to getrusage, but rather
>> because it's returning a value that cannot be interpreted in any way
>> from pure PHP.
>
> So if the constant is added, which is trivial, then the only reason left is
> that it's returning slightly less precise value and maybe slightly harder
> way to interpret it, right? If so, then I don't think this is enough to
> break BC. It would be much better done using documentation where we should
> also add a note about the new constant.

There is no real backwards-compatibility break, because the function was
not correctly usable in the first place. This means that anyone who
currently uses the function already has broken code.

An actual deprecation would make any existing user of the function aware
of the fact that their code is broken. Just adding the necessary
constant will not unbreak existing broken code, it will just enable
users to use the function correctly going forward, *if* they read the
documentation. Users relying on existing tutorials or StackOverflow
answers will continue to have broken code.

Best regards
Tim Düsterhus

Jakub Zelenka

2 years ago

On Sat, Nov 4, 2023 at 11:40 AM Tim Düsterhus <tim@bastelstu.be> wrote:

> Hi
>
> On 11/3/23 21:51, Jakub Zelenka wrote:
> >> The main reason I would like to see this deprecated is not the fact that
> >> it's returning a less precise value compared to getrusage, but rather
> >> because it's returning a value that cannot be interpreted in any way
> >> from pure PHP.
> >
> > So if the constant is added, which is trivial, then the only reason left
> is
> > that it's returning slightly less precise value and maybe slightly harder
> > way to interpret it, right? If so, then I don't think this is enough to
> > break BC. It would be much better done using documentation where we
> should
> > also add a note about the new constant.
>
> There is no real backwards-compatibility break, because the function was
> not correctly usable in the first place. This means that anyone who
> currently uses the function already has broken code.
>
>

Well we don't really know how users use that function. If someone takes a
look, they can see that the time is not in milliseconds. It basically just
uses different unit so it might still be useful for comparison of times if
for example it is logged somewhere and then compared or even used as a
measure (e.g. for some dashboards). So I certainly still see some valid use
cases here.

I have just quickly checked docs and it seems to me that it offers some
extra info compare to getrusage. Specifically it differentiate between
times for current process only and times for process and children. That
might be potentially useful for someone using pcntl_fork and similar. In
addition it returns total number of ticks from reboot which might have some
use cases as well. I'm not sure how this could be replaced by just
getrusage. Do I miss anything?

Regards

Jakub