Skip to content

zend_closures.c: Call closure directly #17372

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.

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

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Jan 5, 2025

This prevents a bunch of indirection and calling a C function via a function pointer

This prevents a bunch of indirection and calling a C function via a function pointer
@Girgias Girgias marked this pull request as ready for review January 5, 2025 23:08
@Girgias Girgias requested a review from dstogov as a code owner January 5, 2025 23:08
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

So, this is avoiding a call to zend_is_callable_ex()? Is this measurably faster?

if (call_user_function_named(CG(function_table), NULL, ZEND_THIS, return_value, num_args, args, named_args) == FAILURE) {
RETVAL_FALSE;
}
zend_fcall_info_cache fcc = {
Copy link
Member

@iluuu1994 iluuu1994 Jan 6, 2025

Choose a reason for hiding this comment

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

Nit: This causes some unnecessary initialization or most fields that are set later. It's probably elided if zend_closure_get_closure() is inlined though.

@Girgias
Copy link
Member Author

Girgias commented Jan 6, 2025

So, this is avoiding a call to zend_is_callable_ex()? Is this measurably faster?

Basically yes as the current call does the following:

  • call_user_function_named (which is just _call_user_function_impl() which sets up a FCI struct to call
  • zend_call_function which, due to lack of an FCC, calls
  • zend_is_callable_ex (which calls zend_is_callable_at_frame) which calls
  • Z_OBJ_HANDLER_P(callable, get_closure) (which corresponds to zend_closure_get_closure) to get the FCC to continue the zend_call_function() call

I did not measure if this is faster or not, as I'm trying to rely less on the function_name field of the FCI to see if this can be removed at one point and just rely on the FCC struct.

Another reason is that this is the only reference to call_user_function_named.

I could also just inline zend_closure_get_closure to prevent the function call together.

(note to self, I should update https://www.phpinternalsbook.com/php7/internal_types/functions/callables.html or move it to the new php-src internal docs as some stuff has changed since I wrote this)

@iluuu1994
Copy link
Member

A quick benchmark would be good when touching performance critical code. Sometimes performance changes unexpectedly.

@Girgias
Copy link
Member Author

Girgias commented Jan 6, 2025

A quick benchmark would be good when touching performance critical code. Sometimes performance changes unexpectedly.

What sort of benchmark do you want me to run? I also don't see how this could possibly be slower than the existing indirections.

@iluuu1994
Copy link
Member

I see this is only called when doing $c->__invoke(), so it should be ok. But generally, it's not too time consuming to test changes that might have performance implications. You can just do a synthetic benchmark, i.e. hit your code in a loop of enough iterations to be measurable.

@Girgias Girgias merged commit e40543a into php:master Jan 7, 2025
10 checks passed
@Girgias Girgias deleted the closure-calling-api branch January 7, 2025 09:20
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.

2 participants