diff --git a/CHANGELOG.md b/CHANGELOG.md index 71bcc507..94f7f663 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). * [Performance] **Debug mode lets opcache cache untransformed files** — in debug mode the autoloader includes a file the cache records as untransformed by its original path when the record is fresh (same source size and mtime, tracked resources not newer), so opcache caches it instead of compiling it through `php://filter` on every request; woven files, stale records and misses keep the filter (XDebug breakpoints and magic constants of woven files rely on it, a changed source is woven again). Both the filter and the autoloader use one freshness rule, `CachePathManager::queryFreshCacheState()`. `AspectKernel::init()` tracks the aspect files without loading the aspects: their files are resolved through the wrapped composer loaders (`AopComposerLoader::findOriginalFile()`). `AspectContainer::onRegistration()` never autoloads any more: an id whose class is not loaded yet reaches the listener as a candidate. Warm debug request of the fixture project: `init()` ~1.3 ms → ~0.25 ms, an untransformed file ~38 µs → ~10 µs, the whole request 27–40% faster. * [Performance] **Proxy bodies built as AST** — the proxy generators build every intercepted method, function and (static) initialization body directly as syntax tree nodes (`static $__joinPoint` with its `@var` docblock, the `InterceptorInjector::for*()` call and the `__invoke()`), instead of printing each body to a string and parsing it back; the proxy is printed once. Intercepted methods are looked up in a name index built once per woven class, declared types are converted from the syntax tree without a round trip through reflection types, and attribute arguments are cloned by one shared traverser. Generated proxies are byte-identical; proxy generation takes about a third of its former time (weaving case of the transformation benchmark: −43%). `ClassProxyGenerator::getJoinpointInvocationBody()` is replaced by `getJoinpointInvocationStatements()` and `createOriginalMethodCallable()`. @@ -70,6 +71,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). * [Fixed] `cache:warmup:aop` and `debug:advisor` no longer fail with "Cannot rewind a generator that was already run" when more than one include path is configured (#751). **Internal** diff --git a/src/Instrument/AGENTS.md b/src/Instrument/AGENTS.md index 79d4e6d9..d2d0744b 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 (production: woven classes via composer's class map; debug: a fresh untransformed record → original path, so opcache caches it; woven/stale/miss → php://filter) 4. Caching lives in SourceTransformingLoader::filter() — cache hit → cached content emitted as-is (no parsing, no transformers); miss → StreamMetaData + transformer chain → write cache 5. Freshness rule lives in one place: CachePathManager::queryFreshCacheState() (record filemtime/filesize, container isFreshSince(cachedAt); PREBUILT_CACHE trusts records), used by the filter and the debug autoloader @@ -57,7 +57,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 1829a34d..4f4396c5 100644 --- a/src/Instrument/ClassLoading/SourceTransformingLoader.php +++ b/src/Instrument/ClassLoading/SourceTransformingLoader.php @@ -18,7 +18,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; @@ -47,6 +49,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 */ @@ -111,7 +125,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 { @@ -125,6 +140,23 @@ public static function ensureRegistered(AspectContainer $container): void self::$container = $container; self::$cachePathManager = $container->getService(CachePathManager::class); 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 b58981ef..7a5e00be 100644 --- a/src/Instrument/Transformer/WeavingTransformer.php +++ b/src/Instrument/Transformer/WeavingTransformer.php @@ -109,6 +109,7 @@ public function transform(StreamMetaData $metadata): TransformerResult $advisors, $metadata, $class, + $namespace, $parsedSource->isStrictMode(), ); $totalTransformations += (int) $wasClassProcessed; @@ -125,16 +126,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 { try { @@ -165,13 +168,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(); // Intercepted methods are looked up by name several times per method, here and in the proxy generators $methods = ClassProxyGenerator::indexMethods($class); 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 2081d529..97abddac 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 @@ +