-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add new core autoloading mechanism for classes and functions #8294
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?
Conversation
fyi, the test Zend/tests/autoloading/function/register_autoloader.phpt appears to be failing due to the param parsing failing as the passed in string is not already callable. |
The test exceptions_during_autoloading004.phpt appears to be failing as the function autoloading doesn't appear to be triggered for the function in a namespace. |
62b7697
to
29c9cc9
Compare
29c9cc9
to
ee63c9b
Compare
…ot be unregistered) There are two issues to resolve: 1. The FCC is not refetch when trying to unregister a trampoline 2. Comparing the function pointer of trampolines is meaningless as they are reallocated, thus we need to compare the name of the function Found while working on phpGH-8294
…be unregistered) There are two issues to resolve: 1. The FCC is not refetch when trying to unregister a trampoline 2. Comparing the function pointer of trampolines is meaningless as they are reallocated, thus we need to compare the name of the function Found while working on GH-8294 Closes GH-10033
ee63c9b
to
874b3f5
Compare
ca7a80f
to
41f9378
Compare
2e93f3c
to
f88a32e
Compare
359966e
to
90c39d3
Compare
Zend/zend_vm_def.h
Outdated
function_name = (zval*)RT_CONSTANT(opline, opline->op2); | ||
func = zend_hash_find_known_hash(EG(function_table), Z_STR_P(function_name+1)); | ||
if (UNEXPECTED(func == NULL)) { | ||
ZEND_VM_DISPATCH_TO_HELPER(zend_undefined_function_helper); | ||
zval *function_name = (zval*)RT_CONSTANT(opline, opline->op2); | ||
/* Fetch lowercase name stored in the next literal slot */ | ||
fbc = zend_lookup_function_ex(Z_STR_P(function_name), Z_STR_P(function_name+1), /* use_autoload */ true); | ||
if (UNEXPECTED(fbc == NULL)) { | ||
if (EXPECTED(!EG(exception))) { | ||
ZEND_VM_DISPATCH_TO_HELPER(zend_undefined_function_helper); | ||
} | ||
HANDLE_EXCEPTION(); |
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.
I would propose to try autoloading only after simple hash lookup.
This should completely eliminate performance degradation for existing code.
e.g.
function_name = (zval*)RT_CONSTANT(opline, opline->op2);
func = zend_hash_find_known_hash(EG(function_table), Z_STR_P(function_name+1));
if (UNEXPECTED(func == NULL)) {
func = zend_autoload_function(Z_STR_P(function_name), Z_STR_P(function_name+1));
if (UNEXPECTED(func == NULL)) {
if (EXPECTED(!EG(exception))) {
ZEND_VM_DISPATCH_TO_HELPER(zend_undefined_function_helper);
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.
Shouldn't this already be the case because of how I implemented zend_lookup_function_ex()
?
As it will first check the HashTable, and then call the autoloader if it hasn't found an entry.
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.
Your implementation introduces overhead of call to zend_lookup_function_ex()
, few additional checks and use of zend_hash_find()
instead of zend_hash_find_known_hash()
.
I don't know where to write, but I vote for consts (defines) autoloading, that will be extra-usable after const objects got allowed in 8.1. |
Is there any plans to implement this feature in 8.4 release? |
I need to get back to finishing the RFC to explain why certain proposed ideas don't work. But that's the objective yes. |
Just a thought, but would it be another logical way of using one, yet-agnostic, list for all autoload functions rather than two, but have a bitflag on each registered callable as to what autoload occasions that function should fire on? Another possible approach to this is to have a third, but generic, set autoload functions which take a bitwise parameter to map one or more event types (i.e. Something else to consider, is that there will certainly be cases where a coder may want the exact same segment of code to execute in more than one particular event. From a RAM-occupancy standpoint, having only one copy of a registered function in-memory, where its purpose is to fire on more than one event type, would be a little more efficient. Also, if, in a future RFC, that there is the thought to add more autoload event types, additional bitflags can be reserved for them (i.e. |
I don't think this is a good API at all. Which is why I did not go with this in the first place, the previous proposal for function autoloading did just that, and I think it is confusing and if one of the autoloading mechanisms needs to be amened to behave differently via a flag or what-no this would pollute the whole API surface. Individual functions are cheap, and clear. Moreover, I frankly think it is unlikely that a class autoloader and a function autoloader are remotely close in implementation. Classes in PHP are typically stored in a one to one correspondences with files, whereas doing that for functions is a questionable choice. Similarly, thing for autoloading constants, or type aliases. |
OpCache doesn't work properly as the local function gets bound to a global one.
e6d02e3
to
060bab7
Compare
…all" This approach doesn't really work This reverts commit 060bab7.
This PR introduces a new autoloading mechanism built into the engine which supports the autoloading of classes and functions.
RFC: https://wiki.php.net/rfc/core-autoloading