Skip to content

Commit 2732e35

Browse files
ondrejmirtesondrejmirtes
committed
Do not keep the symbol cache entries of directories after the scan
OptimizedDirectorySourceLocatorFactory kept every file it checked, with its hash, stat signature and symbols, for the rest of the process, so that a file reachable from two locators would be looked at once. A directory is scanned once per process (OptimizedDirectorySourceLocatorRepository), so that kept 5.4 MB for 3,810 files in every Sylius worker with little to reuse. The files that were hashed or scanned are still shared by the locators of one scan, the ones vouched for by their stat signature are checked against each locator's cache - another stat for a file two locators reach. A locator none of whose files waits for the scan is filled in right away, so that on a warm run only one directory's cache entry is in memory at a time. Amp-Thread-ID: https://ampcode.com/threads/T-01a1b250-2318-7374-8dd7-2834f59aa720
1 parent cb9b3e6 commit 2732e35

2 files changed

Lines changed: 88 additions & 37 deletions

File tree

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

Lines changed: 65 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -47,15 +47,6 @@ final class OptimizedDirectorySourceLocatorFactory
4747

4848
private const SCAN_LOCK_POLL_INTERVAL_MICROSECONDS = 50_000;
4949

50-
/**
51-
* The files this process already checked or scanned, as the caches keep them: [content hash,
52-
* stat signature, classes, functions, constants]. The directories a process builds locators for
53-
* can overlap, and a file reachable from two of them is then looked at once.
54-
*
55-
* @var array<string, array{string, string|null, string[], string[], string[]}>
56-
*/
57-
private array $checkedFiles = [];
58-
5950
public function __construct(
6051
private FileNodesFetcher $fileNodesFetcher,
6152
#[AutowiredParameter(ref: '@fileFinderScan')]
@@ -120,6 +111,11 @@ function (array $requests): void {
120111
* Fills in the symbol maps of the locators, each for its files, from their caches and by
121112
* scanning the files that changed since.
122113
*
114+
* Nothing is kept once the scan is done, and a locator none of whose files has to be scanned
115+
* is filled in right away, so that the cache entries are not all in memory at once - on a warm
116+
* run that is every locator. A file reachable from two locators is checked against the cache
117+
* of each, but hashed and scanned at most once.
118+
*
123119
* @param list<array{non-empty-string, string[], OptimizedDirectorySourceLocator}> $requests
124120
*/
125121
private function scan(array $requests): void
@@ -129,8 +125,12 @@ private function scan(array $requests): void
129125
$scanLocks = [];
130126

131127
try {
132-
$cachedEntries = [];
133-
foreach ($requests as $i => [$cacheKey]) {
128+
// the files whose cache entry could not vouch for them: hashed, and scanned when the
129+
// hash did not match either
130+
$checkedFiles = [];
131+
$filesToScan = [];
132+
$waitingForScan = [];
133+
foreach ($requests as [$cacheKey, $files, $locator]) {
134134
$cached = $this->loadCachedSymbols($cacheKey, $variableCacheKey);
135135
// On a cold cache every parallel worker that is not forked from a process that did
136136
// the scan already builds the same locators at once, and would scan the same files.
@@ -150,21 +150,23 @@ private function scan(array $requests): void
150150
}
151151
}
152152

153-
$cachedEntries[$i] = $cached;
154-
}
155-
156-
$filesToScan = [];
157-
foreach ($requests as $i => [, $files]) {
158-
$cached = $cachedEntries[$i] ?? [];
153+
// the entries of the files not waiting for the scan
154+
$checked = [];
155+
$waitsForScan = false;
159156
foreach ($files as $file) {
160-
if (array_key_exists($file, $this->checkedFiles) || array_key_exists($file, $filesToScan)) {
157+
if (array_key_exists($file, $checkedFiles)) {
158+
$checked[$file] = $checkedFiles[$file];
159+
continue;
160+
}
161+
if (array_key_exists($file, $filesToScan)) {
162+
$waitsForScan = true;
161163
continue;
162164
}
163165

164166
$signature = $signatures->get($file);
165167
$cachedFile = $cached[$file] ?? null;
166168
if ($cachedFile !== null && $signature !== null && $cachedFile[1] === $signature) {
167-
$this->checkedFiles[$file] = $cachedFile;
169+
$checked[$file] = $cachedFile;
168170
continue;
169171
}
170172

@@ -174,40 +176,49 @@ private function scan(array $requests): void
174176
}
175177

176178
if ($cachedFile !== null && $cachedFile[0] === $hash) {
177-
$this->checkedFiles[$file] = [$hash, $signature, $cachedFile[2], $cachedFile[3], $cachedFile[4]];
179+
$checked[$file] = $checkedFiles[$file] = [$hash, $signature, $cachedFile[2], $cachedFile[3], $cachedFile[4]];
178180
continue;
179181
}
180182

181183
$filesToScan[$file] = [$hash, $signature];
184+
$waitsForScan = true;
182185
}
183-
}
184186

185-
if ($filesToScan !== []) {
186-
$foundSymbols = $this->symbolFinderInFiles->findSymbols(array_keys($filesToScan), $this->phpVersion->supportsEnums());
187-
foreach ($filesToScan as $file => [$hash, $signature]) {
188-
[$classes, $functions, $constants] = $foundSymbols[$file] ?? [[], [], []];
189-
$this->checkedFiles[$file] = [$hash, $signature, $classes, $functions, $constants];
187+
if ($waitsForScan) {
188+
$waitingForScan[] = [$cacheKey, $files, $locator, $cached, $checked];
189+
} else {
190+
$this->fill($cacheKey, $variableCacheKey, $locator, $cached, $checked);
190191
}
192+
193+
// before the next entry is loaded
194+
unset($cached, $checked);
195+
}
196+
197+
if ($filesToScan === []) {
198+
return;
199+
}
200+
201+
$foundSymbols = $this->symbolFinderInFiles->findSymbols(array_keys($filesToScan), $this->phpVersion->supportsEnums());
202+
foreach ($filesToScan as $file => [$hash, $signature]) {
203+
[$classes, $functions, $constants] = $foundSymbols[$file] ?? [[], [], []];
204+
$checkedFiles[$file] = [$hash, $signature, $classes, $functions, $constants];
191205
}
206+
unset($foundSymbols, $filesToScan);
192207

193-
foreach ($requests as $i => [$cacheKey, $files, $locator]) {
208+
foreach (array_keys($waitingForScan) as $i) {
209+
[$cacheKey, $files, $locator, $cached, $checked] = $waitingForScan[$i];
210+
unset($waitingForScan[$i]);
194211
$entry = [];
195212
foreach ($files as $file) {
196-
if (!array_key_exists($file, $this->checkedFiles)) {
213+
$checkedFile = $checked[$file] ?? $checkedFiles[$file] ?? null;
214+
if ($checkedFile === null) {
197215
continue;
198216
}
199217

200-
$entry[$file] = $this->checkedFiles[$file];
218+
$entry[$file] = $checkedFile;
201219
}
202220

203-
// A warm run finds every file unchanged and does not write anything. A cold miss is
204-
// written even when empty, so that the workers waiting for the lock read it back.
205-
if ($entry !== $cachedEntries[$i]) {
206-
$this->cache->save($cacheKey, $variableCacheKey, $entry);
207-
}
208-
209-
[$classToFile, $functionToFiles, $constantToFile] = $this->changeStructure($entry);
210-
$locator->fillScanned($classToFile, $functionToFiles, $constantToFile);
221+
$this->fill($cacheKey, $variableCacheKey, $locator, $cached, $entry);
211222
}
212223
} finally {
213224
// Release even if scanning or saving throws, so a failing worker cannot leave other
@@ -218,6 +229,23 @@ private function scan(array $requests): void
218229
}
219230
}
220231

232+
/**
233+
* @param non-empty-string $cacheKey
234+
* @param array<string, array{string, string|null, string[], string[], string[]}>|null $cached
235+
* @param array<string, array{string, string|null, string[], string[], string[]}> $entry
236+
*/
237+
private function fill(string $cacheKey, string $variableCacheKey, OptimizedDirectorySourceLocator $locator, ?array $cached, array $entry): void
238+
{
239+
// A warm run finds every file unchanged and does not write anything. A cold miss is
240+
// written even when empty, so that the workers waiting for the lock read it back.
241+
if ($entry !== $cached) {
242+
$this->cache->save($cacheKey, $variableCacheKey, $entry);
243+
}
244+
245+
[$classToFile, $functionToFiles, $constantToFile] = $this->changeStructure($entry);
246+
$locator->fillScanned($classToFile, $functionToFiles, $constantToFile);
247+
}
248+
221249
/**
222250
* Take an exclusive cross-process lock for a directory's symbol scan so that on a cold cache only
223251
* the first worker scans it. Best effort: a null return means locking is unavailable or the wait

‎tests/PHPStan/Reflection/BetterReflection/SourceLocator/OptimizedDirectorySourceLocatorFactoryTest.php‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
use PHPStan\File\FileStatSignatures;
1212
use PHPStan\Php\PhpVersion;
1313
use PHPStan\Testing\PHPStanTestCase;
14+
use function array_keys;
1415
use function clearstatcache;
1516
use function file_put_contents;
1617
use function max;
@@ -98,6 +99,28 @@ public function testBatchFillsEveryLocator(): void
9899
$this->assertCount(1, $sameKey->locateIdentifiersByType(new DefaultReflector($sameKey), new IdentifierType(IdentifierType::IDENTIFIER_CLASS)));
99100
}
100101

102+
public function testBatchFillsCachedLocatorsAndLocatorsWaitingForTheScan(): void
103+
{
104+
file_put_contents($this->directory . '/a.php', '<?php namespace OdslFactoryTest; class First {}');
105+
file_put_contents($this->directory . '/b.php', '<?php namespace OdslFactoryTest; class Second {}');
106+
$this->createFactory()->createByFiles([$this->directory . '/a.php', $this->directory . '/b.php'], 'odsl-factory-test-cached');
107+
108+
file_put_contents($this->directory . '/c.php', '<?php namespace OdslFactoryTest; class Third {}');
109+
$batch = $this->createFactory()->createBatch();
110+
$waiting = $batch->createByFiles([$this->directory . '/a.php', $this->directory . '/c.php', $this->directory . '/b.php'], 'odsl-factory-test-waiting');
111+
$cached = $batch->createByFiles([$this->directory . '/a.php', $this->directory . '/b.php'], 'odsl-factory-test-cached');
112+
$alsoWaiting = $batch->createByFiles([$this->directory . '/c.php', $this->directory . '/a.php'], 'odsl-factory-test-also-waiting');
113+
$batch->scan();
114+
115+
$this->assertCount(3, $waiting->locateIdentifiersByType(new DefaultReflector($waiting), new IdentifierType(IdentifierType::IDENTIFIER_CLASS)));
116+
$this->assertCount(2, $cached->locateIdentifiersByType(new DefaultReflector($cached), new IdentifierType(IdentifierType::IDENTIFIER_CLASS)));
117+
$this->assertCount(2, $alsoWaiting->locateIdentifiersByType(new DefaultReflector($alsoWaiting), new IdentifierType(IdentifierType::IDENTIFIER_CLASS)));
118+
$this->assertSame(
119+
[$this->directory . '/a.php', $this->directory . '/c.php', $this->directory . '/b.php'],
120+
array_keys($this->cache->load('odsl-factory-test-waiting', $this->getVariableCacheKey()) ?? []),
121+
);
122+
}
123+
101124
private function hasClass(string $className): bool
102125
{
103126
// a new factory is a new run: nothing but the cache is shared

0 commit comments

Comments
 (0)