Skip to content

perf(instrument): bound the parser-reflection AST cache while weaving - #757

Closed
lisachenko wants to merge 4 commits into
masterfrom
perf/749-bounded-ast-cache
Closed

lisachenko wants to merge 4 commits into
masterfrom
perf/749-bounded-ast-cache

Conversation

@lisachenko

Copy link
Copy Markdown
Member

What and why

Fixes #749.

Trade-off, maintainer decision needed: bounding the AST cache cuts warmup peak memory by 76%, but makes cold warmup about 16–22% slower. #749 expected a speed-up as well. That was measured on bare parseFile() calls and does not hold for a real warmup. The import fix below is independent of the limit and worth keeping either way.

The change

  1. Imports from the block being woven (WeavingTransformer):
    • processSingleClass() takes the ReflectionFileNamespace already iterated in transform() and reads the original imports from it. The second new ReflectionFileNamespace($file, $ns) lookup is gone.
    • That lookup only avoided re-parsing the file because the AST cache happened to hit.
    • Fixes a bug: in a file that declares the same namespace in several blocks, a class from the second (or a later) block got the imports of the first block. New test with fixture _files/duplicate-ns.php; it fails on master.
  2. Bounded cache:
    • SourceTransformingLoader::ensureRegistered() calls ReflectionEngine::setMaximumCachedFiles(MAXIMUM_CACHED_SYNTAX_TREES = 512) when a container is configured. CacheWarmer and filters registered early go through this path too.
    • A limit the application already set is kept. parser-reflection has no getter, so the code checks whether its typed property is initialized.
    • Nothing else in src/ relies on a file's AST staying cached; everything else parses by file name or works on nodes it already holds.
  3. Docs: src/Instrument/AGENTS.md (init flow, plus a rule never to rely on cached ASTs) and CHANGELOG (Performance and Fixed entries).

Numbers

  • Setup: cold CacheWarmer run over vendor/ (3,942 files) with two name-based advisors, PHP 8.5, opcache on, JIT off.
  • Runs: master c3fcd30 and head alternated, 6 each, fresh cache dir per run, medians.
  • Output: woven output over vendor/ is identical to master (786 cache files, apart from the expected absolute __DIR__ paths).
Metric Before After Change
Peak memory 678 MB 162 MB −76%
Warmup wall time 8.94 s 10.33 s +16%
Warmup CPU time 7.96 s 9.69 s +22%
Whole process, real time 9.13 s 10.46 s +15%

Limit candidates (head with the limit set explicitly, 6 rounds alternating with master):

Limit Peak memory CPU time vs master Wall time vs master
64 62 MB ×1.49 ×1.42
256 112 MB ×1.43 ×1.38
512 (chosen) 162 MB ×1.21 ×1.20
1024 240 MB ×1.25 ×1.22
unbounded (no limit set) 678 MB ×1.07 (noise) ×1.06
  • Why it gets slower: freeing the evicted trees, about 3× more GC runs (roughly double the GC time), and ancestor files being parsed again after eviction.
  • Where it matters: a process that weaves fewer than 512 files never evicts anything, so the limit only affects large warmups and large cache-miss requests.
  • Transformation performance group (5 alternating runs each, median ratios): default rules −2.9%, all rules +0.1%, WeavingTransformer +2.1%. All within run-to-run spread (master alone ranged 0.455–0.553).

Options

  1. Keep it as is: −76% memory, +16–22% warmup time. Useful where memory is the constraint, such as containers with a tight memory_limit or a large vendor/ in appDir.
  2. Drop the limit, keep the import fix: remove the setMaximumCachedFiles() call and the constant. Warmup speed stays as on master, and the duplicate-namespace bug is still fixed.
  3. Make the limit opt-in: for example a kernel option, or leave applications to call setMaximumCachedFiles() themselves.

A cleaner long-term fix would be upstream in parser-reflection: a forgetFile() API plus LRU instead of first-in-first-out eviction. The woven file's own AST could then be evicted once it is processed. An experiment with woven files not cached at all gave 128 MB at about +15–20% time.

Unrelated bug found, not fixed here

WeavingTransformer::saveProxyToCache() builds the proxy path from the source file only. When one file declares several woven classes, their proxies overwrite each other in the same cache file (visible in multiple-ns-woven.php).

Tests

  • WeavingTransformerTest::testProxyCopiesImportsOfTheNamespaceBlockDeclaringTheClass with the new fixture _files/duplicate-ns.php: fails on master, passes here.
  • SourceTransformingLoaderTest: two tests in separate processes, one checking that the default limit is applied and one that a limit set by the application is kept.

Checklist

  • The title follows Conventional Commits (type(scope): summary)
  • Tests cover the change
  • composer check passes (phpunit after merging master: 3073 tests, phpstan level 10, php-cs-fixer, run locally)
  • User-facing or breaking changes are in CHANGELOG.md (and the upgrade guide for breaking changes)

🤖 Generated with Claude Code

https://claude.ai/code/session_01KXjD75egSqDNujmNS5kr5c


Generated by Claude Code

claude added 2 commits October 7, 2026 23:05
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KXjD75egSqDNujmNS5kr5c
…t-cache

# Conflicts:
#	CHANGELOG.md
#	src/Instrument/Transformer/WeavingTransformer.php
@codecov

codecov Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.64%. Comparing base (79ec2a5) to head (df206a2).

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #757      +/-   ##
============================================
+ Coverage     97.61%   97.64%   +0.02%     
- Complexity     1830     1831       +1     
============================================
  Files           104      104              
  Lines          4831     4834       +3     
============================================
+ Hits           4716     4720       +4     
+ Misses          115      114       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member Author

Closing without merge: the bounded AST cache makes weaving slower, which defeats the purpose of #749.

Measured: cold cache:warmup:aop over vendor/ (3,942 files), 6 alternating runs each, medians, PHP 8.5, JIT off.

master limit 512
Peak memory 678 MB 162 MB (−76%)
Warmup wall time 8.94 s 10.33 s (+16%)
Warmup CPU time 7.96 s 9.69 s (+22%)
  • Every limit was slower: 64, 256, 512 and 1024 were all 20–49% slower than no limit.
  • Why: freeing the evicted trees, about 3× more GC runs, and ancestor files parsed again after eviction. A process weaving fewer than 512 files would not notice, but large warmups get slower.
  • What could change this: a cheaper approach would need upstream support in parser-reflection, namely a forgetFile() API and LRU instead of first-in-first-out eviction. Even then, an experiment with woven files not cached at all still cost about +15–20% time.

Kept for reference: the branch perf/749-bounded-ast-cache stays as is. Apart from the limit, it contains an independent bug fix that can be picked up separately: a class declared in the second (or a later) block of a namespace that a file declares several times got the imports of the first block in its proxy. The fix reads the imports from the ReflectionFileNamespace being woven instead of looking them up again by file and namespace name. It comes with a regression test and fixture tests/Instrument/Transformer/_files/duplicate-ns.php.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Performance: unbounded parser-reflection AST cache during weaving (671 MB peak, −25% parse time when bounded)

2 participants