Skip to content
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.

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

Add clear the cache of APCu and Opcache #385

Merged
merged 2 commits into from
Sep 28, 2021
Merged

Add clear the cache of APCu and Opcache #385

merged 2 commits into from
Sep 28, 2021

Conversation

sy-records
Copy link
Contributor

Fix #384

If PHP has APCu/OPcache enabled, reloads will be affected when reloading, so you need to clear the cache

@taylorotwell
Copy link
Member

What does this actually fix? The OP of the issue doesn't actually talk about what their problem is. How can I recreate a problem that this code change fixes?

@kevincobain2000
Copy link
Contributor

Hi @taylorotwell
The issue is that php artisan octane:reload won't reflect the code changes when opcache.enable_cli => On.
Hence, opcache_reset() is required at the time of worker reload.

This pull req fixes it by adding the opcache_reset, which will trigger when the deployer does php artisan octane:reload for graceful reload. However, this pull req also means that opcache_reset will be triggered once --max-requests= for the worker has reached.

@sy-records
Copy link
Contributor Author

When cache extensions such as OPcache are enabled, using reload or --watch will cause the new content to not take effect. @taylorotwell

@sy-records
Copy link
Contributor Author

@kevincobain2000 --max-requests will also pull up the process again, and both will call the onWorkerStart callback, so maybe we can add an environment judgment

@kevincobain2000
Copy link
Contributor

kevincobain2000 commented Sep 28, 2021

Thanks @sy-records
It's difficult. I could be wrong below.

  1. Environment judgement won't work - as the env at the time of octane:start on the original worker won't be overridden by new env value at the time of octane:reload.

  2. Secondly, judgement if the reload is due to --max-requests or by the command octane:reload via $argv is also not possible, as the swoole_server reloads with it's own set of arguments after signal is dispatched.


One solution I could think of is to have a singleton or /tmp/ or ./storage/ or Cache that has the timestamp of when octane:reload is hit. And do opcache_reset within a window of one minute of timestamp. One minute should be enough time for signal to dispatch and reload the workers.
Or vice-versa if we can know that worker's max-requests has reached and workers are going to reload..

@sy-records
Copy link
Contributor Author

Environment judgement won't work - as the env at the time of octane:start on the original worker won't be overridden by new env value at the time of octane:reload.

It won't overwrite is right, it's reload, not restart

@taylorotwell taylorotwell merged commit 9ffa49c into laravel:1.x Sep 28, 2021
@sy-records sy-records deleted the cache branch September 29, 2021 00:31
@hughcube
Copy link
Contributor

Why clear opCache when each worker starts? This should be decided by the user (via php.ini or php -d opcache.enable_cli=0)
There is no way to use the optimization brought by opCache every time it is cleared, especially in the scenario of --max-requests=1

@@ -37,6 +37,7 @@ public function __invoke($server, int $workerId)

$this->dispatchServerTickTaskEverySecond($server);
$this->streamRequestsToConsole($server);
$this->clearCache();

Choose a reason for hiding this comment

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

每个 worker 在 start 的时候都跑一遍?

Copy link
Contributor

Choose a reason for hiding this comment

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

是这个意思的, 你的 --max-requests 设置的越大影响越小

@kevincobain2000
Copy link
Contributor

kevincobain2000 commented Dec 16, 2021

@hughcube

Why clear opCache when each worker starts?

Each worker starts upon 1) reloading application or 2) when --max-requests have reached. Ideally, it is better to clear cache when only app is reloaded, and not when --max-requests make the worker reload. However there is no way to judge if worker is reloaded via octane:reload or has reached max requests.

是这个意思的, 你的 --max-requests 设置的越大影响越小

Therefore, yes

@hughcube
Copy link
Contributor

@kevincobain2000
It’s not the point, I think it should be controlled by the user himself
php.ini or php -d opcache.enable_cli=0

@kevincobain2000
Copy link
Contributor

kevincobain2000 commented Dec 16, 2021

@hughcube

I am not sure if I get what you mean by

I think it should be controlled by the user himself
php.ini or php -d opcache.enable_cli=0

You mean control enable/disable of opcache or flush opcache?
Or you mean, Opcache is unnecessary since app is already cached by swoole or rr? I also want to know this, do we really need Opcache and does it add value anymore?

For the apps that have opcache enabled, flushing opcache cannot be controlled by user (in a separate cli), unless the Octane app is restarted. Unlike fpm, where fpm:reload flushes opcache, cli cannot flush opcache. Therefore, in the case of octane:reload (...a new deployment), cache codes need to be flushed.

@hughcube
Copy link
Contributor

@kevincobain2000

I understand what you mean.

We now use docker deployment. When building, the opcode of commonly used php files will be cached first, and set --max-requests=1, so clearing the cache every time I start the worker will increase my overhead.

Clear the cache when octane starts, such as this
php artisan octane:start --clear-opcode-cache=1

Clearing the cache when the worker starts can be used for hot updates, such as this
php artisan octane:start --clear-opcode-cache=2

I think clearing the cache is an option, or even the default, but it is not necessary.

@kevincobain2000
Copy link
Contributor

@hughcube
I agree that clearing the cache code should be optional, especially for the use cases like yours as you are using docker and octane:start is going to flush the cache anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

octane:reload with opcache_reset()
5 participants