From 2c5ed50d5c3efdbd2f24110911bc142866e4eca4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= <tim@tideways-gmbh.com>
Date: Mon, 17 Jun 2024 17:07:50 +0200
Subject: [PATCH] zend_compile: Add support for `%d` to `sprintf()`
 optimization (#14561)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* zend_compile: Rename `string_placeholder_count` to `placeholder_count` in `zend_compile_func_sprintf()`

This is intended to make the diff of a follow-up commit smaller.

* zend_compile: Add support for `%d` to `sprintf()` optimization

This extends the existing `sprintf()` optimization by support for the `%d`
placeholder, which effectively equivalent to an `(int)` cast followed by a
`(string)` cast.

For a synthetic test using:

    <?php

    $a = 'foo';
    $b = 42;

    for ($i = 0; $i < 100_000_000; $i++) {
        sprintf("%s-%d", $a, $b);
    }

This optimization yields a 1.3× performance improvement:

    $ hyperfine 'sapi/cli/php -d zend_extension=php-src/modules/opcache.so -d opcache.enable_cli=1 test.php' \
          '/tmp/unoptimized -d zend_extension=php-src/modules/opcache.so -d opcache.enable_cli=1 test.php'
    Benchmark 1: sapi/cli/php -d zend_extension=php-src/modules/opcache.so -d opcache.enable_cli=1 test.php
      Time (mean ± σ):      3.296 s ±  0.094 s    [User: 3.287 s, System: 0.005 s]
      Range (min … max):    3.213 s …  3.527 s    10 runs

    Benchmark 2: /tmp/unoptimized -d zend_extension=php-src/modules/opcache.so -d opcache.enable_cli=1 test.php
      Time (mean ± σ):      4.300 s ±  0.025 s    [User: 4.290 s, System: 0.007 s]
      Range (min … max):    4.266 s …  4.334 s    10 runs

    Summary
      sapi/cli/php -d zend_extension=php-src/modules/opcache.so -d opcache.enable_cli=1 test.php ran
        1.30 ± 0.04 times faster than /tmp/unoptimized -d zend_extension=php-src/modules/opcache.so -d opcache.enable_cli=1 test.php

* Fix sprintf_rope_optimization_003.phpt test expecation for 32-bit integers

* zend_compile: Indent switch-case labels in zend_compile_func_sprintf()

* Add GMP test to sprintf() rope optimization

* Add `%s` test case to sprintf() GMP test
---
 Zend/zend_compile.c                           |  67 +++++----
 .../sprintf_rope_optimization_001.phpt        |   6 +
 .../sprintf_rope_optimization_003.phpt        | 134 ++++++++++++++++++
 .../sprintf_rope_optimization_004.phpt        |  21 +++
 4 files changed, 198 insertions(+), 30 deletions(-)
 create mode 100644 ext/standard/tests/strings/sprintf_rope_optimization_003.phpt
 create mode 100644 ext/standard/tests/strings/sprintf_rope_optimization_004.phpt

diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c
index ed1eb782f5f..2bc4d670331 100644
--- a/Zend/zend_compile.c
+++ b/Zend/zend_compile.c
@@ -4739,9 +4739,9 @@ static zend_result zend_compile_func_sprintf(znode *result, zend_ast_list *args)
 
 	char *p;
 	char *end;
-	uint32_t string_placeholder_count;
+	uint32_t placeholder_count;
 	
-	string_placeholder_count = 0;
+	placeholder_count = 0;
 	p = Z_STRVAL_P(format_string);
 	end = p + Z_STRLEN_P(format_string);
 
@@ -4757,13 +4757,14 @@ static zend_result zend_compile_func_sprintf(znode *result, zend_ast_list *args)
 		}
 
 		switch (*q) {
-		case 's':
-			string_placeholder_count++;
-			break;
-		case '%':
-			break;
-		default:
-			return FAILURE;
+			case 's':
+			case 'd':
+				placeholder_count++;
+				break;
+			case '%':
+				break;
+			default:
+				return FAILURE;
 		}
 
 		p = q;
@@ -4771,7 +4772,7 @@ static zend_result zend_compile_func_sprintf(znode *result, zend_ast_list *args)
 	}
 
 	/* Bail out if the number of placeholders does not match the number of values. */
