We have/require symfony/polyfill-php80 and symfony/polyfill-php81...

Is there much (any?) performance loss to swapping to using it now, for those who aren't on PHP 8.0+ already?

In T338103#8900961, @Reedy wrote:

We have/require symfony/polyfill-php80 and symfony/polyfill-php81...

Is there much (any?) performance loss to swapping to using it now, for those who aren't on PHP 8.0+ already?

Not just "much", but a massive lot unfortunately:

<?php

$num = 100000000;
$str = 'abcdefghijklmno';

$s = hrtime(true);
for ( $i = 0; $i < $num; $i++ ) {
    $res1 = ord( $str ) === 95;
}
$time = (hrtime( true ) - $s)/10**6;
echo "ORD: $time\n";

$s = hrtime(true);
for ( $i = 0; $i < $num; $i++ ) {
    $res3 = str_starts_with( $str, '_' );
}
$time = (hrtime( true ) - $s)/10**6;
echo "START: $time\n";

class SymfonyPhp80 {
	public static function str_starts_with2(string $haystack, string $needle): bool
    {
        return 0 === strncmp($haystack, $needle, \strlen($needle));
	}
}

function str_starts_with2(?string $haystack, ?string $needle): bool { return SymfonyPhp80::str_starts_with2($haystack ?? '', $needle ?? ''); }

$s = hrtime(true);
for ( $i = 0; $i < $num; $i++ ) {
    $res3 = str_starts_with2( $str, '_' );
}
$time = (hrtime( true ) - $s)/10**6;
echo "START SYM: $time\n";

$ php8.1 test.php 
ORD: 1573.309082
START: 1598.216216
START SYM: 6385.82479

(Also note that in this test run ord() is faster than str_starts_with())

Using the polyfill involves a userland function call, two null coalesces, a userland method call, and an underlying implementation that uses strncmp, strlen, and a comparison. This wouldn't matter in ordinary code, but since this code is (or at least was) extremely hot, any slowdown matters, particularly a 4x one.

I wonder if this method really is that hot—in the api.php-specific wall time flamegraph for May 8th, I get no hits at all for the method name, and ApiResult shows up as ~0.1%, which is so little that I have trouble finding it in the flamegraph. In the CPU time graph, ApiResult::applyTransformations shows up as responsible for ~0.4% of CPU time, but isMetadataKey is not visible there either.

I368cd3b526edeb247a6ebda7420897a51357407d appears to have been based on an XHGui profile, which tends to overstate the impact of small functions called relatively often.

The function is documented to accept integer (from array key index). The comment suggest that the current code (with ord()) works with integer (not documented on php.net), while str_starts_with() is not documented to do. It seems in that case it is converted to a string first, not sure. Maybe depends on strict mode.

So the question is, if this should be changed or not.

Probably should look at revisiting this...

If it is hot, we should treat it as such; I'm guessing @TK-999's comments probably still ring true, and are easily confirmed.

T411214: Deprecated: ord(): Providing a string that is not one byte long is deprecated. Use ord($str[0]) instead in /var/www/wiki/mediawiki/core/includes/Api/ApiResult.php on line 792 is creating reason to change this function, with a slightly scary comment that currently exists in it

		// Optimization: This is a very hot and highly optimized code path. Note that ord() only
		// considers the first character and also works with empty strings and integers.

As of PHP 8.5:

We can't pass an empty string to ord(), nor can we pass an int...

Nor does it only consider the first character... So that comment seems to be wrong (or at least, out of date, added in rMW4175998db26c: Micro-optimize ApiResult::isMetadataKey even more) on many cases...

Change #1212225 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/core@master] ApiResult: Fix "ord(): Providing a string that is not one byte long is deprecated."

https://gerrit.wikimedia.org/r/1212225

Yeah, I'm not sure why the method was determined to be extremely hot, it's possible the profile(r) we used back then was faulty. str_starts_with is more readable, does not emit deprecations, and from my quick tests, it seems even faster than ord, so...

Might be worth re-visiting the other ord() calls and any relevant guarding...

Change #1212225 merged by jenkins-bot:

[mediawiki/core@master] ApiResult: Fix "ord(): Providing a string that is not one byte long is deprecated."

https://gerrit.wikimedia.org/r/1212225

Change #1212283 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/core@REL1_43] ApiResult: Fix "ord(): Providing a string that is not one byte long is deprecated."

https://gerrit.wikimedia.org/r/1212283

Change #1212284 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/core@REL1_44] ApiResult: Fix "ord(): Providing a string that is not one byte long is deprecated."

https://gerrit.wikimedia.org/r/1212284

Change #1212285 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/core@REL1_45] ApiResult: Fix "ord(): Providing a string that is not one byte long is deprecated."

https://gerrit.wikimedia.org/r/1212285

Change #1212283 merged by jenkins-bot:

[mediawiki/core@REL1_43] ApiResult: Fix "ord(): Providing a string that is not one byte long is deprecated."

https://gerrit.wikimedia.org/r/1212283

Change #1212284 merged by jenkins-bot:

[mediawiki/core@REL1_44] ApiResult: Fix "ord(): Providing a string that is not one byte long is deprecated."

https://gerrit.wikimedia.org/r/1212284

Change #1212285 merged by jenkins-bot:

[mediawiki/core@REL1_45] ApiResult: Fix "ord(): Providing a string that is not one byte long is deprecated."

https://gerrit.wikimedia.org/r/1212285