Skip to content

Commit 395468a

Browse files
ondrejmirtesclaude
andcommitted
Answer reflection cache misses without a failing include()
FileCacheStorage::load() learned that an entry was never written by letting `@include` fail, which costs about 29us per miss: PHP walks the include_path and builds two warnings only for the @ to discard them. A stat() rules the file out in about 1.4us and adds nothing to the hit path, where the include stats the file anyway. Misses are far from rare. CachedPhpInternalSourceLocator sits behind every file locator, so every class name they fail to resolve asks the cache whether it is a PHP built-in - 114k misses per run on a large project, in every run, warm cache or not. The stubs map is the only source the wrapped locator reads from, so a class it does not list can never have been cached; checking the map first skips the key hashing and the stat() too. The wrapped locator is still asked, so class alias resolution behaves as before. Measured on a large project (10 workers, full analysis): the cache-miss path dropped from 5.0s to 1.95s of fleet time with the guard alone, and the whole run by 2.3s of user CPU across four ABBA pairs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C2cdRMUX8Be44eUzEM6RQ2
1 parent 9b0cede commit 395468a

4 files changed

Lines changed: 117 additions & 0 deletions

File tree

‎src/Cache/FileCacheStorage.php‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,17 @@ public function load(string $key, string $variableKey)
4949
{
5050
[,, $filePath] = $this->getFilePaths($key);
5151

52+
// Letting include() fail is an expensive way to learn that an entry was
53+
// never written: PHP walks the include_path and builds two warnings only
54+
// to have them discarded by the @, about 29us per miss, while the stat()
55+
// that rules the file out costs about 1.4us and is free on the hit path
56+
// (the include stats the file anyway). Misses are not rare - every class
57+
// name that reaches the PHP-internal locator and turns out not to be a
58+
// built-in is one, tens of thousands per run and per worker.
59+
if (!is_file($filePath)) {
60+
return null;
61+
}
62+
5263
return (static function ($variableKey, $filePath) {
5364
$cacheItem = @include $filePath;
5465
if (!$cacheItem instanceof CacheItem) {

‎src/Reflection/BetterReflection/BetterReflectionSourceLocatorFactory.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,7 @@ public function create(): SourceLocator
185185
new PhpInternalSourceLocator($astPhp8Locator, $this->phpstormStubsSourceStubber),
186186
$this->cache,
187187
$this->phpVersion,
188+
$this->phpstormStubsSourceStubber,
188189
));
189190

190191
$locators[] = $this->skipBundledPolyfills(new AutoloadSourceLocator($this->fileNodesFetcher, true));

‎src/Reflection/BetterReflection/SourceLocator/CachedPhpInternalSourceLocator.php‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
use PHPStan\BetterReflection\Reflection\ReflectionEnum;
1212
use PHPStan\BetterReflection\Reflection\ReflectionFunction;
1313
use PHPStan\BetterReflection\Reflector\Reflector;
14+
use PHPStan\BetterReflection\SourceLocator\SourceStubber\PhpStormStubsSourceStubber;
1415
use PHPStan\BetterReflection\SourceLocator\Type\SourceLocator;
1516
use PHPStan\Cache\Cache;
1617
use PHPStan\Internal\ComposerHelper;
@@ -59,6 +60,7 @@ public function __construct(
5960
private SourceLocator $inner,
6061
private Cache $cache,
6162
private PhpVersion $phpVersion,
63+
private PhpStormStubsSourceStubber $stubber,
6264
)
6365
{
6466
}
@@ -70,6 +72,20 @@ public function locateIdentifier(Reflector $reflector, Identifier $identifier):
7072
if ($identifier->isClass() || $identifier->isFunction()) {
7173
$name = strtolower($name);
7274
}
75+
76+
// Almost everything that reaches this locator is a userland class name
77+
// that every earlier locator failed to resolve, and a name the stubs do
78+
// not have can never have been cached - the stubber is the only source
79+
// the wrapped locator reads from. Ruling those out against the stubs map
80+
// is an array lookup, where asking the cache means hashing the key and
81+
// stat()ing a file that will never exist. The wrapped locator is still
82+
// asked: it resolves class aliases before consulting the stubber, so it
83+
// can answer for a name the map does not list - such an answer just does
84+
// not get cached, under a key that would be the alias anyway.
85+
if ($identifier->isClass() && !$this->stubber->hasClass($name)) {
86+
return $this->inner->locateIdentifier($reflector, $identifier);
87+
}
88+
7389
$cacheKey = sprintf('phpinternal-%s-%s', $identifier->getType()->getName(), $name);
7490
$variableCacheKey = $this->getVariableCacheKey();
7591

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace PHPStan\Cache;
4+
5+
use FilesystemIterator;
6+
use Override;
7+
use PHPStan\Internal\DirectoryCreatorException;
8+
use PHPStan\Testing\PHPStanTestCase;
9+
use RecursiveDirectoryIterator;
10+
use RecursiveIteratorIterator;
11+
use function is_dir;
12+
use function rmdir;
13+
use function sys_get_temp_dir;
14+
use function uniqid;
15+
use function unlink;
16+
17+
final class FileCacheStorageTest extends PHPStanTestCase
18+
{
19+
20+
private string $directory;
21+
22+
protected function setUp(): void
23+
{
24+
parent::setUp();
25+
26+
$this->directory = sys_get_temp_dir() . '/' . uniqid('phpstan-cache-', true);
27+
}
28+
29+
#[Override]
30+
protected function tearDown(): void
31+
{
32+
parent::tearDown();
33+
34+
if (!is_dir($this->directory)) {
35+
return;
36+
}
37+
38+
$files = new RecursiveIteratorIterator(
39+
new RecursiveDirectoryIterator($this->directory, FilesystemIterator::SKIP_DOTS),
40+
RecursiveIteratorIterator::CHILD_FIRST,
41+
);
42+
foreach ($files as $file) {
43+
if ($file->isDir()) {
44+
rmdir($file->getPathname());
45+
continue;
46+
}
47+
48+
unlink($file->getPathname());
49+
}
50+
51+
rmdir($this->directory);
52+
}
53+
54+
public function testMissingEntry(): void
55+
{
56+
$storage = new FileCacheStorage($this->directory);
57+
58+
$this->assertNull($storage->load('never-written', 'v1'));
59+
}
60+
61+
/**
62+
* @throws DirectoryCreatorException
63+
*/
64+
public function testEntrySavedAfterAMissIsLoadedBack(): void
65+
{
66+
$storage = new FileCacheStorage($this->directory);
67+
68+
// the miss comes first on purpose: load() stat()s the file to avoid a
69+
// failing include(), and PHP's stat cache must not answer the second
70+
// load() with what the first one saw
71+
$this->assertNull($storage->load('key', 'v1'));
72+
$storage->save('key', 'v1', ['data' => 1]);
73+
74+
$this->assertSame(['data' => 1], $storage->load('key', 'v1'));
75+
}
76+
77+
/**
78+
* @throws DirectoryCreatorException
79+
*/
80+
public function testEntryWithDifferentVariableKey(): void
81+
{
82+
$storage = new FileCacheStorage($this->directory);
83+
$storage->save('key', 'v1', ['data' => 1]);
84+
85+
$this->assertNull($storage->load('key', 'v2'));
86+
$this->assertSame(['data' => 1], $storage->load('key', 'v1'));
87+
}
88+
89+
}

0 commit comments

Comments
 (0)