-	if (string_placeholder_count != (args->children - 1)) {
+	if (placeholder_count != (args->children - 1)) {
 		return FAILURE;
 	}
 
@@ -4785,27 +4786,22 @@ static zend_result zend_compile_func_sprintf(znode *result, zend_ast_list *args)
 
 	znode *elements = NULL;
 
-	if (string_placeholder_count > 0) {
-		elements = safe_emalloc(sizeof(*elements), string_placeholder_count, 0);
+	if (placeholder_count > 0) {
+		elements = safe_emalloc(sizeof(*elements), placeholder_count, 0);
 	}
 
 	/* Compile the value expressions first for error handling that is consistent
 	 * with a function call: Values that fail to convert to a string may emit errors.
 	 */
-	for (uint32_t i = 0; i < string_placeholder_count; i++) {
+	for (uint32_t i = 0; i < placeholder_count; i++) {
 		zend_compile_expr(elements + i, args->child[1 + i]);
-		if (elements[i].op_type == IS_CONST) {
-			if (Z_TYPE(elements[i].u.constant) != IS_ARRAY) {
-				convert_to_string(&elements[i].u.constant);
-			}
-		}
 	}
 
 	uint32_t rope_elements = 0;
 	uint32_t rope_init_lineno = -1;
 	zend_op *opline = NULL;
 
-	string_placeholder_count = 0;
+	placeholder_count = 0;
 	p = Z_STRVAL_P(format_string);
 	end = p + Z_STRLEN_P(format_string);
 	char *offset = p;
@@ -4817,7 +4813,7 @@ static zend_result zend_compile_func_sprintf(znode *result, zend_ast_list *args)
 
 		char *q = p + 1;
 		ZEND_ASSERT(q < end);
-		ZEND_ASSERT(*q == 's' || *q == '%');
+		ZEND_ASSERT(*q == 's' || *q == 'd' || *q == '%');
 
 		if (*q == '%') {
 			/* Optimization to not create a dedicated rope element for the literal '%':
@@ -4837,21 +4833,32 @@ static zend_result zend_compile_func_sprintf(znode *result, zend_ast_list *args)
 			opline = zend_compile_rope_add(result, rope_elements++, &const_node);
 		}
 
-		if (*q == 's') {
-			/* Perform the cast of constant arrays when actually evaluating corresponding placeholder
-			 * for correct error reporting.
-			 */
-			if (elements[string_placeholder_count].op_type == IS_CONST) {
-				if (Z_TYPE(elements[string_placeholder_count].u.constant) == IS_ARRAY) {
-					zend_emit_op_tmp(&elements[string_placeholder_count], ZEND_CAST, &elements[string_placeholder_count], NULL)->extended_value = IS_STRING;
-				}
+		if (*q != '%') {
+			switch (*q) {
+				case 's':
+					/* Perform the cast of constants when actually evaluating the corresponding placeholder
+					 * for correct error reporting.
+					 */
+					if (elements[placeholder_count].op_type == IS_CONST) {
+						if (Z_TYPE(elements[placeholder_count].u.constant) == IS_ARRAY) {
+							zend_emit_op_tmp(&elements[placeholder_count], ZEND_CAST, &elements[placeholder_count], NULL)->extended_value = IS_STRING;
+						} else {
+							convert_to_string(&elements[placeholder_count].u.constant);
+						}
+					}
+					break;
+				case 'd':
+					zend_emit_op_tmp(&elements[placeholder_count], ZEND_CAST, &elements[placeholder_count], NULL)->extended_value = IS_LONG;
+					break;
+				EMPTY_SWITCH_DEFAULT_CASE();
 			}
+
 			if (rope_elements == 0) {
 				rope_init_lineno = get_next_op_number();
 			}
-			opline = zend_compile_rope_add(result, rope_elements++, &elements[string_placeholder_count]);
+			opline = zend_compile_rope_add(result, rope_elements++, &elements[placeholder_count]);
 
-			string_placeholder_count++;
+			placeholder_count++;
 		}
 
 		p = q;
diff --git a/ext/standard/tests/strings/sprintf_rope_optimization_001.phpt b/ext/standard/tests/strings/sprintf_rope_optimization_001.phpt
index 91fdddfe029..634e575729d 100644
--- a/ext/standard/tests/strings/sprintf_rope_optimization_001.phpt
+++ b/ext/standard/tests/strings/sprintf_rope_optimization_001.phpt
@@ -100,6 +100,10 @@ try {
 	var_dump(sprintf());
 } catch (\Throwable $e) {echo $e, PHP_EOL; } echo PHP_EOL;
 
+try {
+	var_dump(sprintf('%s-%s-%s', true, false, true));
+} catch (\Throwable $e) {echo $e, PHP_EOL; } echo PHP_EOL;
+
 echo "Done";
 ?>
 --EXPECTF--
@@ -173,4 +177,6 @@ Stack trace:
 #0 %s(97): sprintf()
 #1 {main}
 
+string(4) "1--1"
+
 Done
diff --git a/ext/standard/tests/strings/sprintf_rope_optimization_003.phpt b/ext/standard/tests/strings/sprintf_rope_optimization_003.phpt
new file mode 100644
index 00000000000..9a0d819b277
--- /dev/null
+++ b/ext/standard/tests/strings/sprintf_rope_optimization_003.phpt
@@ -0,0 +1,134 @@
+--TEST--
+Test sprintf() function : Rope Optimization for '%d'.
+--FILE--
+<?php
+function func($num) {
+	return $num + 1;
+}
+function sideeffect() {
+	echo "Called!\n";
+	return "foo";
+}
+class Foo {
+	public function __construct() {
+		echo "Called\n";
+	}
+}
+
+$a = 42;
+$b = -1337;
+$c = 3.14;
+$d = new stdClass();
+
+try {
+	var_dump(sprintf("%d", $a));
+} catch (\Throwable $e) {echo $e, PHP_EOL; } echo PHP_EOL;
+
+try {
+	var_dump(sprintf("%d/%d", $a, $b));
+} catch (\Throwable $e) {echo $e, PHP_EOL; } echo PHP_EOL;
+
+try {
+	var_dump(sprintf("%d/%d/%d", $a, $b, $c));
+} catch (\Throwable $e) {echo $e, PHP_EOL; } echo PHP_EOL;
+
+try {
+	var_dump(sprintf("%d/%d/%d/%d", $a, $b, $c, $d));
+} catch (\Throwable $e) {echo $e, PHP_EOL; } echo PHP_EOL;
+
+try {
+	var_dump(sprintf("%d/", func(0)));
+} catch (\Throwable $e) {echo $e, PHP_EOL; } echo PHP_EOL;
+
+try {
+	var_dump(sprintf("/%d", func(0)));
+} catch (\Throwable $e) {echo $e, PHP_EOL; } echo PHP_EOL;
+
+try {
+	var_dump(sprintf("/%d/", func(0)));
+} catch (\Throwable $e) {echo $e, PHP_EOL; } echo PHP_EOL;
+
+try {
+	var_dump(sprintf("%d/%d/%d/%d", $a, $b, func(0), $a));
+} catch (\Throwable $e) {echo $e, PHP_EOL; } echo PHP_EOL;
+
+try {
+	var_dump(sprintf("%d/%d/%d/%d", __FILE__, __LINE__, 1, M_PI));
+} catch (\Throwable $e) {echo $e, PHP_EOL; } echo PHP_EOL;
+
+try {
+	var_dump(sprintf("%d/%d/%d", new Foo(), new Foo(), new Foo(), ));
+} catch (\Throwable $e) {echo $e, PHP_EOL; } echo PHP_EOL;
+
+try {
+	var_dump(sprintf('%d/%d/%d', [], [], []));
+} catch (\Throwable $e) {echo $e, PHP_EOL; } echo PHP_EOL;
+
+try {
+	if (PHP_INT_SIZE == 8) {
+		var_dump(sprintf('%d/%d/%d', PHP_INT_MAX, 0, PHP_INT_MIN));
+		var_dump("2147483647/0/-2147483648");
+	} else {
+		var_dump("9223372036854775807/0/-9223372036854775808");
+		var_dump(sprintf('%d/%d/%d', PHP_INT_MAX, 0, PHP_INT_MIN));
+	}
+} catch (\Throwable $e) {echo $e, PHP_EOL; } echo PHP_EOL;
+
+try {
+	var_dump(sprintf('%d/%d/%d', true, false, true));
+} catch (\Throwable $e) {echo $e, PHP_EOL; } echo PHP_EOL;
+
+try {
+	var_dump(sprintf("%d/%d", true, 'foo'));
+} catch (\Throwable $e) {echo $e, PHP_EOL; } echo PHP_EOL;
+
+try {
+	var_dump(sprintf("%d", 'foo'));
+} catch (\Throwable $e) {echo $e, PHP_EOL; } echo PHP_EOL;
+
+echo "Done";
+?>
+--EXPECTF--
+string(2) "42"
+
+string(8) "42/-1337"
+
+string(10) "42/-1337/3"
+
+
+Warning: Object of class stdClass could not be converted to int in %s on line 33
+string(12) "42/-1337/3/1"
+
+string(2) "1/"
+
+string(2) "/1"
+
+string(3) "/1/"
+
+string(13) "42/-1337/1/42"
+
+string(8) "0/53/1/3"
+
+Called
+Called
+Called
+
+Warning: Object of class Foo could not be converted to int in %s on line 57
+
+Warning: Object of class Foo could not be converted to int in %s on line 57
+
+Warning: Object of class Foo could not be converted to int in %s on line 57
+string(5) "1/1/1"
+
+string(5) "0/0/0"
+
+string(42) "9223372036854775807/0/-9223372036854775808"
+string(24) "2147483647/0/-2147483648"
+
+string(5) "1/0/1"
+
+string(3) "1/0"
+
+string(1) "0"
+
+Done
diff --git a/ext/standard/tests/strings/sprintf_rope_optimization_004.phpt b/ext/standard/tests/strings/sprintf_rope_optimization_004.phpt
new file mode 100644
index 00000000000..a1c937aeb2c
--- /dev/null
+++ b/ext/standard/tests/strings/sprintf_rope_optimization_004.phpt
@@ -0,0 +1,21 @@
+--TEST--
+Test sprintf() function : Rope Optimization for '%d' with GMP objects
+--EXTENSIONS--
+gmp
+--FILE--
+<?php
+
+$a = new GMP("42");
+$b = new GMP("-1337");
+$c = new GMP("999999999999999999999999999999999");
+
+try {
+	var_dump(sprintf("%d/%d/%d/%s", $a, $b, $c, $c + 1));
+} catch (\Throwable $e) {echo $e, PHP_EOL; } echo PHP_EOL;
+
+echo "Done";
+?>
+--EXPECTF--
+string(63) "42/-1337/4089650035136921599/1000000000000000000000000000000000"
+
+Done