Skip to content

Conversation

iluuu1994
Copy link
Member

@iluuu1994
Copy link
Member Author

@dktapps Ping. I haven't done any benchmarking yet.

@dktapps
Copy link
Contributor

dktapps commented Sep 23, 2025

This isn't caching static closures yet, right? So there should be no observable effect on performance

@iluuu1994
Copy link
Member Author

iluuu1994 commented Sep 23, 2025

This isn't caching static closures yet, right?

Right. IIRC static closures themselves have a small benefit, though I didn't see it in the CI benchmark. I'm not sure if maybe Symfony already uses linting to add static.

iluuu1994 added a commit to php/benchmarking-symfony-demo-2.2.3 that referenced this pull request Sep 24, 2025
@iluuu1994
Copy link
Member Author

Okay, testing Symfony Demo with all static closures turned into non-static ones shows virtually no improvement. Regardless, they should benefit if we can cache them. I'll try this in the same PR then, given this one isn't useful on its own.

@arnaud-lb
Copy link
Member

One drawback of non-static closures is that they retain $this, which can increase memory usage when the lifetime of $this is supposed to be shorter and references a large graph. I believe this is why coding guidelines and IDEs recommend to manually declare closure as static.

Big +1 on this.

zend_error(E_WARNING, "Cannot bind an instance to a static closure, this will be an error in PHP 9");
return 0;
// FIXME: Restrict this workaround to implicitly static closures?
// Will need an additional fn_flag.
Copy link
Member

Choose a reason for hiding this comment

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

This could be a zend_closure flag, passed to zend_create_closure() by ZEND_DECLARE_LAMBDA_FUNCTION. The flag can be stored in OBJ_EXTRA_FLAGS() as there is no other place to store a flag in zend_closure. I do this in the pfa branch: https://github.com/arnaud-lb/php-src/blob/23dce325280c7c6c90043ad3bf830073f6838df3/Zend/zend_types.h#L873 + https://github.com/arnaud-lb/php-src/blob/23dce325280c7c6c90043ad3bf830073f6838df3/Zend/zend_closures.c#L31

@iluuu1994
Copy link
Member Author

iluuu1994 commented Sep 24, 2025

Quick test: In Symfony Demo with all static closures turned into non-static ones, this patch can turn 122/283 into static ones. That's ~43%, so not bad at all. How many of those can also be cached remains to be seen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants