Skip to content

Commit c6ada6f

Browse files
ondrejmirtesclaude
andcommitted
Restart for OPcache even when turbo is already loaded
The restart had a single trigger — the distributed turbo binary being available but not loaded — and the OPcache activation only rode along with it. A php.ini that already loads turbo (a source checkout with a locally built extension) therefore never restarted and ran with OPcache dormant, or with JIT on and fork mode declined because of it. The OPcache configuration is now a trigger of its own: the process restarts whenever the configuration the restart would set differs from the one in effect, whether or not there is an extension to load. A process whose php.ini already matches (or that was restarted) does not restart again; the retry stop moves from the extension-path marker, which keeps its worker-propagation role, to a generic phpstan.restarted marker, since an OPcache-only restart has no extension path to carry. Directives unknown to the running PHP (opcache.jit before 8.0) never count as a difference. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ek2A83bWb3Yf4CRmtvBfi4
1 parent 80324fe commit c6ada6f

3 files changed

Lines changed: 187 additions & 35 deletions

File tree

‎.github/workflows/e2e-tests.yml‎

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1019,12 +1019,20 @@ jobs:
10191019
- name: "Build the turbo extension"
10201020
run: make -C turbo-ext -j"$(nproc)"
10211021

1022+
- name: "Load the turbo extension through php.ini"
1023+
# TurboProcessRestarter re-executes the process with the loaded php.ini
1024+
# to activate OPcache; an extension passed as -d would not survive that,
1025+
# one from the ini scan dir is loaded again
1026+
run: |
1027+
echo "extension=$PWD/turbo-ext/phpstan_turbo.so" | sudo tee "$(php -r 'echo PHP_CONFIG_FILE_SCAN_DIR;')/99-phpstan-turbo.ini"
1028+
php -r 'exit(extension_loaded("phpstan_turbo") ? 0 : 1);'
1029+
10221030
- name: "Verify the arena is actually exercised"
10231031
# Without an active extension ArenaCache is the no-op PHP twin and no
10241032
# shared memory exists at all; without fork the workers do not publish in
10251033
# lockstep. Either would make the test below pass for the wrong reason.
10261034
run: |
1027-
OUTPUT=$(php -d extension="$PWD/turbo-ext/phpstan_turbo.so" bin/phpstan diagnose -c e2e/turbo-arena/phpstan.neon)
1035+
OUTPUT=$(php bin/phpstan diagnose -c e2e/turbo-arena/phpstan.neon)
10281036
echo "$OUTPUT"
10291037
e2e/bashunit -a contains 'Turbo extension: enabled' "$OUTPUT"
10301038
e2e/bashunit -a contains 'Mechanism: fork (pcntl_fork)' "$OUTPUT"
@@ -1035,7 +1043,7 @@ jobs:
10351043
# extension is now always used, so the variable has to be inert -
10361044
# https://github.com/phpstan/phpstan/issues/15143
10371045
run: |
1038-
OUTPUT=$(PHPSTAN_TURBO=0 php -d extension="$PWD/turbo-ext/phpstan_turbo.so" bin/phpstan diagnose -c e2e/turbo-arena/phpstan.neon)
1046+
OUTPUT=$(PHPSTAN_TURBO=0 php bin/phpstan diagnose -c e2e/turbo-arena/phpstan.neon)
10391047
echo "$OUTPUT"
10401048
e2e/bashunit -a contains 'Turbo extension: enabled' "$OUTPUT"
10411049
@@ -1050,17 +1058,16 @@ jobs:
10501058
# must cost sharing, never results - the analysis has to come out exactly
10511059
# as it does with the runner's normal /dev/shm.
10521060
run: |
1053-
SO="$PWD/turbo-ext/phpstan_turbo.so"
10541061
cd e2e/turbo-arena
10551062
rm -rf tmp
1056-
php -d extension="$SO" ../../bin/phpstan analyse --no-progress > "$RUNNER_TEMP/reference.txt" 2>&1 || true
1063+
php ../../bin/phpstan analyse --no-progress > "$RUNNER_TEMP/reference.txt" 2>&1 || true
10571064
tail -5 "$RUNNER_TEMP/reference.txt"
10581065
# a warm disk cache is the worst case: every worker loads and publishes
10591066
# the same entries at the same moment, so duplicates pile up fastest
1060-
php -d extension="$SO" ../../bin/phpstan clear-result-cache
1067+
php ../../bin/phpstan clear-result-cache
10611068
sudo mount -o remount,size=32M /dev/shm
10621069
df -h /dev/shm
1063-
php -d extension="$SO" ../../bin/phpstan analyse --no-progress > "$RUNNER_TEMP/small-shm.txt" 2>&1 || true
1070+
php ../../bin/phpstan analyse --no-progress > "$RUNNER_TEMP/small-shm.txt" 2>&1 || true
10641071
tail -40 "$RUNNER_TEMP/small-shm.txt"
10651072
# bashunit takes the haystack as a single argv entry, and execve caps
10661073
# one argument at 128 KB - a few hundred KB of analysis output only
@@ -1114,19 +1121,27 @@ jobs:
11141121
./configure
11151122
make -j"$(nproc)"
11161123
1124+
- name: "Load both extensions through php.ini"
1125+
# TurboProcessRestarter re-executes the process with the loaded php.ini
1126+
# to activate OPcache; extensions passed as -d would not survive that,
1127+
# ones from the ini scan dir are loaded again
1128+
run: |
1129+
printf 'extension=%s\nextension=%s\n' "$PWD/turbo-ext/phpstan_turbo.so" "$RUNNER_TEMP/forkunsafe/modules/forkunsafe.so" | sudo tee "$(php -r 'echo PHP_CONFIG_FILE_SCAN_DIR;')/99-phpstan-turbo.ini"
1130+
php -r 'exit(extension_loaded("phpstan_turbo") && extension_loaded("forkunsafe") ? 0 : 1);'
1131+
11171132
- name: "Verify the extension wedges a forked child on its own"
11181133
# a plain exit() in a forked child must hang, or the test below would
11191134
# pass for the wrong reason
11201135
run: |
11211136
set +e
1122-
timeout 10 php -d extension="$RUNNER_TEMP/forkunsafe/modules/forkunsafe.so" e2e/fork-unsafe-extension/fork-and-exit.php
1137+
timeout 10 php e2e/fork-unsafe-extension/fork-and-exit.php
11231138
CODE=$?
11241139
set -e
11251140
e2e/bashunit -a equals "124" "$CODE"
11261141
11271142
- name: "Verify fork mode is actually used"
11281143
run: |
1129-
OUTPUT=$(php -d extension="$PWD/turbo-ext/phpstan_turbo.so" -d extension="$RUNNER_TEMP/forkunsafe/modules/forkunsafe.so" bin/phpstan diagnose -c e2e/fork-unsafe-extension/phpstan.neon)
1144+
OUTPUT=$(php bin/phpstan diagnose -c e2e/fork-unsafe-extension/phpstan.neon)
11301145
echo "$OUTPUT"
11311146
e2e/bashunit -a contains 'Turbo extension: enabled' "$OUTPUT"
11321147
e2e/bashunit -a contains 'Mechanism: fork (pcntl_fork)' "$OUTPUT"
@@ -1138,7 +1153,7 @@ jobs:
11381153
run: |
11391154
cd e2e/fork-unsafe-extension
11401155
rm -rf tmp
1141-
OUTPUT=$(../bashunit -a exit_code "0" "timeout 120 php -d extension=$PWD/../../turbo-ext/phpstan_turbo.so -d extension=$RUNNER_TEMP/forkunsafe/modules/forkunsafe.so ../../bin/phpstan analyse --no-progress -vvv")
1156+
OUTPUT=$(../bashunit -a exit_code "0" "timeout 120 php ../../bin/phpstan analyse --no-progress -vvv")
11421157
echo "$OUTPUT"
11431158
../bashunit -a contains 'Mechanism: fork (pcntl_fork)' "$OUTPUT"
11441159
../bashunit -a contains '[OK] No errors' "$OUTPUT"

