-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
Avoid registering top functions with opcache_compile_file() #16862
base: master
Are you sure you want to change the base?
Conversation
Previously, this only worked for classes. It worked by preventing early binding, and then declaring the class at runtime. Instead of doing that, we now avoid calling zend_accel_load_script() completely, which prevents the functions from being added to the function table. Fixes phpGH-16668
According to documentation "opcache_compile_file — Compiles and caches a PHP script without executing it". This doesn't say if function and classes are loaded into the current process. Actually, they were loaded and this patch changes the behavior. The PR doesn't not affect the main desired behavior (opcache priming), but may break some code that relays on the existing behavior (e.g. usage of |
You are right. This is what I referred to here:
IMO, it doesn't make much sense to register only functions but not classes. This will break when mixing classes and functions, because using the class will trigger the autoloader and then fail due to function redeclaration. Nonetheless, this should be discussed on the ML because some code might have to adjust. |
I believe the removal of function caching is a bit of an overreaction: it was always known, to those who used it, that preloading will cache functions, and requires an include guard to avoid function redeclaration errors, and in fact it is a very useful feature. There is nothing wrong with this behaviour, and it does what you expect it to do. |
What do you mean by "caching"? |
@iluuu1994 I have confused the linked issue with another issue related to preloads that I encountered and worked around with include guards (amphp/amp#321). From what I see here, this PR would disable population of the function table when using opcache_compile_file (which is actually correct, I again misinterpeted the initial issue, thinking it was about preloading); this is what I meant by disabling cache (would the function's op_arrays still be stored anywhere to avoid re-parsing though?) |
Yes, the functions are still cached in opcache, they just aren't loaded in the current request. That said, I have noticed some inconsistencies with preloading I'll have to examine. Namely, opcache_compile_file() dodges opcache during preloading and just loads it like normal. Skipping function declaration will be a bit more tricky there, we need to be careful not to introduce more inconsistencies. |
Previously, this only worked for classes. It worked by preventing early binding, and then declaring the class at runtime. Instead of doing that, we now avoid calling zend_accel_load_script() completely, which prevents the functions from being added to the function table.
This is breaking, and may thus only be applied to master. I will also notify the list and see whether there are concerns.
Fixes GH-16668