opcache_is_script_cached_in_file_cache(string $filename) #16979

iamacarpet · 2024-11-28T10:42:29Z

A potential change to opcache_is_script_cached(...) that allows checking the file cache instead of SHM.

It's adding a new parameter, bool $file_cache = false, defaulting to false so the default / existing behaviour remains the same: when the second parameter is true, it'll check JUST the file cache, not SHM.

This should give us a better ability to test things relating to file caching, and hopefully enable some better unit testing for use with opcache.file_cache_read_only.

There are a couple of bits missing, like updating the docs, but I wanted to float the idea first.

@dstogov, @iluuu1994 , @arnaud-lb , @nielsdos : what do you think?

iluuu1994 · 2024-11-28T16:09:05Z

Hi @iamacarpet! No objections in terms of functionality, although I think this is something that should be mentioned on the mailing list. The parameter itself is a bit confusing. It's not obvious whether it additionally checks for file cache, or exclusively file cache. Since it's the latter, it may be better to create a separate method.

iamacarpet · 2024-11-28T16:25:14Z

Thanks @iluuu1994 , I'll do that, and yes, I'm not averse to making it its own function if that's preferable.

I wish I could get the automated test to pass so I knew I was along the right lines - I have no idea where I'm going wrong.

If you've got enough time, does anything stand out to you?

EDIT:

I think I've got it to pass now, will just need to wait for this latest build.

It appears like it's coming up false when opcache.file_cache_only is set.

I'm not 100% this is a bug with what this function is checking, rather than it being a bug with opcache.file_cache_only, as I seem to think when I was testing opcache.file_cache_read_only by hand (generating file cache on the CLI), it wasn't generating the cache correctly with that option set then either.

nielsdos

No objections from me functionality-wise.
I don't have time to deeply look into this now though.

nielsdos · 2024-11-28T22:05:36Z

ext/opcache/zend_accelerator_module.c

+	if (zend_parse_parameters(ZEND_NUM_ARGS(), "S|b", &script_name, &file_cache) == FAILURE) {
+		RETURN_THROWS();
+	}


