opcache_is_script_cached() honors preloaded scripts

[m6w6]

opcache_is_script_cached currently ignores preloaded scripts.

[m6w6]

Any chance this'll get a review pre-8.1RC1?

[dstogov]

It looks like we have a different problem.
opcache_is_script_cached() should work for preloaded scripts in the same way as for regular cached scripts, however it works only for full-paths or already created indirect keys.

I suppose, this should be fixed in a different way. Using zend_resolve_path().

diff --git a/ext/opcache/zend_accelerator_module.c b/ext/opcache/zend_accelerator_module.c
index da1696bb92..4822c5b8a8 100644
--- a/ext/opcache/zend_accelerator_module.c
+++ b/ext/opcache/zend_accelerator_module.c
@@ -307,11 +307,12 @@ ZEND_INI_END()
 
 static int filename_is_in_cache(zend_string *filename)
 {
-	zend_string *key;
+	zend_string *key = zend_resolve_path(filename);
 
-	key = accel_make_persistent_key(filename);
 	if (key != NULL) {
 		zend_persistent_script *persistent_script = zend_accel_hash_find(&ZCSG(hash), key);
+
+		zend_string_release(key);
 		if (persistent_script && !persistent_script->corrupted) {
 			if (ZCG(accel_directives).validate_timestamps) {
 				zend_file_handle handle;

@nikic do you see any problems?

[nikic]

@dstogov I'm not really familiar with path handling in opcache. Maybe this will cause performance regressions for file_override_enabled, as it will now try to realpath resolve the path?

[dstogov]

@nikic good point. It's better to keep the old behavior for file_override_enabled. Like this:

diff --git a/ext/opcache/zend_accelerator_module.c b/ext/opcache/zend_accelerator_module.c
index da1696bb92..379c68e188 100644
--- a/ext/opcache/zend_accelerator_module.c
+++ b/ext/opcache/zend_accelerator_module.c
@@ -305,13 +305,17 @@ ZEND_INI_BEGIN()
 #endif
 ZEND_INI_END()
 
-static int filename_is_in_cache(zend_string *filename)
+static int filename_is_in_cache(zend_string *filename, bool use_realpath)
 {
 	zend_string *key;
 
-	key = accel_make_persistent_key(filename);
+	key = use_realpath ? zend_resolve_path(filename) : accel_make_persistent_key(filename);
 	if (key != NULL) {
 		zend_persistent_script *persistent_script = zend_accel_hash_find(&ZCSG(hash), key);
+
+		if (use_realpath) {
+			zend_string_release(key);
+		}
 		if (persistent_script && !persistent_script->corrupted) {
 			if (ZCG(accel_directives).validate_timestamps) {
 				zend_file_handle handle;
@@ -337,7 +341,7 @@ static int accel_file_in_cache(INTERNAL_FUNCTION_PARAMETERS)
 		zval *zv = ZEND_CALL_ARG(execute_data , 1);
 
 		if (Z_TYPE_P(zv) == IS_STRING && Z_STRLEN_P(zv) != 0) {
-			return filename_is_in_cache(Z_STR_P(zv));
+			return filename_is_in_cache(Z_STR_P(zv), 0);
 		}
 	}
 	return 0;
@@ -921,5 +925,5 @@ ZEND_FUNCTION(opcache_is_script_cached)
 		RETURN_FALSE;
 	}
 
-	RETURN_BOOL(filename_is_in_cache(script_name));
+	RETURN_BOOL(filename_is_in_cache(script_name, 1));
 }

[dstogov]

@m6w6 does the last patch above solve the problem?

[m6w6]

@dstogov no, maybe there's a misunderstanding.

The problem is that during preloading, one cannot check whether a specific file has already been preloaded.

opcache_is_script_cached() checks ZCG(accelerator_enabled) which is false and even if it would be true, the files are not in ZCSG(hash) during preloading, at least as far as I understood.

That said, I'd be fine with a separate opcache_is_script_preloaded() function, too.

[dstogov]

I see. This is completely different problem and you are the first who asked for this feature.
I hope, it should be possible to find a solution without introducing new functions or behavior.

Can you describe what you are doing.
Do you use opcache_compile_file() and like to prevent errors because of double compilation of the same file? Can't you make a wrapper function that will keep static $hash_table?

In worst case it's better to introduce opcache_is_script_preloaded().

@nikic FYI

[m6w6]

@dstogov, I guess one would need to wrap the autoloader (which nowadays is usually composer)

[dstogov]

During preloading autoloader should behave in the same way as usually.
It shouldn't lead to inclusion of the same script twice.

Composer doesn't use opcache_compile_file() so you'll have to modify something anyway.

Please, describe your scenario and problem.

[m6w6]

Okay, imagine you've got a list from opcache status, which files make most sense to preload, which is separated into a list to just opcache_compile and a list to require due to $reasons, but this is actually not that important, I think.

In either way, I cannot ask PHP if a file has already been preloaded. I think it's a simple requirement and initially thought opcache_is_script_cached would already answer that question, but it doesn't.

[dstogov]

opcache_is_script_cached() shouldn't return true during preloading, because files are not cached yet.

If we introduce opcache_is_script_preloaded() it should work not only during preloading, but on any following request. It can't relay only on preload_scripts, because it is valid only during preloading.

See alternative implementation #7435

I'm not a fun of introducing new internal function for every possible scenario. Instead of using opcache_is_script_preloaded() it's possible to keep $hash of in your preload script.

I'm also not sure if it's possible to introduce internal function after feature freeze.

[nikic]

Okay, imagine you've got a list from opcache status, which files make most sense to preload, which is separated into a list to just opcache_compile and a list to require due to $reasons, but this is actually not that important, I think.

In either way, I cannot ask PHP if a file has already been preloaded. I think it's a simple requirement and initially thought opcache_is_script_cached would already answer that question, but it doesn't.

I still don't really get under what circumstances you could end up with an already preloaded file. Does your list of preloading candidates contain duplicates?

[m6w6]

I think it's a legitimate question to ask whether a file has been preloaded, as it is to ask whether a file has already been op-cached. I would have been fine if opcache_is_script_cached had answered this question while preloading without distinguishing between opcached and preloaded, but I'd also be fine with #7435, which clearly is a lot more sophisticated.

NB: Looks like using opcache_compile_file for preloading is not working as well as require anyway: https://github.com/m6w6/test-php-preload

[dstogov]

On Thu, Sep 2, 2021 at 9:51 AM Michael Wallner ***@***.***> wrote: I think it's a legitimate question to ask whether a file has been preloaded, as it is to ask whether a file has already been op-cached.

At request time, you can get information about preloaded files through opcache_get_status(true). During preloading this information is not available, because the preloading wasn't finished yet.

I would have been fine if opcache_is_script_cached had answered this question while preloading without distinguishing between opcached and preloaded, but I'd also be fine with #7435 <#7435>, which clearly is a lot more sophisticated.

This PR implements what you probably need, but I still didn't understand the use-case.

NB: Looks like using opcache_compile_file for preloading is not working as well as require anyway: https://github.com/m6w6/test-php-preload Yeah. There is a problem with circular class dependencies.

Nikita, can you please take a look. Thanks. Dmitry.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7300 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AATEZUV62JQD2O7FM4C6RNTT74NIBANCNFSM5A3WHUCA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[nikic]

NB: Looks like using opcache_compile_file for preloading is not working as well as require anyway: https://github.com/m6w6/test-php-preload Yeah. There is a problem with circular class dependencies.
Nikita, can you please take a look. Thanks. Dmitry.

This issue is fixed in PHP 8.1 already. Unresolved initializers no longer prevent opcache_compile_file() preloading.

[m6w6]

This issue is fixed in PHP 8.1 already. Unresolved initializers no longer prevent opcache_compile_file() preloading.

ACK, can confirm: https://github.com/m6w6/test-php-preload/actions/runs/1212844178

Closing in favor of #7435