opcache_is_script_cached_in_file_cache(string $filename)

[iamacarpet]

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]

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]

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]

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

[iamacarpet]

Thank you for the help with this!

[dstogov]

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

[iamacarpet]

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?

[arnaud-lb]

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

[iamacarpet]

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!

[arnaud-lb]

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.

[iamacarpet]

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?

[iamacarpet]

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?

[iluuu1994]

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

[iamacarpet]

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]

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");').

[iamacarpet]

Just FYI, I've added some tests that cover GH-16551 (opcache.file_cache_read_only).

Hopefully I've done it right, but please let me know if not.

I deliberately made it fail to check it was running as expected:

Since then, I've pushed another update to ensure it passes again.

[iamacarpet]

Probably nothing to worry about if we won't go with this method anyway, but, for my own learning experience, does this look correct?:

ZCG(mem)

I'm not sure that feels right when the other variable is now cache_handle.mem, however, it compiles and runs fine.

[iamacarpet]

I'm a bit confused:

The CircleCI unit test for ARM is failing reliably on this line (it's returning true, when it should be false, as the file shouldn't have been cached yet).

I've added in the call to opcache_invalidate(...) above, which should remove it from the SHM if it's already there, and yet, it's still returning true.

Is this potentially a bug on ARM?

I'll sleep on it and see if I can figure out what's happening tomorrow.

[iamacarpet]

I couldn't re-create a problem with opcache_is_script_cached(...) in the 4 tests I created to probe this, so it's obviously something I was doing wrong.

I've removed the unnecessary tests and re-written the failing test to remove the part causing the issue.

[dstogov]

Instead of all these changes below, I would just add bool validate_only argument to zend_file_cache_script_load() and return true after header validation and before arena allocation and actual script loading.

[iamacarpet]

Thank you for the suggestion @dstogov !

I've made the modifications to do it this way, however, since zend_file_cache_script_load() returns zend_persistent_script *, I couldn't return true - I've had to mock that by returning a fixed, static instance of the zend_persistent_script struct.

Does this fit your expectations?

[dstogov]

Now this looks fine

[iamacarpet]

Thanks @dstogov ,

@kocsismate , are you happy with this now?

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

[iluuu1994]

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]

I would prefer renaming this to zend_file_cache_script_load_ex() and keeping the signature for zend_file_cache_script_load() the same so we don't need to adjust all callers.