Skip to content

Add optimizer support for prototype methods #7452

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+190 −12
Open

Add optimizer support for prototype methods #7452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions Zend/Optimizer/zend_func_info.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "zend_call_graph.h"
#include "zend_func_info.h"
#include "zend_inference.h"
#include "zend_execute.h"
#ifdef _WIN32
#include "win32/ioutil.h"
#endif
Expand Down Expand Up @@ -100,18 +101,20 @@ ZEND_API int zend_func_info_rid = -1;

uint32_t zend_get_internal_func_info(
const zend_function *callee_func, const zend_call_info *call_info, const zend_ssa *ssa) {
if (callee_func->common.scope) {
/* This is a method, not a function. */
if (callee_func->common.scope && !call_info->is_prototype) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be callee_func->common.scope && call_info->is_prototype? is_prototype indicates we might be calling an overridden method.
/* This is a non-prototype method. */
return 0;
}

zend_string *name = callee_func->common.function_name;
if (!name) {
zend_string *name = get_function_or_method_name_or_null(callee_func);
if (name == NULL) {
zend_string_release(name);
/* zend_pass_function has no name. */
return 0;
}

zval *zv = zend_hash_find_known_hash(&func_info, name);
zend_string_release(name);
if (!zv) {
return 0;
}
Expand Down
3 changes: 3 additions & 0 deletions Zend/Optimizer/zend_func_infos.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,7 @@ static const func_info_t func_infos[] = {
F1("posix_getrlimit", MAY_BE_ARRAY|MAY_BE_ARRAY_KEY_LONG|MAY_BE_ARRAY_KEY_STRING|MAY_BE_ARRAY_OF_LONG|MAY_BE_ARRAY_OF_STRING|MAY_BE_FALSE),
#endif
F1("random_bytes", MAY_BE_STRING),
FN("Random\\Randomizer::pickArrayKeys", MAY_BE_ARRAY|MAY_BE_ARRAY_KEY_LONG|MAY_BE_ARRAY_OF_STRING),
#if defined(HAVE_HISTORY_LIST)
F1("readline_list_history", MAY_BE_ARRAY|MAY_BE_ARRAY_KEY_LONG|MAY_BE_ARRAY_OF_STRING),
#endif
Expand Down Expand Up @@ -678,6 +679,8 @@ static const func_info_t func_infos[] = {
F1("xml_error_string", MAY_BE_STRING|MAY_BE_NULL),
F1("xml_parser_get_option", MAY_BE_STRING|MAY_BE_LONG|MAY_BE_BOOL),
FN("zend_test_create_throwing_resource", MAY_BE_RESOURCE),
F1("_ZendTestTypeInference::getIntArray", MAY_BE_ARRAY|MAY_BE_ARRAY_KEY_LONG|MAY_BE_ARRAY_OF_LONG),
F1("_ZendTestTypeInference::createIntArray", MAY_BE_ARRAY|MAY_BE_ARRAY_KEY_LONG|MAY_BE_ARRAY_OF_LONG),
FN("zip_open", MAY_BE_RESOURCE|MAY_BE_LONG|MAY_BE_FALSE),
FN("zip_read", MAY_BE_RESOURCE|MAY_BE_FALSE),
F1("ob_gzhandler", MAY_BE_STRING|MAY_BE_FALSE),
Expand Down
1 change: 1 addition & 0 deletions Zend/Optimizer/zend_inference.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3885,6 +3885,7 @@ static zend_always_inline zend_result _zend_update_type_info(
case ZEND_DO_ICALL:
case ZEND_DO_UCALL:
case ZEND_DO_FCALL_BY_NAME:
case ZEND_INIT_METHOD_CALL:
Copy link
Member

@iluuu1994 iluuu1994 Mar 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This looks wrong. ZEND_INIT_METHOD_CALL does not define a result. Methods are called using DO_FCALL, so this is already handled.
if (ssa_op->result_def >= 0) {
zend_func_info *func_info = ZEND_FUNC_INFO(op_array);
zend_call_info *call_info;
Expand Down
22 changes: 22 additions & 0 deletions Zend/Optimizer/zend_optimizer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -914,6 +914,28 @@ zend_function *zend_optimizer_get_called_func(
}
return fbc;
}
} else if (opline->op1_type == IS_CONST
&& Z_TYPE_P(CRT_CONSTANT(opline->op1)) == IS_OBJECT
Copy link
Member

Choose a reason for hiding this comment

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

Objects cannot be IS_CONST. I don't think this branch is hit. You should look at the inferred SSA variable instead.
&& opline->op2_type == IS_CONST && Z_TYPE_P(CRT_CONSTANT(opline->op2)) == IS_STRING
&& op_array->scope
&& !(op_array->fn_flags & ZEND_ACC_TRAIT_CLONE)
&& !(op_array->scope->ce_flags & ZEND_ACC_TRAIT)) {
zend_string *method_name = Z_STR_P(CRT_CONSTANT(opline->op2) + 1);
zend_function *fbc = zend_hash_find_ptr(
&op_array->scope->function_table, method_name);
Copy link
Member

@iluuu1994 iluuu1994 Mar 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The class here should be based on op1 though, not op_array->scope.
if (fbc) {
if (fbc->type == ZEND_INTERNAL_FUNCTION
|| (fbc->type == ZEND_USER_FUNCTION && fbc->common.fn_flags & ZEND_ACC_PUBLIC)) {
/* Prototype methods are potentially overridden. fbc still contains useful type information.
* Some optimizations may not be applied, like inlining or inferring the send-mode of superfluous args.
* A method cannot be overridden if the class or method is final. */
if ((fbc->common.fn_flags & ZEND_ACC_FINAL) == 0 &&
Copy link
Member

Choose a reason for hiding this comment

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

Nit: && on next line.
(fbc->common.scope->ce_flags & ZEND_ACC_FINAL) == 0) {
*is_prototype = true;
}
return fbc;
}
}
}
break;
case ZEND_NEW:
Expand Down
5 changes: 4 additions & 1 deletion Zend/zend_exceptions.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,10 @@ public function __wakeup(): void {}

final public function getMessage(): string {}

/** @return int */
/**
* @return mixed
* @no-verify
*/
final public function getCode() {} // TODO add proper type (i.e. int|string)

final public function getFile(): string {}
Expand Down
2 changes: 1 addition & 1 deletion Zend/zend_exceptions_arginfo.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Zend/zend_execute.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,7 @@ static zend_always_inline zend_function *zend_active_function(void)
}

ZEND_API zend_string *get_active_function_or_method_name(void);
ZEND_API zend_string *get_function_or_method_name_or_null(const zend_function *func);
ZEND_API zend_string *get_function_or_method_name(const zend_function *func);
ZEND_API const char *zend_get_executed_filename(void);
ZEND_API zend_string *zend_get_executed_filename_ex(void);
Expand Down
11 changes: 9 additions & 2 deletions Zend/zend_execute_API.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,13 +582,20 @@ ZEND_API zend_string *get_active_function_or_method_name(void) /* {{{ */
}
/* }}} */

ZEND_API zend_string *get_function_or_method_name(const zend_function *func) /* {{{ */
ZEND_API zend_string *get_function_or_method_name_or_null(const zend_function *func)
{
if (func->common.scope && func->common.function_name) {
return zend_create_member_string(func->common.scope->name, func->common.function_name);
}

return func->common.function_name ? zend_string_copy(func->common.function_name) : ZSTR_INIT_LITERAL("main", 0);
return func->common.function_name ? zend_string_copy(func->common.function_name) : NULL;
}

ZEND_API zend_string *get_function_or_method_name(const zend_function *func) /* {{{ */
{
zend_string *name = get_function_or_method_name_or_null(func);

return name ? name : ZSTR_INIT_LITERAL("main", 0);
}
/* }}} */

Expand Down
22 changes: 20 additions & 2 deletions build/gen_stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,10 @@ protected function __construct(string $name, bool $isBuiltin) {
$this->isBuiltin = $isBuiltin;
}

public function isMixed(): bool {
return $this->isBuiltin && $this->name === "mixed";
}

public function isScalar(): bool {
return $this->isBuiltin && in_array($this->name, ["null", "false", "true", "bool", "int", "float"], true);
}
Expand Down Expand Up @@ -526,7 +530,7 @@ public function toOptimizerTypeMask(): string {
case "iterable":
Copy link
Contributor

Choose a reason for hiding this comment

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

(GitHub doesn't allow commenting on the above line for case "callable":)

Does callable also need to include MAY_BE_ARRAY_OF_REF? If I use callable as a parameter type hint, I can still modify fields of an array callable by reference (and see them get modified)

php > class B { public function main() { echo "in main\n"; } }
php > $class = new B();
php > $method = 'main';
php > $callable = [&$class, &$method];
php > $callable();
in main
php > function example(callable $c) { $c(); $c[1] = 'changed'; }
php > example($callable);
in main
php > var_dump($callable);
array(2) {
  [0]=>
  &object(B)#1 (0) {
  }
  [1]=>
  &string(7) "changed"
}
php >

Not sure what the other implications of this would be for the optimizer, @nikic would know more (E.g. something with a callable type hint might easily cease to be a callable?)
return "MAY_BE_ARRAY|MAY_BE_ARRAY_KEY_ANY|MAY_BE_ARRAY_OF_ANY|MAY_BE_OBJECT";
Copy link
Contributor

Choose a reason for hiding this comment

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

I think MAY_BE_ARRAY_OF_REF would also make sense here?
case "mixed":
return "MAY_BE_ANY|MAY_BE_ARRAY_KEY_ANY|MAY_BE_ARRAY_OF_ANY";
return "MAY_BE_ANY|MAY_BE_ARRAY_KEY_ANY|MAY_BE_ARRAY_OF_ANY|MAY_BE_ARRAY_OF_REF";
}

return $this->toTypeMask();
Expand Down Expand Up @@ -634,6 +638,16 @@ private function __construct(array $types, bool $isIntersection) {
$this->isIntersection = $isIntersection;
}

public function isMixed(): bool {
foreach ($this->types as $type) {
if ($type->isMixed()) {
return true;
}
}

return false;
}

public function isScalar(): bool {
foreach ($this->types as $type) {
if (!$type->isScalar()) {
Expand Down Expand Up @@ -1512,7 +1526,7 @@ public function getFunctionEntry(): string {
}

public function getOptimizerInfo(): ?string {
if ($this->isMethod()) {
if ($this->isMethod() && !$this->isFinalMethod()) {
return null;
}

Expand All @@ -1529,6 +1543,10 @@ public function getOptimizerInfo(): ?string {
return null;
}

if ($type->isMixed()) {
return null;
}

return "\tF" . $this->return->refcount . '("' . addslashes($this->name->__toString()) . '", ' . $type->toOptimizerTypeMask() . "),\n";
}

Expand Down
61 changes: 61 additions & 0 deletions ext/opcache/tests/opt/method_func_info.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
--TEST--
Test that internal methods can be optimized based on zend_func_infos.h
--INI--
opcache.enable=1
opcache.enable_cli=1
opcache.optimization_level=-1
opcache.opt_debug_level=0x20000
opcache.preload=
--EXTENSIONS--
opcache
zend_test
--FILE--
<?php

class Test extends _ZendTestTypeInference {
public function test1(): int {
return $this->getIntArray()[0] ?? 0;
}

public static function test2(): int {
return $this->createIntArray()[0] ?? 0;
}
}

?>
--EXPECTF--
$_main:
; (lines=2, args=0, vars=0, tmps=0)
; (after optimizer)
; %s
0000 DECLARE_CLASS_DELAYED string("test") string("_zendtesttypeinference")
0001 RETURN int(1)

Test::test1:
; (lines=7, args=0, vars=0, tmps=2)
; (after optimizer)
; %s
0000 INIT_METHOD_CALL 0 THIS string("getIntArray")
0001 V0 = DO_FCALL
0002 T1 = FETCH_DIM_IS V0 int(0)
0003 T0 = COALESCE T1 0005
0004 T0 = QM_ASSIGN int(0)
0005 VERIFY_RETURN_TYPE T0
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't these be optimized away then? What does this test test?
0006 RETURN T0
LIVE RANGES:
0: 0005 - 0006 (tmp/var)

Test::test2:
; (lines=8, args=0, vars=0, tmps=2)
; (after optimizer)
; %s
0000 V0 = FETCH_THIS
0001 INIT_METHOD_CALL 0 V0 string("createIntArray")
0002 V0 = DO_FCALL
0003 T1 = FETCH_DIM_IS V0 int(0)
0004 T0 = COALESCE T1 0006
0005 T0 = QM_ASSIGN int(0)
0006 VERIFY_RETURN_TYPE T0
0007 RETURN T0
LIVE RANGES:
0: 0006 - 0007 (tmp/var)
1 change: 1 addition & 0 deletions ext/random/random.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ public function shuffleArray(array $array): array {}

public function shuffleBytes(string $bytes): string {}

/** @return array<int, string> */
public function pickArrayKeys(array $array, int $num): array {}
Comment on lines +151 to 152
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct, the returned array keys might be integers. The correct type would be list<int|string>.

public function __serialize(): array {}
Expand Down
2 changes: 1 addition & 1 deletion ext/random/random_arginfo.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 21 additions & 0 deletions ext/zend_test/test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ ZEND_DECLARE_MODULE_GLOBALS(zend_test)
static zend_class_entry *zend_test_interface;
static zend_class_entry *zend_test_class;
static zend_class_entry *zend_test_child_class;
static zend_class_entry *zend_test_type_inference;
static zend_class_entry *zend_attribute_test_class;
static zend_class_entry *zend_test_trait;
static zend_class_entry *zend_test_attribute;
Expand Down Expand Up @@ -1033,6 +1034,24 @@ static ZEND_METHOD(_ZendTestClass, takesUnionType)
RETURN_NULL();
}

static ZEND_METHOD(_ZendTestTypeInference, getIntArray)
{
array_init(return_value);

add_next_index_long(return_value, 1);
add_next_index_long(return_value, 2);
add_next_index_long(return_value, 3);
}

static ZEND_METHOD(_ZendTestTypeInference, createIntArray)
{
array_init(return_value);

add_next_index_long(return_value, 1);
add_next_index_long(return_value, 2);
add_next_index_long(return_value, 3);
}

// Returns a newly allocated DNF type `Iterator|(Traversable&Countable)`.
//
// We need to generate it "manually" because gen_stubs.php does not support codegen for DNF types ATM.
Expand Down Expand Up @@ -1129,6 +1148,8 @@ PHP_MINIT_FUNCTION(zend_test)
memcpy(&zend_test_class_handlers, &std_object_handlers, sizeof(zend_object_handlers));
zend_test_class_handlers.get_method = zend_test_class_method_get;

zend_test_type_inference = register_class__ZendTestTypeInference();

zend_attribute_test_class = register_class_ZendAttributeTest();

zend_test_trait = register_class__ZendTestTrait();
Expand Down
15 changes: 15 additions & 0 deletions ext/zend_test/test.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,21 @@ class _ZendTestChildClass extends _ZendTestClass
public function returnsThrowable(): Exception {}
}

class _ZendTestTypeInference
{
/**
* @return array<int, int>
* @refcount 1
*/
final public function getIntArray(): array {}

/**
* @return array<int, int>
* @refcount 1
*/
final public static function createIntArray(): array {}
}

class ZendAttributeTest {
/** @var int */
#[ZendTestRepeatableAttribute]
Expand Down
24 changes: 23 additions & 1 deletion ext/zend_test/test_arginfo.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.