Skip to content

Add first-class callable cache #19863

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

Closed
wants to merge 10 commits into from
Closed

Add first-class callable cache #19863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b45bae9
Add fcc cache
iluuu1994 Sep 17, 2025
34375ab
Fix for optimizer, re-enable for main
iluuu1994 Sep 17, 2025
ec7402d
Fix dom test
iluuu1994 Sep 17, 2025
fb4d6e9
Switch to hash table cache
iluuu1994 Sep 22, 2025
d18d26f
Fix destructor
iluuu1994 Sep 22, 2025
3e93f66
Fix static methods and tests
iluuu1994 Sep 22, 2025
f32bb97
Revert accidental change
iluuu1994 Sep 22, 2025
bee5b99
Use function ptr as callable cache key
iluuu1994 Sep 23, 2025
f8f5006
Use zend_hash_index_lookup() and simplify
iluuu1994 Sep 30, 2025
7ec8ba8
Remove now unneeded callable_convert_dtor()
iluuu1994 Sep 30, 2025
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
6 changes: 6 additions & 0 deletions Zend/Optimizer/compact_literals.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -740,6 +740,12 @@ void zend_optimizer_compact_literals(zend_op_array *op_array, zend_optimizer_ctx
cache_size += 2 * sizeof(void *);
}
break;
case ZEND_CALLABLE_CONVERT:
if (opline->extended_value != (uint32_t)-1) {
opline->extended_value = cache_size;
cache_size += sizeof(void *);
}
break;
}
opline++;
}
Expand Down
6 changes: 3 additions & 3 deletions Zend/tests/exit/exit_as_function.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ foreach ($values as $value) {
}

?>
--EXPECT--
--EXPECTF--
string(4) "exit"
string(3) "die"
object(Closure)#1 (2) {
object(Closure)#%d (2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Every optimization should have at least one test. If the object ID is the same after this optimization/PR, then at least that should be tested. The reason is that any optimization condition can broke in the future and noone will notice it, especially when there is no big impact on the benchmarks in the CI.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is the test - https://3v4l.org/eGPAJ - I expect true after this PR.

["function"]=>
string(4) "exit"
["parameter"]=>
Expand All @@ -31,7 +31,7 @@ object(Closure)#1 (2) {
string(10) "<optional>"
}
}
object(Closure)#2 (2) {
object(Closure)#%d (2) {
["function"]=>
string(4) "exit"
["parameter"]=>
Expand Down
8 changes: 4 additions & 4 deletions Zend/tests/first_class_callable/constexpr/namespace_004.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ foo();

?>
--EXPECTF--
object(Closure)#1 (2) {
object(Closure)#%d (2) {
["function"]=>
string(6) "strrev"
["parameter"]=>
Expand All @@ -36,7 +36,7 @@ object(Closure)#1 (2) {
}
}
string(3) "cba"
object(Closure)#2 (2) {
object(Closure)#%d (2) {
["function"]=>
string(6) "strrev"
["parameter"]=>
Expand All @@ -46,7 +46,7 @@ object(Closure)#2 (2) {
}
}
string(3) "cba"
object(Closure)#2 (2) {
object(Closure)#%d (2) {
["function"]=>
string(6) "strrev"
["parameter"]=>
Expand All @@ -56,7 +56,7 @@ object(Closure)#2 (2) {
}
}
string(3) "cba"
object(Closure)#1 (2) {
object(Closure)#%d (2) {
["function"]=>
string(6) "strrev"
["parameter"]=>
Expand Down
6 changes: 3 additions & 3 deletions Zend/tests/first_class_callable/first_class_callable_optimization.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@ var_dump(test1(...));
var_dump(test2(...));

?>
--EXPECT--
object(Closure)#1 (1) {
--EXPECTF--
object(Closure)#%d (1) {
["function"]=>
string(5) "test1"
}
object(Closure)#1 (1) {
object(Closure)#%d (1) {
["function"]=>
string(5) "test2"
}
14 changes: 7 additions & 7 deletions Zend/tests/magic_methods/trampoline_closure_named_arguments.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ var_dump($type);
var_dump($type->getName());

?>
--EXPECT--
--EXPECTF--
-- Non-static cases --
string(4) "test"
array(3) {
Expand All @@ -69,15 +69,15 @@ array(4) {
["a"]=>
int(123)
["b"]=>
object(Test)#1 (0) {
object(Test)#%d (0) {
}
}
string(4) "test"
array(2) {
["a"]=>
int(123)
["b"]=>
object(Test)#1 (0) {
object(Test)#%d (0) {
}
}
string(4) "test"
Expand Down Expand Up @@ -114,15 +114,15 @@ array(4) {
["a"]=>
int(123)
["b"]=>
object(Test)#1 (0) {
object(Test)#%d (0) {
}
}
string(10) "testStatic"
array(2) {
["a"]=>
int(123)
["b"]=>
object(Test)#1 (0) {
object(Test)#%d (0) {
}
}
string(10) "testStatic"
Expand All @@ -136,12 +136,12 @@ array(1) {
-- Reflection tests --
array(1) {
[0]=>
object(ReflectionParameter)#4 (1) {
object(ReflectionParameter)#%d (1) {
["name"]=>
string(9) "arguments"
}
}
bool(true)
object(ReflectionNamedType)#5 (0) {
object(ReflectionNamedType)#%d (0) {
}
string(5) "mixed"
9 changes: 8 additions & 1 deletion Zend/zend_compile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3964,7 +3964,14 @@ static bool zend_compile_call_common(znode *result, zend_ast *args_ast, zend_fun
opline->op1.num = zend_vm_calc_used_stack(0, fbc);
}

