Omit FETCH_THIS in closures

[iluuu1994]

Non-static closures declared in non-static methods are guaranteed to have $this. The existing comment highlights this, but fails to handle it correctly.

[mvorisek]

This PR seems to be related with #10782, can it fix it?

[iluuu1994]

@mvorisek I don't think this will fix the mentioned problem. It is related to run_time_cache per closure, this only slightly tweaks the opcodes of the closure.

[staabm]

Does this change improve closure performance?

[iluuu1994]

It seems so.

class C {
    public function test() {
        $c = function () {
            for ($i = 0; $i < 100000000; $i++) {
                $this->foo();
            }
        };
        $c();
    }

    private function foo() {}
}

$c = new C();
$c->test();

Before:

~/Developer/php-src @d2a9edfe ?1 ❯ hyperfine 'taskset --cpu-list 0 php-dev -d opcache.enable_cli=1 test.php'                                                 8s
Benchmark 1: taskset --cpu-list 0 php-dev -d opcache.enable_cli=1 test.php
  Time (mean ± σ):     795.8 ms ±   6.9 ms    [User: 790.8 ms, System: 2.7 ms]
  Range (min … max):   779.1 ms … 805.2 ms    10 runs

After:

~/Developer/php-src omit-fetch-this-in-closures ?1 ❯ hyperfine 'taskset --cpu-list 0 php-dev -d opcache.enable_cli=1 test.php'                               4s
Benchmark 1: taskset --cpu-list 0 php-dev -d opcache.enable_cli=1 test.php
  Time (mean ± σ):     570.1 ms ±  15.6 ms    [User: 565.7 ms, System: 2.8 ms]
  Range (min … max):   557.7 ms … 609.9 ms    10 runs

But this is highly synthetic, so take it with a grain of salt. I'm not sure whether it improves real-life performance.

[dstogov]

I like the patch, and in general everything looks right.
I tested it with PHP-Parser benchmark and it shows slight improvement with function JIT and without JIT.
However it also shows degradation with tracing JIT.
For some reason, PHP starts to generate bigger and slower code.

Can you investigate this please. (I suppose, JIT have some CLOSURE specific optimization that stopped work after your patch).

[dstogov]

This demonstrates the worse JIT code generated for FETCH_DIM_R.

<?php
class Foo {
	public $a = 42;
	public function test() {
		return function() {
			return $this->a;
		};
	}
}
$obj = new Foo();
$fn = $obj->test();
for ($i = 0; $i < 100; $i++) {
	$fn();
}
?>

sapi/cli/php -d opcache.jit_debug=0x1ff201 fetch_this.php 2>err1.log
sapi/cli/php.patched -d opcache.jit_debug=0x1ff201 fetch_this.php 2>err2.log
diff -u err1.log err2.log

The degradation is caused by call to zend_jit_fetch_obj(). Without the patch it gets trace_ce recorded by the tracer and generates class guards and optimized code. With the patch trace_ce is NULL and the slow code is generated.

It's possible that we have similar degradation for other related instructions. (ASSIGN_OBJ, INIT_METHOD_CALL, etc)

It looks like the following extension to the tracer fixes all degradation. May be it should record $this class only for closures. (I'm not sure).

diff --git a/ext/opcache/jit/zend_jit_vm_helpers.c b/ext/opcache/jit/zend_jit_vm_helpers.c
index 7df2cd5b902..4d055ac2ece 100644
--- a/ext/opcache/jit/zend_jit_vm_helpers.c
+++ b/ext/opcache/jit/zend_jit_vm_helpers.c
@@ -692,6 +692,25 @@ zend_jit_trace_stop ZEND_FASTCALL zend_jit_trace_execute(zend_execute_data *ex,
 				}
 			}
 			op1_type |= flags;
+		} else if (opline->op1_type == IS_UNUSED
+		 && (opline->opcode == ZEND_FETCH_OBJ_R
+		  || opline->opcode == ZEND_FETCH_OBJ_W
+		  || opline->opcode == ZEND_FETCH_OBJ_RW
+		  || opline->opcode == ZEND_FETCH_OBJ_IS
+		  || opline->opcode == ZEND_FETCH_OBJ_FUNC_ARG
+		  || opline->opcode == ZEND_FETCH_OBJ_UNSET
+		  || opline->opcode == ZEND_ASSIGN_OBJ
+		  || opline->opcode == ZEND_ASSIGN_OBJ_REF
+		  || opline->opcode == ZEND_ASSIGN_OBJ_OP
+		  || opline->opcode == ZEND_PRE_INC_OBJ
+		  || opline->opcode == ZEND_PRE_DEC_OBJ
+		  || opline->opcode == ZEND_POST_INC_OBJ
+		  || opline->opcode == ZEND_POST_DEC_OBJ
+		  || opline->opcode == ZEND_ISSET_ISEMPTY_PROP_OBJ
+		  || opline->opcode == ZEND_UNSET_OBJ
+		  || opline->opcode == ZEND_INIT_METHOD_CALL
+		  || opline->opcode == ZEND_CLONE)) {
+			ce1 = Z_OBJCE(EX(This));
 		}
 		if (opline->op2_type & (IS_TMP_VAR|IS_VAR|IS_CV)
 		 && opline->opcode != ZEND_INSTANCEOF

@iluuu1994 Please finalize this.

[dstogov]

Looks Fine!