Skip to content

Min/max optimization for int/floats #11194

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jun 2, 2023
+218 −10
Merged

Min/max optimization for int/floats #11194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4ea2ccd
ext/standard/array.c: Optimize min/max functions for int/float
Girgias May 6, 2023
a08f5a3
Use zend_compare
Girgias May 6, 2023
fb609dc
Cleanup + max()
Girgias May 6, 2023
b7a11f8
Fix test + int losing precision with float fix
Girgias May 7, 2023
1dff2be
Elide UB
Girgias May 8, 2023
6edcc6b
Reformat tests
Girgias May 31, 2023
2a8940d
[skip ci] add comments
Girgias May 31, 2023
af45e84
[skip ci] Comments fix
Girgias Jun 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 96 additions & 10 deletions ext/standard/array.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1233,15 +1233,58 @@ PHP_FUNCTION(min)
}
} else {
/* mixed min ( mixed $value1 , mixed $value2 [, mixed $value3... ] ) */
zval *min, result;
zval *min;
uint32_t i;

min = &args[0];
zend_long min_lval;
double min_dval;

for (i = 1; i < argc; i++) {
is_smaller_function(&result, &args[i], min);
if (Z_TYPE(result) == IS_TRUE) {
min = &args[i];
if (Z_TYPE_P(min) == IS_LONG) {
min_lval = Z_LVAL_P(min);

for (i = 1; i < argc; i++) {
if (EXPECTED(Z_TYPE(args[i]) == IS_LONG)) {
if (min_lval > Z_LVAL(args[i])) {
min_lval = Z_LVAL(args[i]);
min = &args[i];
}
} else if (Z_TYPE(args[i]) == IS_DOUBLE && (zend_dval_to_lval((double) min_lval) == min_lval)) {
/* if min_lval can be exactly represented as a double, go to double dedicated code */
min_dval = (double) min_lval;
goto double_compare;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really safe? As currently written, when a double is observed, all comparions are done using double, which might compare different longs as the same value: https://3v4l.org/EgniH/rfc#vgit.master_jit

The original issue is primary because of tracing overhead, IMHO microoptimization for <10% is not much helpful vs. the much worse code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mvorisek Note that the zend_compare function (which was previously always used) also casts to a double. I created a test case for min: https://3v4l.org/ZsosL and this behaves the same with this patch and without this patch. It is indeed true that there is a loss of precision when comparing very big doubles, but this is already the case now. There is afaik no change in behaviour.

Or did you test this patch and found an issue? If so, please provide the reproducer.

The original issue is primary because of tracing overhead, IMHO microoptimization for <10% is not much helpful vs. the much worse code.

It turns out that this actually improves the diff real-world workload from the original issue report a lot. The benchmarks I did were only very isolated but it's important to look at the real-world workload. Also there are already a couple of functions which have a fast-path for longs & doubles iirc.

Copy link
Contributor

@mvorisek mvorisek May 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/php/php-src/blob/24771fb08b/ext/standard/array.c#L1267 will produce a bad result when there are doubles between large integers, I did not test it, but originally there was no jump to double cmp only

actually improves the diff real-world workload

the issue (in the diff lib) will be still present, at the main slowdown is due tracing overhead, not due slow php-src

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/php/php-src/blob/24771fb08b/ext/standard/array.c#L1267 will produce a bad result when there are doubles between large integers, I did not tested it, but originally there was no jump to double cmp only

I see what you mean. Here's a test with a behaviour difference:

<?php

var_dump(PHP_INT_MAX*3);
var_dump(min(PHP_INT_MAX*2, PHP_INT_MAX, PHP_INT_MAX-1));
// int(9223372036854775806) previously
// int(9223372036854775807) with this patch

This can probably be worked around. I would prefer a solution that doesn't restart the loop to prevent a performance impact. I also checked with my generic zend_compare improvement and it doesn't suffer from that problem, so we still have that as a possibility.

the issue (in the diff lib) will be still present, at the main slowdown is due tracing overhead, not due slow php-src

Yes, but I didn't say it fully got rid of all the overhead, I said this is a big improvement.

} else {
goto generic_compare;
}
}

RETURN_LONG(min_lval);
} else if (Z_TYPE_P(min) == IS_DOUBLE) {
min_dval = Z_DVAL_P(min);

for (i = 1; i < argc; i++) {
if (EXPECTED(Z_TYPE(args[i]) == IS_DOUBLE)) {
double_compare:
if (min_dval > Z_DVAL(args[i])) {
min_dval = Z_DVAL(args[i]);
min = &args[i];
}
} else if (Z_TYPE(args[i]) == IS_LONG && (zend_dval_to_lval((double) Z_LVAL(args[i])) == Z_LVAL(args[i]))) {
/* if the value can be exactly represented as a double, use double dedicated code otherwise generic */
if (min_dval > (double)Z_LVAL(args[i])) {
min_dval = (double)Z_LVAL(args[i]);
min = &args[i];
}
} else {
goto generic_compare;
}
}
} else {
for (i = 1; i < argc; i++) {
generic_compare:
if (zend_compare(&args[i], min) < 0) {
min = &args[i];
}
}
}

Expand Down Expand Up @@ -1279,15 +1322,58 @@ PHP_FUNCTION(max)
}
} else {
/* mixed max ( mixed $value1 , mixed $value2 [, mixed $value3... ] ) */
zval *max, result;
zval *max;
uint32_t i;

max = &args[0];
zend_long max_lval;
double max_dval;

for (i = 1; i < argc; i++) {
is_smaller_or_equal_function(&result, &args[i], max);
if (Z_TYPE(result) == IS_FALSE) {
max = &args[i];
if (Z_TYPE_P(max) == IS_LONG) {
max_lval = Z_LVAL_P(max);

for (i = 1; i < argc; i++) {
if (EXPECTED(Z_TYPE(args[i]) == IS_LONG)) {
if (max_lval < Z_LVAL(args[i])) {
max_lval = Z_LVAL(args[i]);
max = &args[i];
}
} else if (Z_TYPE(args[i]) == IS_DOUBLE && (zend_dval_to_lval((double) max_lval) == max_lval)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to add some comment here as it took me a little bit to figure out what zend_dval_to_lval((double) max_lval) == max_lval is for. IIUC it's for long to double overflow check, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it's to check that when an int converted to float it can be represented exactly as a float or it loses precision (so for integers higher than 52 bits IIRC)

/* if max_lval can be exactly represented as a double, go to double dedicated code */
max_dval = (double) max_lval;
goto double_compare;
} else {
goto generic_compare;
}
}

RETURN_LONG(max_lval);
} else if (Z_TYPE_P(max) == IS_DOUBLE) {
max_dval = Z_DVAL_P(max);

for (i = 1; i < argc; i++) {
if (EXPECTED(Z_TYPE(args[i]) == IS_DOUBLE)) {
double_compare:
if (max_dval < Z_DVAL(args[i])) {
max_dval = Z_DVAL(args[i]);
max = &args[i];
}
} else if (Z_TYPE(args[i]) == IS_LONG && (zend_dval_to_lval((double) Z_LVAL(args[i])) == Z_LVAL(args[i]))) {
/* if the value can be exactly represented as a double, use double dedicated code otherwise generic */
if (max_dval < (double)Z_LVAL(args[i])) {
max_dval = (double)Z_LVAL(args[i]);
max = &args[i];
}
} else {
goto generic_compare;
}
}
} else {
for (i = 1; i < argc; i++) {
generic_compare:
if (zend_compare(&args[i], max) > 0) {
max = &args[i];
}
}
}

Expand Down
61 changes: 61 additions & 0 deletions ext/standard/tests/array/max_int_float_optimisation.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
--TEST--
Check max() optimisation for int and float types
--SKIPIF--
<?php if (PHP_INT_SIZE != 8) die("skip this test is for 64bit platform only"); ?>
--FILE--
<?php

echo "Start as int optimisation:\n";
var_dump(max(10, 5, 3, 2));
var_dump(max(2, 3, 5, 10));
var_dump(max(10, 5, 3.5, 2));
var_dump(max(2, 3.5, 5, 10));
var_dump(max(10, 5, "3", 2));
var_dump(max(2, "3", 5, 10));
var_dump(max(2, 3, "15", 10));
echo "Check that int not representable as float works:\n";
var_dump(max(PHP_INT_MIN+1, PHP_INT_MIN, PHP_INT_MIN*2));
var_dump(max(PHP_INT_MAX-1, PHP_INT_MAX, PHP_INT_MAX*2));
// Has INF
var_dump(max(PHP_INT_MAX-1, PHP_INT_MAX, PHP_INT_MAX**20));

echo "Start as float optimisation:\n";
var_dump(max(10.5, 5.5, 3.5, 2.5));
var_dump(max(2.5, 3.5, 5.5, 10.5));
var_dump(max(10.5, 5.5, 3, 2.5));
var_dump(max(2.5, 3, 5.5, 10.5));
var_dump(max(10.5, 5.5, "3.5", 2.5));
var_dump(max(2.5, "3.5", 5.5, 10.5));
var_dump(max(2.5, 3.5, "15.5", 10.5));
echo "Check that int not representable as float works:\n";
var_dump(max(PHP_INT_MIN*2, PHP_INT_MIN, PHP_INT_MIN+1));
var_dump(max(PHP_INT_MAX*2, PHP_INT_MAX, PHP_INT_MAX-1));
// Has INF
var_dump(max(PHP_INT_MAX**20, PHP_INT_MAX, PHP_INT_MAX-1));

?>
--EXPECT--
Start as int optimisation:
int(10)
int(10)
int(10)
int(10)
int(10)
int(10)
string(2) "15"
Check that int not representable as float works:
int(-9223372036854775807)
float(1.8446744073709552E+19)
float(INF)
Start as float optimisation:
float(10.5)
float(10.5)
float(10.5)
float(10.5)
float(10.5)
float(10.5)
string(4) "15.5"
Check that int not representable as float works:
int(-9223372036854775807)
float(1.8446744073709552E+19)
float(INF)
61 changes: 61 additions & 0 deletions ext/standard/tests/array/min_int_float_optimisation.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
--TEST--
Check min() optimisation for int and float types
--SKIPIF--
<?php if (PHP_INT_SIZE != 8) die("skip this test is for 64bit platform only"); ?>
--FILE--
<?php

echo "Start as int optimisation:\n";
var_dump(min(10, 5, 3, 2));
var_dump(min(2, 3, 5, 10));
var_dump(min(10, 5, 3.5, 2));
var_dump(min(2, 3.5, 5, 10));
var_dump(min(10, 5, "3", 2));
var_dump(min(2, "3", 5, 10));
var_dump(min(2, 3, "1", 10));
echo "Check that int not representable as float works:\n";
var_dump(min(PHP_INT_MAX-1, PHP_INT_MAX, PHP_INT_MAX*2));
var_dump(min(PHP_INT_MIN+1, PHP_INT_MIN, PHP_INT_MIN*2));
// Has INF
var_dump(min(PHP_INT_MAX-1, PHP_INT_MAX, PHP_INT_MAX**20));

echo "Start as float optimisation:\n";
var_dump(min(10.5, 5.5, 3.5, 2.5));
var_dump(min(2.5, 3.5, 5.5, 10.5));
var_dump(min(10.5, 5.5, 3, 2.5));
var_dump(min(2.5, 3, 5.5, 10.5));
var_dump(min(10.5, 5.5, "3.5", 2.5));
var_dump(min(2.5, "3.5", 5.5, 10.5));
var_dump(min(2.5, 3.5, "1.5", 10.5));
echo "Check that int not representable as float works:\n";
var_dump(min(PHP_INT_MAX*2, PHP_INT_MAX, PHP_INT_MAX-1));
var_dump(min(PHP_INT_MIN*2, PHP_INT_MIN, PHP_INT_MIN+1));
// Has INF
var_dump(min(PHP_INT_MAX**20, PHP_INT_MAX, PHP_INT_MAX-1));

?>
--EXPECT--
Start as int optimisation:
int(2)
int(2)
int(2)
int(2)
int(2)
int(2)
string(1) "1"
Check that int not representable as float works:
int(9223372036854775806)
float(-1.8446744073709552E+19)
int(9223372036854775806)
Start as float optimisation:
float(2.5)
float(2.5)
float(2.5)
float(2.5)
float(2.5)
float(2.5)
string(3) "1.5"
Check that int not representable as float works:
int(9223372036854775806)
float(-1.8446744073709552E+19)
int(9223372036854775806)