I suppose the change away from fast-ZPP to regular ZPP is due to not knowing how this works instead of a deliberate change? (see https://wiki.php.net/rfc/fast_zpp)

Suggested change

if (zend_parse_parameters(ZEND_NUM_ARGS(), "S|b", &script_name, &file_cache) == FAILURE) {

RETURN_THROWS();

}

ZEND_PARSE_PARAMETERS_START(1, 2)

Z_PARAM_STR(script_name)

Z_PARAM_OPTIONAL

Z_PARAM_BOOL(file_cache)

ZEND_PARSE_PARAMETERS_END();

Thank you for the help with this!

dstogov · 2024-11-29T09:09:03Z

ext/opcache/zend_accelerator_module.c

+	zend_stream_init_filename_ex(&handle, filename);
+	handle.opened_path = realpath;
+
+	zend_persistent_script *persistent_script = zend_file_cache_script_load(&handle, true);


Will this lead to loading from the file cache?
This look weird.

Kind of, the , true parameter was to skip loading into SHM, so it'll effectively fully load the file & unserialize it, just not load it into SHM.

I'll agree this probably wasn't ideal.

How about this latest commit, swapping to zend_file_cache_script_validate(...) ?

I was unhappy with the code re-use, as ideally I'd want to replace the validation code within zend_file_cache_script_load(...) with a call to zend_file_cache_script_validate(...):

However, this had the side effect that I'd either be opening, reading & locking the cache file twice, or, we'd have to return a struct from zend_file_cache_script_validate(...) - something like this:

// New struct to hold validation results and the file descriptor typedef struct _zend_file_cache_validation_result { zend_file_cache_metainfo info; int fd; off_t file_size; } zend_file_cache_validation_result; // Modified validation function to return the file descriptor and file size static zend_file_cache_validation_result *zend_file_cache_script_validate(zend_file_handle *file_handle) { ... }

and that felt even more complicated.... So I'm at a loss really.

How would you handle it?

Adding a parameter to zend_file_cache_script_load() would be the simplest solution, but a separate function as you did is a better API (IMHO).

I like your suggestion of making the validate function return a struct, but I suggest to pass the struct as parameter so that the caller can allocate it on the stack:

zend_result zend_file_cache_script_validate(zend_file_cache_validation_result *result);

Maybe rename zend_file_cache_script_validate() to zend_file_cache_open(), and zend_file_cache_validation_result to zend_file_cache_handle, and add a small wrapper of zend_file_cache_open() that doesn't take this parameter:

zend_always_inline zend_result zend_file_cache_open(zend_file_handle *file_handle, zend_file_cache_handle *cache_handle); zend_result zend_file_cache_validate(zend_file_handle *file_handle) { zend_file_cache_handle cache_handle; return zend_file_cache_open(file_handle, &cache_handle); } // maybe also: zend_always_inline zend_file_cache_close(zend_file_cache_handle *cache_handle);

Thanks @arnaud-lb ,

I've just pushed my first try at implementing the way you've suggested.

I actually really like that approach and it makes a lot more logical sense.

In the end, rather than passing around a file descriptor, since we were already reading the whole file to potentially checksum it, I've opted to pass around the zend_arena_checkpoint and zend_arena_alloc values:

I won't pretend I understand this very well, so if I've done something wrong, please speak up!

Sorry, I overlooked the arena usage before my previous comment. Unfortunately, passing around the arena checkpoint would result in fragile code.

Alternatives would be to allocate the buffer in some other way, or to mmap the file, but this is too much changes and this may negatively impact the performance of zend_file_cache_script_load().

At this point I think that adding a validate_only parameter to zend_file_cache_script_load() is a better solution, after all.

No worries, getting your originally suggested method to build & work has been a great learning experience 😃

Thanks for your time reviewing and helping to move this along, it's really appreciated.

At this point I think that adding a validate_only parameter to zend_file_cache_script_load() is a better solution, after all.

This is pretty much what I'd done in my first attempt that @dstogov didn't seem to like, so before I switch back:

@dstogov are you happy with that?

Is there still any interest in us being able to implement this please?

And if so, @dstogov , are you able to weigh in on the above about what your preferred implementation is please?

There were some questions about consistency. I haven't gotten to them yet.

iamacarpet · 2024-12-02T13:59:21Z

That's building & running the tests fine now at-least.

So as I see it, a few things are outstanding:

Do we create a new function instead of opcache_is_script_cached(string $filename, bool $file_cache = false)?
- If we do this, what would it be called? opcache_is_script_cached_in_file_cache(string $filename)?
Is there a preference for implementing via zend_file_cache_script_validate(...) rather than an additional parameter to prevent loading into SHM on zend_file_cache_script_load(...)?
What's going on with opcache.file_cache_only?
- Cache files aren't being created / generated when this option is enabled - why?
- ZCG(accelerator_enabled) evaluates to false when this option is enabled - is that intentional?

I'd love some help / advice if anyone has time please? @iluuu1994 / @dstogov

arnaud-lb · 2024-12-02T14:49:26Z

Hi @iamacarpet

Do we create a new function instead of opcache_is_script_cached(string $filename, bool $file_cache = false)?

If we do this, what would it be called? opcache_is_script_cached_in_file_cache(string $filename)?

I agree that two separate functions is better, and in my opinion this name is good

What's going on with opcache.file_cache_only?

Cache files aren't being created / generated when this option is enabled - why?

ZCG(accelerator_enabled) evaluates to false when this option is enabled - is that intentional?

Could you share your opcache settings ? Anyting in the opcache logs ? I'm not observing this behavior when running php -n -dzend_extension=opcache.so -dopcache.enable_cli=1 -dopcache.file_cache=/tmp/filecache -dopcache.file_cache_only=1 test.php (or with -r 'opcache_compile_file("test.php");').

ext/opcache/zend_file_cache.c

…script_cached_file

dstogov

Now this looks fine

iamacarpet · 2025-02-04T10:44:11Z

Thanks @dstogov ,

@kocsismate , are you happy with this now?

If you've got time for a review please, that would be amazing! :).

iluuu1994 · 2025-02-04T14:00:33Z

ext/opcache/tests/gh16551_002.phpt

+$uncached_file = __DIR__ . '/gh16551_999.inc';
+
+var_dump(
+    opcache_is_script_cached_in_file_cache($file)


These tests look problematic to me. They will fail on first execution (confirmed locally).

========DIFF======== 001- bool(true) 001+ bool(false) 9 bool(false) 8 ========DONE======== FAIL GH-16551: opcache file cache read only: read file cache with limited checks [ext/opcache/tests/gh16551_003.phpt] ========DIFF======== 001- bool(true) 002- bool(true) 001+ bool(false) 002+ bool(false) 9 ========DONE======== FAIL GH-16551: opcache file cache read only: ensure we can't invalidate [ext/opcache/tests/gh16551_004.phpt]

I don't understand why they don't fail on CI. It's also possible they are order dependent, i.e. one job will prime the cache, another will check that the cache is not primed.

One solution might be to set opcache.file_cache to some sub-directory and clean it in the --CLEAN-- section to ensure a consistent state.

@iluuu1994 ,

It's also possible they are order dependent, i.e. one job will prime the cache, another will check that the cache is not primed.

Yes, they are order dependent - and using the conflict tag means they run one after another, even in parallel mode:

--CONFLICTS-- opcache_file_cache

However, I seem to have overlooked the fact that they expect the file-cache to be empty on first run, whereas yours already appears to be populated, potentially from a previous run - it won't be a problem on CI, as they always start with a fresh environment.

I think you are right about using --CLEAN-- - I'll look into implementing this, thank you!

EDIT: actually, no, my mistake - it's already using it's own file cache directly, as:

opcache.file_cache="{TMP}"

{TMP} should be a single-use temporary directory set up by the scripts that run the testing, so it shouldn't be pre-populated, even if you've run before on the same local machine.

We aren't cleaning it, as the script removes the temporary folder when it's finished running anyway.

I'm struggling to tell from your quoted output, but does it appear they are running out-of-order?

Yes, they are order dependent - and using the conflict tag means they run one after another, even in parallel mode:

Indeed, but these tests will still break with --shuffle. I think we should make an effort not to make tests order-dependent.

{TMP} should be a single-use temporary directory set up by the scripts that run the testing, so it shouldn't be pre-populated, even if you've run before on the same local machine.

{TMP} simply resolves to sys_get_temp_dir(), which usually is just /tmp. So it's not single-use. Actually, we depend on it not being single-use for the OPCACHE_VARIATION variation job in nightly, which runs test twice, first populating the file cache and then using it.

Thanks for the clarification! I’ll shuffle things around, probably into the same test rather than multiple? And ensure we’re using a folder than is then subsequently cleaned up.

I don't mind bigger tests if that's easier.

@iluuu1994 , sorry about the delay!

I've refactored the tests following your suggestions, and they should now run in any order, without being interdependent, and support the OPCACHE_VARIATION run properly.

Do these look any better?

And if you are happy with everything, do you think there is still time for this to potentially make it into PHP 8.5?

ext/opcache/zend_file_cache.c

…script_cached_file

ext/opcache/zend_accelerator_module.c

…script_cached_file

iluuu1994 · 2025-04-30T09:42:10Z

@iamacarpet Yes, there should be enough time for 8.5. I'll have another look soon.

iamacarpet · 2025-07-10T21:29:56Z

Thanks @iluuu1994 , sorry to pester, are we still ok time-wise for PHP 8.5?

And is there anything I can do to help this along?

iluuu1994 · 2025-07-11T09:40:58Z

Hi @iamacarpet. Apologies. Nothing to do on your side. I'll have a look today.

iluuu1994

The implementation itself still looks good, but I think the tests could be much simpler. Generally, I'd expect only a few simple tests that clear the cache and optionally create it in SKIPIF, and then checking what opcache_is_script_cached_in_file_cache() reports. Can you simplify them? I can create an example if you wish.

ext/opcache/zend_accelerator_module.c

iluuu1994 · 2025-07-11T13:50:06Z

ext/opcache/tests/gh16551_file_cache_only.phpt

+GH-16551: Behavior with opcache.file_cache_only=1
+--SKIPIF--
+<?php
+if (!extension_loaded('Zend OPcache')) die('skip Zend OPcache extension not available');


No reason for this, the --EXTENSIONS-- section already takes care of it.

iluuu1994 · 2025-07-11T13:51:28Z

ext/opcache/tests/gh16551_file_cache_only.phpt

+        }
+        @rmdir($dir);
+    } catch (UnexpectedValueException $e) { @rmdir($dir); } catch (Exception $e) { @rmdir($dir); }
+}


