Skip to content

Conversation

arnaud-lb
Copy link
Member

@arnaud-lb arnaud-lb commented Jul 29, 2025

Add a C function opcache_preloading() that returns true during preloading. Extensions can use this to detect preloading, not only during compilation/execution, but also in RINIT()/RSHUTDOWN().

Since opcache currently doesn't install any header, I'm adding a new one: zend_accelerator_api.h. Header name is based on other files in ext/opcache.

@arnaud-lb arnaud-lb marked this pull request as ready for review July 29, 2025 14:26
#endif
void *preloaded_internal_run_time_cache;
size_t preloaded_internal_run_time_cache_size;
bool preloading;
Copy link
Member

Choose a reason for hiding this comment

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

I don't know much about packing data structure declarations to reduce space, would it be better to put this next to the other bool fields above?

Copy link
Member Author

Choose a reason for hiding this comment

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

I hesitated, as I was weighting the benefits of packing vs placing the fields near related fields. I chose the latter because there is only one "instance" of this struct at a time, so this should not make a huge difference.

ZEND_FE(zend_test_log_err_debug, arginfo_zend_test_log_err_debug)
ZEND_FE(zend_test_compile_to_ast, arginfo_zend_test_compile_to_ast)
ZEND_FE(zend_test_gh18756, arginfo_zend_test_gh18756)
ZEND_FE(zend_test_opcache_preloading, arginfo_zend_test_opcache_preloading)
Copy link
Member

Choose a reason for hiding this comment

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

given that the relevant function is only being introduced in PHP 8.5, it doesn't make sense for zend_test_opcache_preloading() to exist on 8.4 or below, right? It would probably break if you tried to call it - suggest putting version guards around the declaration in the stub file

Copy link
Member Author

Choose a reason for hiding this comment

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

This will not be merged in 8.4, so the zend_test_opcache_preloading() function will not exist in 8.4 :)

Copy link
Member

Choose a reason for hiding this comment

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

No, but it appears that zend_test supports running the new versions of the extension with older versions of PHP, since we generate arginfo to support older versions of PHP

Copy link
Member Author

Choose a reason for hiding this comment

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

If that's supported I don't think it's on purpose. To support old versions, adding guards in the stub wouldn't be enough, we would also need conditional directives in the implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Older versions of the arginfo are generated as a smoke test for the generator.

@nielsdos
Copy link
Member

I wonder what the motivation is?

@TimWolla
Copy link
Member

@nielsdos Similar motivation as here: #14479 (comment)

@arnaud-lb arnaud-lb closed this in 32290b3 Aug 6, 2025
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.

4 participants