‎src/Turbo/TurboProcessRestarter.php‎

Lines changed: 103 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -2,38 +2,47 @@
22

33
namespace PHPStan\Turbo;
44

5+
use function explode;
56
use function extension_loaded;
67
use function function_exists;
78
use function get_cfg_var;
9+
use function in_array;
810
use function ini_get;
911
use function is_string;
1012
use function max;
1113
use function pcntl_exec;
1214
use function php_ini_loaded_file;
15+
use function strtolower;
16+
use function trim;
1317
use const PHP_BINARY;
1418

1519
/**
16-
* Restarts the main PHPStan process via pcntl_exec() with the distributed
17-
* turbo extension loaded through -d extension=.
20+
* Restarts the main PHPStan process via pcntl_exec() when the process it
21+
* was started as is not the one PHPStan wants to run — for either of two
22+
* reasons, checked independently.
1823
*
19-
* Parallel analysis prefers pcntl_fork()ed workers over spawned ones (see
24+
* The distributed turbo extension is available but not loaded. Parallel
25+
* analysis prefers pcntl_fork()ed workers over spawned ones (see
2026
* ForkParallelChecker) — a forked worker inherits the booted process instead
2127
* of paying a full re-boot. But a forked worker also inherits the parent's
2228
* loaded extensions: the spawn-time `-d extension=` injection
2329
* (ProcessHelper) cannot reach it, and dl() cannot load a binary from next
24-
* to the phar. So when the extension binary is available but not loaded,
25-
* the whole process is re-executed with it before anything else runs —
26-
* forked workers then inherit the extension, its stub shadowing, and (when
27-
* running from a phar) the phar-fork-guard that keeps phar:// reads safe
28-
* across fork.
30+
* to the phar. So the whole process is re-executed with the extension
31+
* before anything else runs — forked workers then inherit the extension,
32+
* its stub shadowing, and (when running from a phar) the phar-fork-guard
33+
* that keeps phar:// reads safe across fork.
2934
*
30-
* The restarted process also gets OPcache activated — with JIT pinned off,
31-
* whatever the user's ini says — so forked workers share the parent's warm
32-
* opcode cache (see resolveOpcacheArgs()).
35+
* The OPcache configuration in effect is not the one PHPStan wants (see
36+
* resolveOpcacheArgs()) — usually because OPcache is dormant on CLI, or has
37+
* JIT on. The restarted process runs with it activated, JIT pinned off, and
38+
* forked workers share the parent's warm opcode cache. This reason stands
39+
* on its own so that a php.ini which already loads turbo (a source checkout
40+
* with a locally built extension, say) does not leave OPcache dormant just
41+
* because there is no extension left to restart for.
3342
*
34-
* The restart carries a marker ini entry with the extension path. It doubles
35-
* as the retry stop (a binary that failed to load leaves the extension
36-
* unloaded — without the marker the restart would loop) and tells
43+
* The restart carries two marker ini entries: RESTARTED_INI is the retry
44+
* stop (a binary that failed to load, or an OPcache that could not start,
45+
* would otherwise restart forever), and EXTENSION_PATH_INI tells
3746
* TurboExtensionSelector that spawned workers still need the -d flag
3847
* (command-line -d flags, unlike php.ini, are not inherited).
3948
*/
@@ -42,6 +51,9 @@ final class TurboProcessRestarter
4251

