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

Fix caching of PHP_SAPI value in opcache file_cache #3222

Closed
wants to merge 1 commit into from

Conversation

YuriyNasretdinov
Copy link

When using opcache.file_cache option both in web and cli contexts you get very strange results when PHP_SAPI constant value has different values in different files (it depends on context in which file was included first: cli or web).

This pull requests makes attempt to fix this in a bit hacky way (we only disallow a single constant, PHP_SAPI, from being substituted).

I am not sure whether or not adding new constant like CONST_CT_NO_PERSISTENT_SUBST would be better.

@nikic
Copy link
Member

nikic commented Apr 26, 2018

Another option would be to include the sapi in the system ID, which would prevent the cache from being shared between different SAPIs. @dstogov What do you think about this? Is the cache supposed to be shared between SAPIs?

@YuriyNasretdinov
Copy link
Author

I personally would like to be able to reuse file cache between CLI and Web because it means that it would consume 2 times less disk space. Also it allows to generate file cache once and deploy it. It is much more convenient in CLI mode rather than doing it from web server :).

@dstogov
Copy link
Member

dstogov commented Apr 26, 2018

The patch prevents caching of PHP_SAPI not only in file_cache, but also in SHM.
Disabling all persistent constants substitution, using ZEND_COMPILE_NO_PERSISTENT_CONSTANT_SUBSTITUTION, may prevent substitution of other constants.

We may include SAPI into system ID, like @nikic proposed, or introduce a new constant/compilation flags that prevent compile-time constant substitution if file-cache is enabled.

@YuriyNasretdinov
Copy link
Author

As far as I can see, patch should not prevent other persistent constants from substitution because there already was logic for persistent constant substitution in opcache extension, but execution never got to that branch because all persistent constants were already substituted.

I agree that this patch prevents caching of PHP_SAPI in SHM as well but I personally do not see it as a big problem because consistency is probably more important :).

Maybe it is not a bad idea to prevent all (persistent) constants from substitution but I am not sure how it is going to affect performance in CLI mode (performance boost is the primary reason for using file_cache in the first place in my opinion).

@dstogov
Copy link
Member

dstogov commented Apr 27, 2018

I've committed another fix (into master only).

83f98f7

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

Successfully merging this pull request may close these issues.

None yet

3 participants