Please move this to some common helper file.

iluuu1994 · 2025-07-11T13:57:12Z

ext/opcache/tests/gh16551_file_cache_only.phpt

+--EXPECTF--
+Initial state (file_cache_only mode):
+bool(false)
+bool(%s)


I think it would be better if you had the setup of the test remove the cached file, and a separate test that includes the file. This way, you can test both conditions rather than assuming this will be correct.

iluuu1994 · 2025-07-11T14:03:21Z

ext/opcache/tests/gh16551_998.inc

@@ -0,0 +1,5 @@
+<?php
+
+$a = 4+5;


Why is this called 998? Also, why does it output 9, while 999 outputs 8. :D

iluuu1994 · 2025-07-11T14:10:34Z

ext/opcache/tests/gh16551_invalidate_file_cache_only.phpt

+       escapeshellarg($helperScript) . ' ' .
+       escapeshellarg($fileToCache) . ' ' .
+       escapeshellarg($cacheDir) . ' ' .                             // Pass path for verification/output
+       escapeshellarg($helperOutputFile);


I don't quite understand why this needs to happen in a separate process. Can you not just include the file from skipif via an include/function call?

iluuu1994 · 2025-07-11T14:11:28Z

ext/opcache/tests/gh16551_invalidate_file_cache_only.phpt