4352
public const EXTENSION_PATH_INI = 'phpstan.turboExtensionPath';
4453

54+
/** Set by the restart so the restarted process never restarts again */
55+
public const RESTARTED_INI = 'phpstan.restarted';
56+
4557
/** Shared memory reserved (not touched) for the opcode cache, in MB — see resolveOpcacheArgs() for the sizing */
4658
private const OPCACHE_MEMORY_CONSUMPTION_MB_LIMIT = 256;
4759

@@ -80,11 +92,10 @@ public static function getRestartExtensionPath(): ?string
8092
*/
8193
public static function restartIfSuitable(array $argv): void
8294
{
83-
if (extension_loaded('phpstan_turbo')) {
84-
return;
85-
}
86-
if (self::getRestartExtensionPath() !== null) {
87-
// already restarted and the extension still did not load
95+
if (get_cfg_var(self::RESTARTED_INI) !== false) {
96+
// already restarted — whatever did not take effect (a binary that
97+
// failed to load, an OPcache that could not start) will not on a
98+
// second try either
8899
return;
89100
}
90101
if (
@@ -100,8 +111,12 @@ public static function restartIfSuitable(array $argv): void
100111
return;
101112
}
102113

103-
$extensionPath = TurboExtensionSelector::findExtension();
104-
if ($extensionPath === null) {
114+
$extensionPath = extension_loaded('phpstan_turbo') ? null : TurboExtensionSelector::findExtension();
115+
$opcacheArgs = self::getOpcacheArgs();
116+
if (
117+
$extensionPath === null
118+
&& !self::resolveOpcacheRestartNeeded($opcacheArgs, self::getCurrentIniValues($opcacheArgs))
119+
) {
105120
return;
106121
}
107122

@@ -113,20 +128,82 @@ public static function restartIfSuitable(array $argv): void
113128
}
114129
$args[] = '-d';
115130
$args[] = 'memory_limit=' . ini_get('memory_limit');
116-
foreach (self::getOpcacheArgs() as $opcacheArg) {
131+
foreach ($opcacheArgs as $opcacheArg) {
117132
$args[] = '-d';
118133
$args[] = $opcacheArg;
119134
}
135+
if ($extensionPath !== null) {
136+
$args[] = '-d';
137+
$args[] = 'extension=' . $extensionPath;
138+
$args[] = '-d';
139+
$args[] = self::EXTENSION_PATH_INI . '=' . $extensionPath;
140+
}
120141
$args[] = '-d';
121-
$args[] = 'extension=' . $extensionPath;
122-
$args[] = '-d';
123-
$args[] = self::EXTENSION_PATH_INI . '=' . $extensionPath;
142+
$args[] = self::RESTARTED_INI . '=1';
124143
foreach ($argv as $arg) {
125144
$args[] = $arg;
126145
}
127146

128147
pcntl_exec(PHP_BINARY, $args);
129-
// pcntl_exec() returns only on failure — continue without the extension
148+
// pcntl_exec() returns only on failure — continue as we are
149+
}
150+
151+
/**
152+
* Whether the OPcache configuration the restart would set differs from
153+
* the one in effect — the second reason to restart, independent of turbo:
154+
* with the extension already loaded through php.ini there would be
155+
* nothing else to restart for, and OPcache would stay dormant.
156+
*
157+
* @param list<string> $opcacheArgs `name=value` entries, see resolveOpcacheArgs()
158+
* @param array<string, string|false> $currentIniValues ini_get() of each of those names
159+
*/
160+
public static function resolveOpcacheRestartNeeded(array $opcacheArgs, array $currentIniValues): bool
161+
{
162+
foreach ($opcacheArgs as $opcacheArg) {
163+
[$name, $value] = explode('=', $opcacheArg, 2);
164+
$current = $currentIniValues[$name] ?? false;
165+
if ($current === false) {
166+
// unknown directive on this PHP (opcache.jit before 8.0) — nothing a restart could change
167+
continue;
168+
}
169+
if (self::normalizeIniValue($current) !== self::normalizeIniValue($value)) {
170+
return true;
171+
}
172+
}
173+
174+
return false;
175+
}
176+
177+
/**
178+
* @param list<string> $opcacheArgs
179+
* @return array<string, string|false>
180+
*/
181+
private static function getCurrentIniValues(array $opcacheArgs): array
182+
{
183+
$values = [];
184+
foreach ($opcacheArgs as $opcacheArg) {
185+
$name = explode('=', $opcacheArg, 2)[0];
186+
$values[$name] = ini_get($name);
187+
}
188+
189+
return $values;
190+
}
191+
192+
/**
193+
* The ini parser stores booleans as "1" / "" for php.ini and -d values
194+
* alike; ini_set()-style spellings are folded the same way.
195+
*/
196+
private static function normalizeIniValue(string $value): string
197+
{
198+
$value = strtolower(trim($value));
199+
if (in_array($value, ['', '0', 'off', 'false', 'no', 'none'], true)) {
200+
return '0';
201+
}
202+
if (in_array($value, ['1', 'on', 'true', 'yes'], true)) {
203+
return '1';
204+
}
205+
206+
return $value;
130207
}
131208

132209
/**

‎tests/PHPStan/Turbo/TurboProcessRestarterTest.php‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,4 +108,64 @@ private static function withSizes(int $memory, int $interned, int $files): array
108108
return $args;
109109
}
110110

111+
/**
112+
* @return iterable<string, array{array<string, string|false>, bool}>
113+
*/
114+
public static function dataResolveOpcacheRestartNeeded(): iterable
115+
{
116+
// what the restarted process reads back through ini_get() for the stock args
117+
$inEffect = [
118+
'opcache.enable' => '1',
119+
'opcache.enable_cli' => '1',
120+
'opcache.jit' => 'disable',
121+
'opcache.jit_buffer_size' => '0',
122+
'opcache.validate_timestamps' => '0',
123+
'opcache.file_update_protection' => '0',
124+
'opcache.max_file_size' => '0',
125+
'opcache.file_cache' => '',
126+
'opcache.save_comments' => '1',
127+
'opcache.memory_consumption' => '256',
128+
'opcache.interned_strings_buffer' => '64',
129+
'opcache.max_accelerated_files' => '20000',
130+
];
131+
132+
yield 'already in effect' => [$inEffect, false];
133+
134+
// php.ini spellings the ini parser leaves as-is versus its "1"/"" for booleans
135+
yield 'already in effect, php.ini spellings' => [
136+
['opcache.enable' => 'On', 'opcache.validate_timestamps' => '', 'opcache.jit' => 'Disable'] + $inEffect,
137+
false,
138+
];
139+
140+
yield 'stock CLI: OPcache dormant' => [
141+
['opcache.enable_cli' => '', 'opcache.validate_timestamps' => '1', 'opcache.file_update_protection' => '2', 'opcache.memory_consumption' => '128', 'opcache.interned_strings_buffer' => '8', 'opcache.max_accelerated_files' => '10000'] + $inEffect,
142+
true,
143+
];
144+
145+
yield 'OPcache active but JIT on' => [
146+
['opcache.jit' => 'tracing', 'opcache.jit_buffer_size' => '64M'] + $inEffect,
147+
true,
148+
];
149+
150+
yield 'OPcache active with a file cache' => [
151+
['opcache.file_cache' => '/tmp/opcache'] + $inEffect,
152+
true,
153+
];
154+
155+
// PHP < 8.0 has no JIT directives — nothing a restart could change there
156+
yield 'directive unknown on this PHP' => [
157+
['opcache.jit' => false, 'opcache.jit_buffer_size' => false] + $inEffect,
158+
false,
159+
];
160+
}
161+
162+
/**
163+
* @param array<string, string|false> $currentIniValues
164+
*/
165+
#[DataProvider('dataResolveOpcacheRestartNeeded')]
166+
public function testResolveOpcacheRestartNeeded(array $currentIniValues, bool $expected): void
167+
{
168+
$this->assertSame($expected, TurboProcessRestarter::resolveOpcacheRestartNeeded(self::STOCK_ARGS, $currentIniValues));
169+
}
170+
111171
}

0 commit comments

Comments
 (0)