Optimize Container and Telescope coroutine state - #556
Conversation
Describe ReplicableContext as the hook that chooses what is installed in a copied coroutine context, rather than requiring every implementation to preserve all state. This covers full inheritance, projected state, and deliberate reset behavior without changing the API. Document the concrete copy behavior on InertiaState and MissingTranslationGroups so each first-party implementation states exactly what crosses the coroutine boundary.
Replace the separate coroutine-local depth, build-stack, resolving-stack, and parameter-override values with one ContainerResolutionState object. Mutate its bounded stacks in place, clone it when coroutine context is copied, and keep parent and child resolution chains independent. Avoid allocating resolution state for idle inspection, cached singleton hits, and parameterized anonymous closure calls. Skip contextual lookup work when no contextual bindings exist, and resolve parameter overrides from the current state once per constructor recipe. Remove the protected per-parameter override helpers in favor of the complete resolveRecipeParameters() extension point. Add focused coverage for state-free paths, depth cleanup, and copied-context isolation.
Group entry queues, update queues, the recursion guard, and deferred-store scheduling in a coroutine-owned RecordingState. Mark the state non-copyable so child coroutines begin with independent queues and schedule their own store while still inheriting the intended batch identity. Append entries and updates directly in constant time instead of rebuilding each queue for every record. Keep empty reads and flushes allocation-free, and remove propagation and request-context filtering tied to the retired scalar Container and Telescope keys. Cover forked and explicitly created children, independent deferred storage, recursion-guard isolation, shared batch identity, and the existing provider behavior.
Record the intentional difference from Laravel's protected per-parameter override helpers. Direct Container subclasses to resolve the complete constructor recipe through resolveRecipeParameters(), which receives the active coroutine resolution state and avoids repeated context reads.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (16)
💤 Files with no reviewable changes (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe container now uses one replicated coroutine-local resolution state. Telescope now uses one coroutine-local recording state. Tests cover closure calls, cached resolution, depth restoration, fork isolation, recording queues, deferred storage, and entry guards. ChangesContainer resolution state
Telescope recording state
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This change consolidates coroutine-local Container and Telescope state while preserving cleanup, child isolation, queue ordering, and dependency-resolution behavior. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Container
participant ContainerResolutionState
participant BoundMethod
Container->>ContainerResolutionState: create or read resolution state
Container->>ContainerResolutionState: track resolution stacks and depth
Container->>BoundMethod: call anonymous closure directly
Container->>ContainerResolutionState: resolve parameters
Container->>ContainerResolutionState: restore state in finally block
sequenceDiagram
participant ParentCoroutine
participant ChildCoroutine
participant Telescope
participant RecordingState
participant EntriesRepository
ParentCoroutine->>Telescope: record parent entry
Telescope->>RecordingState: append parent entry
ParentCoroutine->>ChildCoroutine: fork coroutine
ChildCoroutine->>Telescope: record child entry
Telescope->>RecordingState: append child entry
Telescope->>EntriesRepository: store separate batches
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 48.28% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 58 functions across 13 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Greptile SummaryThis PR consolidates Container and Telescope coroutine bookkeeping into mutable, coroutine-owned state objects to reduce repeated context-array copying.
Confidence Score: 5/5The PR appears safe to merge, with no concrete blocking or independently actionable non-blocking defects established. The consolidated state objects follow the repository’s coroutine copy semantics, and the changed resolution and recording paths retain balanced cleanup and explicit parent-child ownership.
|
| Filename | Overview |
|---|---|
| src/container/src/Container.php | Consolidates resolution bookkeeping, preserves balanced cleanup through finally blocks, and introduces the documented whole-recipe parameter-resolution extension point. |
| src/container/src/ContainerResolutionState.php | Adds independently replicable mutable state for depth, build stacks, resolving stacks, and parameter overrides. |
| src/telescope/src/Telescope.php | Replaces copied context queues and flags with direct mutation of coroutine-owned recording state. |
| src/telescope/src/RecordingState.php | Adds non-copyable queue and lifecycle state so copied child contexts start with independent Telescope recording ownership. |
| src/telescope/src/TelescopeServiceProvider.php | Retains selected recording and batch context inheritance while allowing non-copyable recording state to reset. |
| src/context/src/ReplicableContext.php | Clarifies that implementations define whether replication inherits, projects, or resets state. |
| src/telescope/src/Watchers/RequestWatcher.php | Removes filtering for the obsolete standalone container depth context key. |
| tests/Container/CoroutineSafetyTest.php | Adds coverage for copied-context resolution-state isolation and cleanup behavior. |
| tests/Telescope/Telescope/TelescopeTest.php | Expands coverage for child queue ownership, deferred storage, recursion guards, and inherited batch identity. |
Reviews (1): Last reviewed commit: "Document the container parameter extensi..." | Re-trigger Greptile
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
Container resolution kept depth, build-stack, resolving-stack, and parameter-override state in four coroutine-context values. Stack updates repeatedly read and rewrote arrays through
CoroutineContext, adding measurable work to every transient build, nested resolution, parameter override, and method injection.Telescope used the same pattern for its recording queues, with
array_merge()rebuilding the complete queue for every entry. Its recording guard and deferred-store marker could also cross into a child coroutine even though the child must own its recording lifecycle.This change gives each subsystem one coroutine-owned state object:
ContainerResolutionStateholds all mutable state for one resolution chain. It is cloned when coroutine context is copied, so parent and child stacks remain independent.RecordingStateholds Telescope's queues, recursion guard, and deferred-store marker. It is non-copyable, so a child begins with its own recording lifecycle while retaining the separately managed batch ID.ReplicableContextnow documents that each implementation chooses whether copied context inherits, projects, or resets its state. Existing implementations document their concrete choice.Container extension point
Hypervel no longer exposes Laravel's protected per-parameter override helpers. Container subclasses that customize parameter resolution should override
resolveRecipeParameters(), which receives the currentContainerResolutionStateand resolves the complete parameter list in one pass. This deliberate difference is documented in the Container README.All named Container APIs and their behavior remain unchanged.
Benchmarks
Measurements compare
0.4with this branch on the same machine. Results are medians from alternating baseline and candidate samples with OPcache disabled. The singleton-hit control remains flat, showing that the improvement comes from resolution bookkeeping rather than unrelated cache behavior.0.4makeWith()primitivemakeWith()primitivebuildWith()primitivebuildWith()primitiveOPcache-enabled runs confirmed the same direction, with measured resolution-path improvements between 38.3% and 65.7% and a flat singleton control.
The final anonymous-closure and empty-contextual-registry guards were measured separately against the state-object implementation before those guards:
The state object adds a small bounded amount of memory while a resolving coroutine remains active:
0.4Telescope queue recording changes from repeated full-array copies to direct append:
Verification
composer fix, including formatting, both PHPStan configurations, parallel tests, Testbench, and dogfood tests.Summary by CodeRabbit
Bug Fixes
Documentation