Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iluuu1994
Copy link
Member

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

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
@dstogov
Copy link
Member

dstogov commented Nov 20, 2024

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 opcache_compile_file() during preloading). Please, check this (may be I'm wrong).

@iluuu1994
Copy link
Member Author

iluuu1994 commented Nov 20, 2024

You are right. This is what I referred to here:

This is breaking, and may thus only be applied to master. I will also notify the list and see whether there are concerns.

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. Even more confusing is that only rtd class names are registered, so you can't compile the file twice even though the user cannot see the class as being registered. Actually, this is not true, since RTD collisions are silently ignored.

Nonetheless, this should be discussed on the ML because some code might have to adjust.

@danog
Copy link
Contributor

danog commented Nov 21, 2024

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.

@iluuu1994
Copy link
Member Author

What do you mean by "caching"?

@danog
Copy link
Contributor

danog commented Nov 21, 2024

@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?)

@iluuu1994
Copy link
Member Author

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.

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