From 3b84b4be46d99126a469a8d86180f91edf726d1a Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 23:05:10 +0000 Subject: [PATCH] perf(instrument): bound the parser-reflection AST cache while weaving ReflectionEngine kept the syntax tree of every woven file and every reflected ancestor for the life of the process. The weaving pipeline now limits it when it is brought up: - SourceTransformingLoader::ensureRegistered() (also used by CacheWarmer) calls ReflectionEngine::setMaximumCachedFiles(512) (MAXIMUM_CACHED_SYNTAX_TREES), unless the application set its own limit before (detected through the uninitialized typed property, as parser-reflection has no getter). - WeavingTransformer::processSingleClass() reads the original imports from the ReflectionFileNamespace iterated in transform() instead of a new ReflectionFileNamespace(file, ns), which relied on a cache hit to avoid re-parsing the file and took the imports of the first block when a file declares the same namespace in several blocks. Cold CacheWarmer run over vendor/ (3,942 files, BenchAspect), PHP 8.5, JIT off, base c3fcd30 vs head interleaved, 6 runs each, fresh cache dir per run, medians: peak memory 678 MB -> 162 MB (-76%) warmup wall 8.94 s -> 10.33 s (+16%) warmup CPU 7.96 s -> 9.69 s (+22%) Limit candidates (head with an explicit limit, 6 interleaved runs, CPU time ratio to base): 64: 62 MB, x1.49; 256: 112 MB, x1.43; 512: 162 MB, x1.21; 1024: 240 MB, x1.25; unbounded: 678 MB, x1.07. Bounding costs time on this workload (freeing evicted trees, more GC runs, ancestors parsed again), contrary to the parse-only estimate of the issue; 512 is the best memory/time trade-off measured. Woven output is byte-identical to base. Transformation performance group, 5 interleaved runs, median ratios base -> head: rewriter default 0.102 -> 0.099, all rules 1.386 -> 1.387, WeavingTransformer 0.483 -> 0.494 (+2.1%, within the run spread). Closes #749 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KXjD75egSqDNujmNS5kr5c --- CHANGELOG.md | 2 ++ src/Instrument/AGENTS.md | 4 +-- .../ClassLoading/SourceTransformingLoader.php | 34 ++++++++++++++++++- .../Transformer/WeavingTransformer.php | 24 +++++++------ .../SourceTransformingLoaderTest.php | 29 ++++++++++++++++ .../Transformer/WeavingTransformerTest.php | 21 ++++++++++++ .../Transformer/_files/duplicate-ns.php | 21 ++++++++++++ 7 files changed, 121 insertions(+), 14 deletions(-) create mode 100644 tests/Instrument/Transformer/_files/duplicate-ns.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 5fcd9292..4a13cdab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,7 @@ See [UPGRADE-4.0.md](UPGRADE-4.0.md) for the migration guide. * [Performance] [BC BREAK] **Lazy transformation pipeline** — every source transformer is a deferred container service, tagged by the `SourceTransformer` interface and assembled in registration order; the stream filter and the transformers are only registered/constructed on the first cache miss (`SourceTransformingLoader::ensureRegistered()`), never on a warm-cache request. The protected `AspectKernel::registerTransformers()` hook (returning transformer instances, used e.g. by AspectMock to swap in its own weaver) is replaced by `AspectKernel::registerTransformerServices(AspectContainer $container)`, which registers deferred container definitions: override it to replace, omit, reorder or extend the built-in transformers, or simply `addLazyService()` an extra `SourceTransformer` service from `configureAop()` to append one. The rewrites of single syntax nodes (`new`, `include`/`require`, `__DIR__`/`__FILE__`) are `NodeRewriter` rules dispatched by one `SyntaxTreeRewriter` walk per file before weaving, instead of transformers walking the tree on their own; register a `NodeRewriter` service to add one. A `PrefilteredNodeRewriter` declares source markers, so files without them skip the rule, and the walk itself when no rule is left: with the default rules most files are not walked at all. * [Performance] [BC BREAK] **Caching moved into the core: `CachingTransformer` removed** — the cache decision now lives directly in `SourceTransformingLoader::filter()`: a usable cache record makes the filter emit the cached file content as-is, without parsing the source, constructing a single transformer or building `StreamMetaData`; only a real miss parses and runs the transformer chain, then persists the result. This also removes the old cached-branch double work of re-parsing the cached file to rebuild the token stream, so the warm-cache fast path now applies in **debug mode** too (freshness checks preserved). `SourceTransformingLoader::transformCode()` returns the overall `TransformerResult` and `addTransformer()` is gone — register transformers as container services instead (`AspectKernel::registerTransformerServices()`). * [Performance] [BC BREAK] **Advisor cache compiled to plain PHP** — the advisor cache is no longer a `serialize()`d blob under `{cacheDir}/_aspect/sha1(FQCN)`. Each aspect's pointcut/advisor graph is now compiled into an includable plain-PHP file returning `['version' => 1, 'advisors' => [...]]` built from nested static constructor expressions (advice leaves are pure-static-data `Interceptor::before(SomeAspect::class, 'method')` lazy facade calls, so including a cache file constructs no aspects and no advice closures), shadowing the aspect source below the cache directory: `{appDir}/src/Aspect/LoggingAspect.php` is cached as `{cacheDir}/src/Aspect/LoggingAspect.cache.php`. Loading a warm advisor cache is a single opcache-friendly `include` with no unserialization. **Cache format bump: clear your AOP cache directory when upgrading** (or re-run `cache:warmup:aop`); stale-format files are rebuilt automatically outside `PREBUILT_CACHE` mode. Aspects whose source is not below `appDir` are loaded directly and never cached. +* [Performance] **Bounded parser-reflection AST cache while weaving** — `SourceTransformingLoader::ensureRegistered()` limits `ReflectionEngine`'s syntax tree cache to `SourceTransformingLoader::MAXIMUM_CACHED_SYNTAX_TREES` (512) files, unless the application already called `ReflectionEngine::setMaximumCachedFiles()`. The cache used to keep the AST of every woven file and reflected ancestor for the life of the process: a cold `cache:warmup:aop` over ~3,900 vendor files peaked at 678 MB and now at 162 MB, at the cost of ~15–20% more warmup time (evicted trees are freed and some ancestors are parsed again); processes weaving fewer files than the limit are unaffected (#749). **Removed** * [BC BREAK] Removed `Features::PARAMETER_WIDENING` and the parameter-widening code path in the proxy generators. The feature was a PHP 7.0/7.1 compatibility aid (parameter type widening, wiki.php.net/rfc/parameter-no-type-variance) that has been a no-op concern since the PHP 7.2 baseline: generated proxies always keep the original parameter types. Remove the flag from `AspectKernel::configureAop()` options if you passed it. @@ -68,6 +69,7 @@ See [UPGRADE-4.0.md](UPGRADE-4.0.md) for the migration guide. * [Fixed] Constructor and field access joinpoints are reentrant: like method and function invocations, they save and restore the outer call state. A Before advice on a repeated `new` sees `getThis() === null` instead of the previous object, a nested `new` of the same class returns the right object, the constructor invocation no longer keeps the created object alive, and an advice touching the same property of another object no longer clobbers the outer access (#672). * [Fixed] Closure advices registered through `PointcutBuilder` get stable advisor ids derived from the expression, the advice kind and the closure's source location instead of a process-wide counter, so changing the order of registrations in `configureAop()` no longer makes cached proxies call the wrong advice or fail with an unknown id (#640). * [Fixed] The weaving metadata files (`_transformation.cache`, `_include.cache`) carry a format version: a cache of another version is rebuilt, and with `Features::PREBUILT_CACHE` it fails with a hint to run `cache:warmup:aop`. Records keep the size and mtime of the woven source, so a source whose mtime moved backwards (rsync `-t`, checkout of an older revision) is woven again instead of serving the stale proxy (#685). +* [Fixed] The proxy of a class declared in the second (or later) block of a namespace that a file declares several times copies the imports of its own block instead of the first one, and the imports are no longer looked up again by file name, which re-parsed the file once its syntax tree was evicted from the parser-reflection cache (#749). **Internal** * [Internal] Tooling: PHPUnit 13 with strict flags, PHPStan 2 at level 10 over `src/` and `tests/`, php-cs-fixer with the PER-CS rule set, and `composer test`/`analyze`/`cs`/`check` scripts. diff --git a/src/Instrument/AGENTS.md b/src/Instrument/AGENTS.md index e976d8b0..18c05f3c 100644 --- a/src/Instrument/AGENTS.md +++ b/src/Instrument/AGENTS.md @@ -2,7 +2,7 @@ ## Init flow 1. AspectKernel::init() — singleton, registers deferred transformer services, calls configureAop() -2. SourceTransformingLoader — PHP stream filter (php://filter/read=go.source.transforming.loader/resource=... protocol); registered lazily on the first cache miss via ensureRegistered() +2. SourceTransformingLoader — PHP stream filter (php://filter/read=go.source.transforming.loader/resource=... protocol); registered lazily on the first cache miss via ensureRegistered(), which also bounds the parser-reflection AST cache (`ReflectionEngine::setMaximumCachedFiles(MAXIMUM_CACHED_SYNTAX_TREES)`, unless the application set a limit). Never rely on a file's AST staying cached: read it from the tree being woven (StreamMetaData::$syntaxTree, the iterated ReflectionFileNamespace), a lookup by file name re-parses an evicted file 3. AopComposerLoader::init() — hooks Composer autoloader → redirects through stream filter 4. Caching lives in SourceTransformingLoader::filter() — cache hit → cached content emitted as-is (no parsing, no transformers); miss → StreamMetaData + transformer chain → write cache @@ -56,7 +56,7 @@ Interceptor list entries are first-class advice callables on the aspect instance GeneratedInterceptor descriptors (string advisor ids are rejected). Short names above are `use` aliases managed by ProxyImports: a name colliding with the original file's imports or body gets an adjusted alias (e.g. `use Go\Aop\Framework\Interceptor as AopInterceptor;`), -see src/Proxy/AGENTS.md. WeavingTransformer passes the original imports to the proxy generator constructors. +see src/Proxy/AGENTS.md. WeavingTransformer passes the original imports (of the namespace block declaring the class) to the proxy generator constructors. ### Key invariants - Proxy re-inherits parent+interfaces via reflection (not from woven source) diff --git a/src/Instrument/ClassLoading/SourceTransformingLoader.php b/src/Instrument/ClassLoading/SourceTransformingLoader.php index 7134c4c1..4d783c23 100644 --- a/src/Instrument/ClassLoading/SourceTransformingLoader.php +++ b/src/Instrument/ClassLoading/SourceTransformingLoader.php @@ -20,7 +20,9 @@ use Go\Instrument\Transformer\SourceTransformer; use Go\Instrument\Transformer\StreamMetaData; use Go\Instrument\Transformer\TransformerResult; +use Go\ParserReflection\ReflectionEngine; use php_user_filter as PhpStreamFilter; +use ReflectionProperty; use RuntimeException; use Throwable; @@ -49,6 +51,18 @@ final class SourceTransformingLoader extends PhpStreamFilter */ public const string FILTER_IDENTIFIER = 'go.source.transforming.loader'; + /** + * Number of syntax trees kept in the parser-reflection cache while weaving + * + * Every woven file and every ancestor reflected during advice matching is parsed through + * ReflectionEngine, which keeps each tree for the life of the process unless a limit is set. + * Chosen by measurement of a cold warmup over ~3,900 vendor files: peak memory drops from + * 678 MB to 162 MB for ~20% more time (evicted trees are freed, some ancestors are parsed + * again); 256 and 64 save little more memory but cost ~45% more time. Processes weaving + * fewer files never evict. The limit applies only when the application did not set its own one. + */ + public const int MAXIMUM_CACHED_SYNTAX_TREES = 512; + /** * String buffer */ @@ -118,7 +132,8 @@ public static function register(string $filterId = self::FILTER_IDENTIFIER): voi * * Idempotent; called from the cache-miss paths (autoloader miss, include rewriting, * cache warmup), so a warm-cache request never registers the filter nor constructs - * any transformer. + * any transformer. Bounds the parser-reflection syntax tree cache when the pipeline is + * brought up, see {@see MAXIMUM_CACHED_SYNTAX_TREES}. */ public static function ensureRegistered(AspectContainer $container): void { @@ -135,6 +150,23 @@ public static function ensureRegistered(AspectContainer $container): void self::$cachePathManager = $container->getService(CachePathManager::class); self::$features = $kernelOptions['features']; self::$transformers = null; + + self::limitSyntaxTreeCache(); + } + } + + /** + * Bounds the syntax tree cache of parser-reflection, unless a limit was set already + * + * ReflectionEngine exposes no getter for the limit: its typed property stays uninitialized + * until setMaximumCachedFiles() is called, so an initialized one is a limit chosen by the + * application (or by a previous registration) and is kept as is. + */ + private static function limitSyntaxTreeCache(): void + { + $limitProperty = new ReflectionProperty(ReflectionEngine::class, 'maximumCachedFiles'); + if (!$limitProperty->isInitialized()) { + ReflectionEngine::setMaximumCachedFiles(self::MAXIMUM_CACHED_SYNTAX_TREES); } } diff --git a/src/Instrument/Transformer/WeavingTransformer.php b/src/Instrument/Transformer/WeavingTransformer.php index 829ea1e8..5cd26136 100644 --- a/src/Instrument/Transformer/WeavingTransformer.php +++ b/src/Instrument/Transformer/WeavingTransformer.php @@ -107,6 +107,7 @@ public function transform(StreamMetaData $metadata): TransformerResult $advisors, $metadata, $class, + $namespace, $parsedSource->isStrictMode(), ); $totalTransformations += (int) $wasClassProcessed; @@ -123,16 +124,18 @@ public function transform(StreamMetaData $metadata): TransformerResult /** * Performs weaving of single class if needed, returns true if the class was processed * - * @param Advisor[] $advisors List of advisors - * @param StreamMetaData $metadata - * @param ReflectionClass $class - * @param bool $useStrictMode If the source file used strict mode, the proxy should too + * @param Advisor[] $advisors List of advisors + * @param StreamMetaData $metadata + * @param ReflectionClass $class + * @param ReflectionFileNamespace $namespace Namespace block of the file that declares the class + * @param bool $useStrictMode If the source file used strict mode, the proxy should too * @return bool */ private function processSingleClass( array $advisors, StreamMetaData $metadata, ReflectionClass $class, + ReflectionFileNamespace $namespace, bool $useStrictMode, ): bool { $advices = $this->adviceMatcher->getAdvicesForClass($class, $advisors); @@ -149,13 +152,12 @@ private function processSingleClass( $newClassName = $class->getShortName() . AspectContainer::ORIGINAL_TRAIT_SUFFIX; $newFqcn = ($class->getNamespaceName() !== '' ? $class->getNamespaceName() . '\\' : '') . $newClassName; - $classFileName = $class->getFileName(); - if ($classFileName === false) { - return false; - } - // Imports of the original file are copied into the proxy (parameter defaults and types rely on - // them); the generators reserve their names and alias their own imports around them - $originalImports = new ReflectionFileNamespace($classFileName, $class->getNamespaceName())->getNamespaceAliases(); + // Imports of the namespace block declaring the class are copied into the proxy (parameter defaults + // and types rely on them); the generators reserve their names and alias their own imports around them. + // They are read from the block being woven: a lookup by file and namespace name would re-parse the file + // once its syntax tree is evicted from the parser-reflection cache, and would return the first block + // when the file declares the same namespace several times + $originalImports = $namespace->getNamespaceAliases(); // For traits: rename the trait (legacy approach, TraitProxyGenerator generates a child trait). // For enums: convert the enum body to a trait (cases extracted to proxy enum by EnumProxyGenerator). diff --git a/tests/Instrument/ClassLoading/SourceTransformingLoaderTest.php b/tests/Instrument/ClassLoading/SourceTransformingLoaderTest.php index ad5bbd53..f1db053b 100644 --- a/tests/Instrument/ClassLoading/SourceTransformingLoaderTest.php +++ b/tests/Instrument/ClassLoading/SourceTransformingLoaderTest.php @@ -20,11 +20,14 @@ use Go\Instrument\Transformer\SourceTransformer; use Go\Instrument\Transformer\StreamMetaData; use Go\Instrument\Transformer\TransformerResult; +use Go\ParserReflection\ReflectionEngine; use Go\PhpUnit\UsesTemporaryDirectory; use LogicException; use PhpToken; +use PHPUnit\Framework\Attributes\RunInSeparateProcess; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; +use ReflectionProperty; #[\PHPUnit\Framework\Attributes\AllowMockObjectsWithoutExpectations] class SourceTransformingLoaderTest extends TestCase @@ -192,6 +195,32 @@ public function testEarlyRegistrationIsConfiguredByTheKernel(): void $this->assertSame(1, $transformer->callCount); } + /** + * The syntax tree cache of parser-reflection is unbounded until a limit is set: weaving bounds it + * (a separate process starts with no limit, which can not be unset once set) + */ + #[RunInSeparateProcess] + public function testRegistrationBoundsTheSyntaxTreeCache(): void + { + $limitProperty = new ReflectionProperty(ReflectionEngine::class, 'maximumCachedFiles'); + $this->assertFalse($limitProperty->isInitialized()); + + $this->registerLoader([]); + + $this->assertSame(SourceTransformingLoader::MAXIMUM_CACHED_SYNTAX_TREES, $limitProperty->getValue()); + } + + #[RunInSeparateProcess] + public function testRegistrationKeepsTheSyntaxTreeCacheLimitOfTheApplication(): void + { + ReflectionEngine::setMaximumCachedFiles(1000); + + $this->registerLoader([]); + + $limitProperty = new ReflectionProperty(ReflectionEngine::class, 'maximumCachedFiles'); + $this->assertSame(1000, $limitProperty->getValue()); + } + public function testTransformerFailureNamesTheTransformerAndTheFile(): void { $failingTransformer = new FailingSourceTransformerStub(); diff --git a/tests/Instrument/Transformer/WeavingTransformerTest.php b/tests/Instrument/Transformer/WeavingTransformerTest.php index 7b540001..9cb14207 100644 --- a/tests/Instrument/Transformer/WeavingTransformerTest.php +++ b/tests/Instrument/Transformer/WeavingTransformerTest.php @@ -103,6 +103,27 @@ public function testMultipleNamespacesInOneFile(): void $this->assertEquals($expected, $actual); } + /** + * A file may declare the same namespace in several blocks, each with its own imports: the proxy of a class + * must copy the imports of the block declaring it, not the ones of the first block of that namespace + */ + public function testProxyCopiesImportsOfTheNamespaceBlockDeclaringTheClass(): void + { + $metadata = $this->loadTestMetadata('duplicate-ns'); + $this->transformer->transform($metadata); + + $this->assertStringContainsString('trait SecondBlockClassOriginalTrait', $metadata->source); + $this->assertStringNotContainsString('FirstBlockClassOriginalTrait', $metadata->source); + $this->assertSame(1, preg_match("/AOP_CACHE_DIR . '(.+)';$/m", $metadata->source, $matches)); + + $proxyContent = (string) file_get_contents('vfs://' . $matches[1]); + $this->assertStringContainsString('class SecondBlockClass', $proxyContent); + $this->assertStringContainsString('use Go\\Stubs\\Collision\\The as Level;', $proxyContent); + $this->assertStringNotContainsString('Go\\Stubs\\Collision\\Interceptor', $proxyContent); + $this->assertStringContainsString('string $level = Level::LEVEL', $proxyContent); + $this->assertPhpCompiles($proxyContent); + } + /** * Do not make anything for code without classes */ diff --git a/tests/Instrument/Transformer/_files/duplicate-ns.php b/tests/Instrument/Transformer/_files/duplicate-ns.php new file mode 100644 index 00000000..36118b9a --- /dev/null +++ b/tests/Instrument/Transformer/_files/duplicate-ns.php @@ -0,0 +1,21 @@ +