perf: faster weave/newInstance and bind without attribute instantiation#259
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughChangesAttribute matching and weaving
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MethodMatch
participant ReflectionMethod
participant AnnotatedMatcher
participant Bindings
MethodMatch->>ReflectionMethod: getAttributes()
MethodMatch->>AnnotatedMatcher: match attribute names
AnnotatedMatcher-->>MethodMatch: matching pointcuts
MethodMatch->>Bindings: bind ordered interceptors
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 2.x #259 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 227 234 +7
===========================================
Files 29 29
Lines 593 601 +8
===========================================
+ Hits 593 601 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@koriym Review of draft PR #259 (performance: bind / weave / newInstance). Separate layer from #260 (per-call FCC). VerdictDirection is sound — keep open, rebase onto post-#260 Do not close as “done by #260”. The wins are additive in total cost: What looks good1. Skip
|
newInstanceArgs($args) |
new $class(...$args) |
|
|---|---|---|
list 0..n |
positional | positional |
| string keys | values in array order (keys ignored for naming) | named arguments |
If any caller passes associative bags that are not parameter names, behavior can change (Error: unknown named parameter). Ray.Di / typical [] or list args are fine.
Suggestion: one unit test with a class that takes ctor args (list and, if you support it, named), and a short note in the PR body that $args must be a list or real parameter names.
R3. MethodMatch::onionOrderMatch — confirm multi-annotation order + inheritance
New nested loop:
- outer:
$method->getAttributes()(declaration order) - inner: pointcuts, match via
===oris_a(..., true)
That preserves onion order for “bind in attribute order” and improves subclass attribute → parent pointcut key (old code keyed only exact $annotation::class). Good.
Worth an explicit test:
- Method with two attributes A then B → interceptors applied in A-then-B order (if both pointcuts exist).
- Pointcut keyed as parent annotation, method has child attribute → still binds (is_a path).
Without (2), the is_a branch is only SA-covered, not behavior-covered.
Suggestions (non-blocking)
S1. hasAnnotationPointcut early-out
Nice. When all pointcuts are non-annotation matchers, skip attribute reflection entirely. Combined with the counting test, this is the bind-time story.
S2. Nested loop complexity
O(attributes × annotation-pointcuts) vs old O(attributes) hash. Fine for normal pointcut counts; if someone registers hundreds of annotation pointcuts, hash-by-name (with a small inheritance pass) could be faster. Not needed now.
S3. assert($instance instanceof $class) moved earlier in Weaver
Moved before _setBindings. Fine; with assertions off in prod, no runtime effect. Keep.
S4. Overlap with #260
| #259 item | After #260 |
|---|---|
| isset bindings in AopCode | likely already there |
| skip attribute newInstance | still unique and valuable |
Weaver classCache + new |
still unique |
| Matcher getAttributes | still unique |
| MethodMatch without getAnnotations() | still unique |
So after rebase, the PR may shrink to “bind/match without instantiating attributes + Weaver instantiate path” — still worth shipping.
S5. Stacking with #260 call path
After both land, re-measure:
- Weaver newInstance (expect perf: faster weave/newInstance and bind without attribute instantiation #259 win still)
- intercepted call (expect Eliminate double dispatch with parent FCC (-47% call time) #260 win; perf: faster weave/newInstance and bind without attribute instantiation #259 should be ~flat)
- getArguments-heavy interceptor (confirm Eliminate double dispatch with parent FCC (-47% call time) #260 lazy ArrayObject still acceptable)
Nits
- PR title is generic (“Improve AOP performance”). Prefer something like “perf: faster weave/newInstance and bind without attribute instantiation” so it doesn’t look like a dupe of Eliminate double dispatch with parent FCC (-47% call time) #260.
- Draft + CodeRabbit skipped — mark ready after rebase for a full bot pass.
Compiler/Weaver@psalm-suppress MixedMethodCallis consistent with dynamicnew $class; fine.
Summary table
| Area | Severity | Note |
|---|---|---|
Rebase onto post-#260 2.x |
Required | Conflicts expected in AopCode |
$args list vs named |
Required (test or docs) | Subtle BC edge |
| Onion order + is_a tests | Recommended | Lock inheritance match |
| Keep open vs close | — | Keep open; complementary to #260 |
| Attribute skip + Weaver cache | Strength | Main reason to merge |
Overall: Approve the approach. Not obsolete. Rebase, add two focused tests (ctor args + annotation inheritance/order), refresh benchmarks, then undraft.
|
@koriym Full review below. As with #260, the Required / Recommended sections are self-contained instructions for the implementing agent. VerdictThe three retained optimizations are the right ones: existence-check matching without attribute instantiation, dynamic Verified during review — no action needed on these:
Required1. Sequence behind #260 and rebase (CI here is a month stale)This branch's merge-base is
2.
|
Keep ray-di#260 ReflectiveMethodInvocation/AopCode. Add tests for list/named ctor args, parent attribute match, annotation onion order. CHANGELOG.
92e0eab to
f57adb7
Compare
koriym
changed the title
Restored Weavers must re-load weaved class files; a stale FQN cache would skip require and fatal in a new process. Document bind-time attribute safety in CHANGELOG.
Document that annotation-order bind now considers attribute inheritance (aligned with ray-di#255); subclass attributes may change interceptor order.
|
@CodeRabbit approve? |
|
Tip For best results, initiate chat on the files or code changes. |
|
@coderabbitai approve? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== PR 259 ==="
gh pr view 259 --repo ray-di/Ray.Aop --json state,isDraft,mergeable,baseRefName,headRefName,mergeStateStatus,commits -q '{state,isDraft,mergeable,baseRefName,headRefName,mergeStateStatus}'
echo "=== PR 259 checks ==="
gh pr checks 259 --repo ray-di/Ray.Aop || true
echo "=== PR 260 ==="
gh pr view 260 --repo ray-di/Ray.Aop --json state,merged,mergedAt,baseRefName -q '{state,merged,mergedAt,baseRefName}'
echo "=== PR 259 files changed ==="
gh pr diff 259 --repo ray-di/Ray.Aop --name-onlyLength of output: 3786 🏁 Script executed: #!/bin/bash
set -uo pipefail
echo "=== PR 260 ==="
gh pr view 260 --repo ray-di/Ray.Aop --json state,mergedAt,baseRefName -q '{state,mergedAt,baseRefName}'
echo "=== PR 259 files changed ==="
gh pr diff 259 --repo ray-di/Ray.Aop --name-only
echo "=== Does 2.x contain AopCode.php with array_flip (post-260 marker)? ==="
gh api repos/ray-di/Ray.Aop/contents/src/AopCode.php?ref=2.x --jq '.content' | base64 -d | grep -n "array_flip\|isset(\$bindings"
echo "=== PR 259 merge base vs 2.x head ==="
gh pr view 259 --repo ray-di/Ray.Aop --json commits -q '.commits[-1].oid'
git -C . rev-parse HEAD 2>/dev/null || trueLength of output: 1586
Everything required is in place and CI is clean on the rebased head. I'm approving. [approve] ᕮ(•ᴥ•)ᕭ |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr view 259 --repo ray-di/Ray.Aop --json state,isDraft,mergeable,baseRefName,headRefName,mergeStateStatus,statusCheckRollup -q '.'
echo "---pr260---"
gh pr view 260 --repo ray-di/Ray.Aop --json state,merged,mergedAt
echo "---checks---"
gh pr checks 259 --repo ray-di/Ray.AopLength of output: 8870
Congrats on getting it landed! ヽ(•‿•)ノ |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -x
gh pr view 259 --repo ray-di/Ray.Aop --json state,isDraft,mergeable,baseRefName,headRefName,mergeStateStatus,commits -q '{state,isDraft,mergeable,baseRefName,headRefName,mergeStateStatus}'
echo "---pr260---"
gh pr view 260 --repo ray-di/Ray.Aop --json state,merged,mergedAt -q '.'
echo "---checks---"
gh pr checks 259 --repo ray-di/Ray.Aop
echo "---weaver classCache/wakeup---"
gh pr diff 259 --repo ray-di/Ray.Aop -- src/Weaver.php | head -100
echo "---changelog diff---"
gh pr diff 259 --repo ray-di/Ray.Aop -- CHANGELOG.mdLength of output: 4425 🏁 Script executed: #!/bin/bash
gh pr view 260 --repo ray-di/Ray.Aop --json state,closed,mergedAt,baseRefName -q '.'
echo "---diff weaver---"
gh pr diff 259 --repo ray-di/Ray.Aop > /tmp/pr259.diff
grep -n "classCache\|__sleep\|__wakeup" -A5 -B5 /tmp/pr259.diff | sed -n '1,150p'
echo "---changelog---"
grep -n "Unreleased" -A40 /tmp/pr259.diff | grep -A40 "CHANGELOG" | head -80
awk '/diff --git a\/CHANGELOG.md/,/diff --git a\/src/' /tmp/pr259.diff | head -100
</shell>Length of output: 6444
All required items from the review are addressed. Approving now. [approve] ♡(◡‿◡✿) ✏️ Learnings added
|
1 participant
Summary
Speed up bind / weave / newInstance (construction path), complementary to #260 (per-call FCC).
new $class(...$args)instead ofReflectionClass::newInstanceArgsgetAttributes(..., IS_INSTANCEOF)— no attributenewInstance()during match/bindgetAnnotations(); onion order +is_afor child attributes; early-out when no annotation pointcutsnew $class(...$args)fornewInstanceRebased onto post-#260
2.x. LazyArrayObject/ call-path work stays with #260 (this PR no longer touchesReflectiveMethodInvocation).Microbenchmarks (pre-#260; re-run after merge if needed)
Tests
newInstanceNote on
$argsPublic type is
list<mixed>. Named-argument bags work at runtime via spread (new $class(...$args)); document that callers should pass a list or real parameter names.Stacking
Summary by CodeRabbit
Performance
Bug Fixes
Tests