+
+// 3. Check if helper script succeeded via output file
+if (!file_exists($helperOutputFile) || trim(file_get_contents($helperOutputFile)) !== 'SUCCESS') {
+    echo "Helper script failed:\n";


In general, I think tests should be minimal rather than produce the maximal amount of output. If they break, it's easy to extend them and see what's wrong. Otherwise, the size is probably tripple of what it needs to be.

iluuu1994 · 2025-07-11T14:13:53Z

ext/opcache/tests/gh16551_jit_read_behavior.phpt

+opcache.enable=1
+opcache.enable_cli=1
+opcache.jit=tracing
+opcache.jit_buffer_size=16M ; Ensure JIT has buffer


Please stick with the default value of 64M if this is needed.

iluuu1994 · 2025-07-11T14:15:06Z

ext/opcache/tests/gh16551_jit_read_behavior.phpt

+; Main test process runs WITH JIT ENABLED
+opcache.enable=1
+opcache.enable_cli=1
+opcache.jit=tracing


I would question whether this needs to be JIT specific. The JIT only works in shm, it does not influence file cache, so what are we testing here?

…script_cached_file

Co-authored-by: Ilija Tovilo <[email protected]>

iamacarpet · 2025-07-11T21:16:31Z

Thanks for taking the time to review @iluuu1994 , and you were dead right about the tests:

I realise that on reflection, my main mistake was attempting to add in missing tests for an already merged PR (#16551) to this PR: it made things unnecessarily confusing, so I've removed all of these, and if you'll consider accepting a refined version, I'll submit these as another independent PR.

I've adopted your suggestions the best that I can, however, I'll admit the tests still aren't my strong point so feel free to give a better example if you'd still like to refine it please.

Is this a little more what you had in mind now?

…b.com/iamacarpet/php-src into opcache/opcache_is_script_cached_file

iamacarpet requested review from kocsismate and dstogov as code owners November 28, 2024 10:42

github-actions bot added the Extension: opcache label Nov 28, 2024

iamacarpet force-pushed the opcache/opcache_is_script_cached_file branch 5 times, most recently from 56f1c82 to 1cad353 Compare November 28, 2024 12:04

github-actions bot added the Extension: zend_test label Nov 28, 2024

opcache_is_script_cached: file_cache option

200193b

iamacarpet force-pushed the opcache/opcache_is_script_cached_file branch 3 times, most recently from 15dd23c to e77866e Compare November 28, 2024 15:23

opcache_is_script_cached: file_cache: test

798a07d

iamacarpet force-pushed the opcache/opcache_is_script_cached_file branch from e77866e to 798a07d Compare November 28, 2024 15:44

fix when file_cache_read_only

824e638

remove file_cache_only

30641d6

iamacarpet force-pushed the opcache/opcache_is_script_cached_file branch from 6cf19dc to 30641d6 Compare November 28, 2024 16:36

nielsdos reviewed Nov 28, 2024

View reviewed changes

dstogov reviewed Nov 29, 2024

View reviewed changes

iamacarpet added 2 commits November 29, 2024 15:10

switch to zend_file_cache_script_validate(...)

283844e

fix build

8cc7293

iamacarpet requested a review from dstogov December 2, 2024 13:59

arnaud-lb reviewed Dec 2, 2024

View reviewed changes

ext/opcache/zend_file_cache.c Outdated Show resolved Hide resolved

iamacarpet added 2 commits December 2, 2024 15:39

switch to opcache_is_script_cached_in_file_cache(...)

fd93557

remove double close(fd)

7e23285

iamacarpet added 2 commits February 3, 2025 15:01

Merge remote-tracking branch 'origin/master' into opcache/opcache_is_…

cd0177f

…script_cached_file

switch to validate_only method

7cff70a

iamacarpet force-pushed the opcache/opcache_is_script_cached_file branch from 6bca50d to 7cff70a Compare February 3, 2025 16:15

dstogov approved these changes Feb 4, 2025

View reviewed changes

iluuu1994 reviewed Feb 4, 2025

View reviewed changes

Merge remote-tracking branch 'origin/master' into opcache/opcache_is_…

59a4ecc

…script_cached_file

iamacarpet force-pushed the opcache/opcache_is_script_cached_file branch from 7c9559b to 14dfd4f Compare February 20, 2025 11:32

zend_file_cache_script_load_ex

b3b9501

iamacarpet force-pushed the opcache/opcache_is_script_cached_file branch from 14dfd4f to b3b9501 Compare February 21, 2025 09:49

iluuu1994 reviewed Feb 24, 2025

View reviewed changes

ext/opcache/zend_accelerator_module.c Show resolved Hide resolved

Merge remote-tracking branch 'origin/master' into opcache/opcache_is_…

d1b74e9

…script_cached_file

iamacarpet force-pushed the opcache/opcache_is_script_cached_file branch 4 times, most recently from 6e184da to c5734c0 Compare April 24, 2025 15:46

improve unit tests

1b8311b

iamacarpet force-pushed the opcache/opcache_is_script_cached_file branch from c5734c0 to 1b8311b Compare April 24, 2025 15:56

iamacarpet requested a review from iluuu1994 April 28, 2025 10:18

iluuu1994 reviewed Jul 11, 2025

View reviewed changes

iamacarpet and others added 3 commits July 11, 2025 20:05

Merge remote-tracking branch 'origin/master' into opcache/opcache_is_…

3e573f3

…script_cached_file

reduce whitespace

9346db9

Co-authored-by: Ilija Tovilo <[email protected]>

simplify tests & isolate to current change

f9c05a1

Merge branch 'opcache/opcache_is_script_cached_file' of https://githu…

d750ae0

…b.com/iamacarpet/php-src into opcache/opcache_is_script_cached_file

iamacarpet force-pushed the opcache/opcache_is_script_cached_file branch from e9605e7 to d750ae0 Compare July 11, 2025 21:33

fix local test run

fbb4194

-	if (zend_parse_parameters(ZEND_NUM_ARGS(), "S|b", &script_name, &file_cache) == FAILURE) {
-		RETURN_THROWS();
-	}
+	ZEND_PARSE_PARAMETERS_START(1, 2)
+		Z_PARAM_STR(script_name)
+		Z_PARAM_OPTIONAL
+		Z_PARAM_BOOL(file_cache)
+	ZEND_PARSE_PARAMETERS_END();

opcache_is_script_cached_in_file_cache(string $filename) #16979

Are you sure you want to change the base?

opcache_is_script_cached_in_file_cache(string $filename) #16979

Uh oh!

Conversation

iamacarpet commented Nov 28, 2024

Uh oh!

iluuu1994 commented Nov 28, 2024

Uh oh!

iamacarpet commented Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nielsdos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamacarpet Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnaud-lb Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamacarpet commented Dec 2, 2024

Uh oh!

arnaud-lb commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dstogov left a comment

Choose a reason for hiding this comment

Uh oh!

iamacarpet commented Feb 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamacarpet Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iluuu1994 Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

iluuu1994 commented Apr 30, 2025

Uh oh!

iamacarpet commented Jul 10, 2025

Uh oh!

iluuu1994 commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iluuu1994 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

iamacarpet commented Nov 28, 2024 •

edited

Loading

iamacarpet Nov 29, 2024 •

edited

Loading

arnaud-lb Dec 2, 2024 •

edited

Loading

arnaud-lb commented Dec 2, 2024 •

edited

Loading

iamacarpet Feb 24, 2025 •

edited

Loading

iluuu1994 Feb 24, 2025 •

edited

Loading

iluuu1994 commented Jul 11, 2025 •

edited

Loading