Avoid registering top functions with opcache_compile_file() #16862
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. |
|
Great work @iluuu1994 , Now that #16551 has been merged for support of It's possible to work around the current behaviour, but it isn't ideal: Ideally, I'd love to see us adopt this change, but otherwise, updating the documentation to reflect the actual behaviour would be useful. |
| if (CG(compiler_options) & ZEND_COMPILE_WITHOUT_EXECUTION) { | ||
| /* Not a pretty API, but we need to distinguish between successful and | ||
| * unsuccessful compilations. */ | ||
| return (void *) -1; | ||
| } | ||
|
|
There was a problem hiding this comment.
Please, check the situation when from_shared_memory is false.
@iluuu1994 can you please explain that inconsistencies. Limiting only |
Yes. During preloading,
This is a fair point. What would be the correct behavior in this case? If the script cannot be cached, there's no point in compiling it at all. So probably the function should just abort. |
I afraid this may break preloading (linking of classes loaded through opcache_compile_file()).
probably yes. |
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