zend_emit_op_tmp(result, ZEND_CALLABLE_CONVERT, NULL, NULL);
zend_op *callable_convert_op = zend_emit_op_tmp(result, ZEND_CALLABLE_CONVERT, NULL, NULL);
if (opline->opcode == ZEND_INIT_FCALL
|| opline->opcode == ZEND_INIT_FCALL_BY_NAME
|| opline->opcode == ZEND_INIT_NS_FCALL_BY_NAME) {
callable_convert_op->extended_value = zend_alloc_cache_slot();
} else {
callable_convert_op->extended_value = (uint32_t)-1;
}
return true;
}

Expand Down
6 changes: 6 additions & 0 deletions Zend/zend_execute_API.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,8 @@ void init_executor(void) /* {{{ */
zend_fiber_init();
zend_weakrefs_init();

zend_hash_init(&EG(callable_convert_cache), 8, NULL, ZVAL_PTR_DTOR, 0);

EG(active) = 1;
}
/* }}} */
Expand Down Expand Up @@ -420,6 +422,8 @@ ZEND_API void zend_shutdown_executor_values(bool fast_shutdown)
zend_stack_clean(&EG(user_error_handlers), (void (*)(void *))ZVAL_PTR_DTOR, 1);
zend_stack_clean(&EG(user_exception_handlers), (void (*)(void *))ZVAL_PTR_DTOR, 1);

zend_hash_clean(&EG(callable_convert_cache));

#if ZEND_DEBUG
if (!CG(unclean_shutdown)) {
gc_collect_cycles();
Expand Down Expand Up @@ -516,6 +520,8 @@ void shutdown_executor(void) /* {{{ */
if (EG(ht_iterators) != EG(ht_iterators_slots)) {
efree(EG(ht_iterators));
}

zend_hash_destroy(&EG(callable_convert_cache));
}

#if ZEND_DEBUG
Expand Down
2 changes: 2 additions & 0 deletions Zend/zend_globals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,8 @@ struct _zend_executor_globals {

zend_strtod_state strtod_state;

HashTable callable_convert_cache;

void *reserved[ZEND_MAX_RESERVED_RESOURCES];
};

Expand Down
20 changes: 18 additions & 2 deletions Zend/zend_vm_def.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9709,12 +9709,28 @@ ZEND_VM_HANDLER(167, ZEND_COPY_TMP, TMPVAR, UNUSED)
ZEND_VM_NEXT_OPCODE();
}

ZEND_VM_HANDLER(202, ZEND_CALLABLE_CONVERT, UNUSED, UNUSED)
ZEND_VM_HANDLER(202, ZEND_CALLABLE_CONVERT, UNUSED, UNUSED, NUM|CACHE_SLOT)
{
USE_OPLINE
zend_execute_data *call = EX(call);

zend_closure_from_frame(EX_VAR(opline->result.var), call);
if (opline->extended_value != (uint32_t)-1) {
zend_object *closure = CACHED_PTR(opline->extended_value);
if (closure) {
ZVAL_OBJ_COPY(EX_VAR(opline->result.var), closure);
} else {
zval *closure_zv = zend_hash_index_lookup(&EG(callable_convert_cache), (zend_ulong)(uintptr_t)call->func);
if (Z_TYPE_P(closure_zv) == IS_NULL) {
zend_closure_from_frame(closure_zv, call);
}
ZEND_ASSERT(Z_TYPE_P(closure_zv) == IS_OBJECT);
closure = Z_OBJ_P(closure_zv);
ZVAL_OBJ_COPY(EX_VAR(opline->result.var), closure);
CACHE_PTR(opline->extended_value, closure);
}
} else {
zend_closure_from_frame(EX_VAR(opline->result.var), call);
}

if (ZEND_CALL_INFO(call) & ZEND_CALL_RELEASE_THIS) {
OBJ_RELEASE(Z_OBJ(call->This));
Expand Down
36 changes: 34 additions & 2 deletions Zend/zend_vm_execute.h
View file
Open in desktop

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

2 changes: 1 addition & 1 deletion Zend/zend_vm_opcodes.c
View file
Open in desktop

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

10 changes: 5 additions & 5 deletions ext/dom/tests/registerPhpFunctionNS.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,14 @@ $xpath->registerPhpFunctionNS('urn:bar', 'test', 'strtolower');
var_dump($xpath->query('//a[bar:test(string(@href)) = "https://php.net"]'));

?>
--EXPECT--
--EXPECTF--
--- Legit cases: global function callable ---
object(DOMNodeList)#5 (1) {
["length"]=>
int(1)
}
--- Legit cases: string callable ---
object(DOMNodeList)#5 (1) {
object(DOMNodeList)#%d (1) {
["length"]=>
int(1)
}
Expand All @@ -79,12 +79,12 @@ array(1) {
[0]=>
string(15) "https://PHP.net"
}
object(DOMNodeList)#3 (1) {
object(DOMNodeList)#%d (1) {
["length"]=>
int(0)
}
--- Legit cases: instance class method callable ---
object(DOMNodeList)#6 (1) {
object(DOMNodeList)#%d (1) {
["length"]=>
int(1)
}
Expand All @@ -100,7 +100,7 @@ array(1) {
--- Legit cases: global function callable that returns nothing ---
string(15) "https://PHP.net"
--- Legit cases: multiple namespaces ---
object(DOMNodeList)#5 (1) {
object(DOMNodeList)#%d (1) {
["length"]=>
int(1)
}