-
Notifications
You must be signed in to change notification settings - Fork 1
Partials v2 gen #22
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
base: master
Are you sure you want to change the base?
Partials v2 gen #22
Conversation
7062e66 to
f65bf45
Compare
d820a4f to
75dbd22
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get a test case where the function has optional arguments and an incorrect number is given (just to see the error message once).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test shows this error message: https://github.com/arnaud-lb/php-src/blob/75dbd22353d81d34d63587f9c703523f5d10b82e/Zend/tests/partial_application/rfc_examples_003.phpt
Do you prefer a dedicated test case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I just realize that the error messages for userland and native functions are already inconsistent with each other:
75dbd22 to
bf4b4f6
Compare
Co-authored-by: Joe Watkins <[email protected]>
Co-authored-by: Tim Düsterhus <[email protected]>
bf4b4f6 to
b753b0c
Compare
| try { | ||
| test(1,...)(?); | ||
| } catch (Error $ex) { | ||
| echo "OK"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| echo "OK"; | |
| echo $ex::class, ": ", $ex->getMessage(), PHP_EOL; |
Zend/tests/partial_application/superfluous_args_are_forwarded.phpt
Outdated
Show resolved
Hide resolved
Zend/tests/partial_application/superfluous_args_are_forwarded.phpt
Outdated
Show resolved
Hide resolved
| if (UNEXPECTED(zend_call_trampoline_arginfo[0].name == NULL)) { | ||
| zend_call_trampoline_arginfo[0].name = ZSTR_KNOWN(ZEND_STR_ARGS); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to do this at MINIT, so that it's not necessary to check the (unexpected) path for every call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree
| ZEND_API bool zend_check_type_ex( | ||
| const zend_type *type, zval *arg, zend_class_entry *scope, | ||
| bool is_return_type, bool is_internal) | ||
| { | ||
| return zend_check_type(type, arg, scope, is_return_type, is_internal); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of this? Wouldn't it work to expose zend_check_type() directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not that easy, as the function is always_inline, and uses always_inline functions from this file as well. zend_check_type() is critical, I didn't want to take the risk of performance regression.
So we have the following options:
- Move the function and a bunch of functions it uses to a header file (zend_check_type, zend_check_type_slow, zend_check_intersection_type_from_list, probably others), so we don't lose inline-ability
- Make the function extern inline, but we don't do that (there was a thread about that in a PR).
- Export a wrapper, like I did here
I will think about about the other options
PFAs are created by generating the AST of a Closure at runtime and compiling it. The compiled op_array can be cached similarly to linked classes.
About caching
There are two levels of caching:
ZCG(hash))The cache key in
ZCG(hash)is composed of the address of the declaring opline, the address of the function being called, and a prefix.This scheme is not ideal because:
ZCG(hash)may be a valid file nameZCG(hash)A better solution might be to cache PFA op_arrays in the declaring op_array (similarly, the inheritance cache is stored in classes). This cache could store multiple PFAs, and could be used regardless of opcache being enabled.
Alternatively, use a global two-level map: First level is indexed by the "signature" of the PFA, second level by the function being called. This would lead to a better cache hit rate when the same function is partialled in multiple locations.
TODO