Skip to content

opcache_is_script_cached_in_file_cache(string $filename) #16979

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.

Sign up for GitHub

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

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
+133 −1
Open

opcache_is_script_cached_in_file_cache(string $filename) #16979

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
200193b
opcache_is_script_cached: file_cache option
iamacarpet Nov 28, 2024
798a07d
opcache_is_script_cached: file_cache: test
iamacarpet Nov 28, 2024
824e638
fix when file_cache_read_only
iamacarpet Nov 28, 2024
30641d6
remove file_cache_only
iamacarpet Nov 28, 2024
283844e
switch to zend_file_cache_script_validate(...)
iamacarpet Nov 29, 2024
8cc7293
fix build
iamacarpet Dec 2, 2024
fd93557
switch to opcache_is_script_cached_in_file_cache(...)
iamacarpet Dec 2, 2024
7e23285
remove double close(fd)
iamacarpet Dec 2, 2024
906b92d
zend_file_cache_open with struct
iamacarpet Dec 2, 2024
1cdf3fa
fix build
iamacarpet Dec 2, 2024
f620d9c
Unit Tests for GH-16551
iamacarpet Dec 3, 2024
6df1562
fix for file_cache_only
iamacarpet Dec 3, 2024
e282578
probe ARM test failure
iamacarpet Dec 4, 2024
17edbe6
remove unnecessary tests
iamacarpet Dec 4, 2024
cd0177f
Merge remote-tracking branch 'origin/master' into opcache/opcache_is_…
iamacarpet Feb 3, 2025
7cff70a
switch to validate_only method
iamacarpet Feb 3, 2025
59a4ecc
Merge remote-tracking branch 'origin/master' into opcache/opcache_is_…
iamacarpet Feb 20, 2025
b3b9501
zend_file_cache_script_load_ex
iamacarpet Feb 20, 2025
d1b74e9
Merge remote-tracking branch 'origin/master' into opcache/opcache_is_…
iamacarpet Apr 24, 2025
1b8311b
improve unit tests
iamacarpet Apr 24, 2025
3e573f3
Merge remote-tracking branch 'origin/master' into opcache/opcache_is_…
iamacarpet Jul 11, 2025
9346db9
reduce whitespace
iamacarpet Jul 11, 2025
f9c05a1
simplify tests & isolate to current change
iamacarpet Jul 11, 2025
d750ae0
Merge branch 'opcache/opcache_is_script_cached_file' of https://githu…
iamacarpet Jul 11, 2025
fbb4194
fix local test run
iamacarpet Jul 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ext/opcache/ZendAccelerator.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1916,7 +1916,7 @@ static zend_op_array *file_cache_compile_file(zend_file_handle *file_handle, int

HANDLE_BLOCK_INTERRUPTIONS();
SHM_UNPROTECT();
persistent_script = zend_file_cache_script_load(file_handle);
persistent_script = zend_file_cache_script_load(file_handle, false);
SHM_PROTECT();
HANDLE_UNBLOCK_INTERRUPTIONS();
if (persistent_script) {
Expand Down Expand Up @@ -2133,7 +2133,7 @@ zend_op_array *persistent_compile_file(zend_file_handle *file_handle, int type)

/* Check the second level cache */
if (!persistent_script && ZCG(accel_directives).file_cache) {
persistent_script = zend_file_cache_script_load(file_handle);
persistent_script = zend_file_cache_script_load(file_handle, false);
}

/* If script was not found or invalidated by validate_timestamps */
Expand Down
2 changes: 1 addition & 1 deletion ext/opcache/opcache.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,4 @@ function opcache_jit_blacklist(Closure $closure): void {}
*/
function opcache_get_configuration(): array|false {}

function opcache_is_script_cached(string $filename): bool {}
function opcache_is_script_cached(string $filename, bool $file_cache = false): bool {}
7 changes: 5 additions & 2 deletions ext/opcache/opcache_arginfo.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 27 additions & 0 deletions ext/opcache/tests/gt16979.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
--TEST--
GH-16979: opcache_is_script_cached support file_cache argument
--INI--
opcache.enable=1
opcache.enable_cli=1
opcache.jit=disable
opcache.jit_buffer_size=0
opcache.file_cache={TMP}
--EXTENSIONS--
opcache
--FILE--
<?php

opcache_compile_file(__FILE__);

var_dump(
opcache_is_script_cached(__FILE__, false)
);

var_dump(
opcache_is_script_cached(__FILE__, true)
);

?>
--EXPECT--
bool(true)
bool(true)
46 changes: 42 additions & 4 deletions ext/opcache/zend_accelerator_module.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,13 @@
#include <time.h>

#include "php.h"
#include "zend.h"
#include "ZendAccelerator.h"
#include "zend_API.h"
#include "zend_closures.h"
#include "zend_shared_alloc.h"
#include "zend_accelerator_blacklist.h"
#include "zend_file_cache.h"
#include "php_ini.h"
#include "SAPI.h"
#include "zend_virtual_cwd.h"
Expand Down Expand Up @@ -363,6 +365,37 @@ static int filename_is_in_cache(zend_string *filename)
return 0;
}

static int filename_is_in_file_cache(zend_string *filename)
{
if (!ZCG(accel_directives).file_cache) {
return 0;
}

zend_string *realpath;

realpath = zend_resolve_path(filename);

if (!realpath) {
return 0;
}

zend_file_handle handle;

zend_stream_init_filename_ex(&handle, filename);
handle.opened_path = realpath;

zend_persistent_script *persistent_script = zend_file_cache_script_load(&handle, true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@iamacarpet iamacarpet Nov 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

@arnaud-lb arnaud-lb Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


zend_destroy_file_handle(&handle);

if (persistent_script) {
return 1;
}

return 0;
}


static int accel_file_in_cache(INTERNAL_FUNCTION_PARAMETERS)
{
if (ZEND_NUM_ARGS() == 1) {
Expand Down Expand Up @@ -983,10 +1016,11 @@ ZEND_FUNCTION(opcache_compile_file)
ZEND_FUNCTION(opcache_is_script_cached)
{
zend_string *script_name;
bool file_cache = 0;

ZEND_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_STR(script_name)
ZEND_PARSE_PARAMETERS_END();
if (zend_parse_parameters(ZEND_NUM_ARGS(), "S|b", &script_name, &file_cache) == FAILURE) {
RETURN_THROWS();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the help with this!


if (!validate_api_restriction()) {
RETURN_FALSE;
Expand All @@ -996,5 +1030,9 @@ ZEND_FUNCTION(opcache_is_script_cached)
RETURN_FALSE;
}

RETURN_BOOL(filename_is_in_cache(script_name));
if (file_cache) {
RETURN_BOOL(filename_is_in_file_cache(script_name));
} else {
RETURN_BOOL(filename_is_in_cache(script_name));
}
}
3 changes: 2 additions & 1 deletion ext/opcache/zend_file_cache.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1813,7 +1813,7 @@ static void zend_file_cache_unserialize(zend_persistent_script *script,
zend_file_cache_unserialize_early_bindings(script, buf);
}

zend_persistent_script *zend_file_cache_script_load(zend_file_handle *file_handle)
zend_persistent_script *zend_file_cache_script_load(zend_file_handle *file_handle, bool force_file_cache_only)
{
zend_string *full_path = file_handle->opened_path;
int fd;
Expand Down Expand Up @@ -1928,6 +1928,7 @@ zend_persistent_script *zend_file_cache_script_load(zend_file_handle *file_handl
}

if (!file_cache_only &&
!force_file_cache_only &&
!ZCSG(restart_in_progress) &&
!ZCSG(restart_pending) &&
!ZSMMG(memory_exhausted) &&
Expand Down
2 changes: 1 addition & 1 deletion ext/opcache/zend_file_cache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
#define ZEND_FILE_CACHE_H

int zend_file_cache_script_store(zend_persistent_script *script, bool in_shm);
zend_persistent_script *zend_file_cache_script_load(zend_file_handle *file_handle);
zend_persistent_script *zend_file_cache_script_load(zend_file_handle *file_handle, bool force_file_cache_only);
void zend_file_cache_invalidate(zend_string *full_path);

#endif /* ZEND_FILE_CACHE_H */
Loading