perf(amp): avoid fibers in future adapter - #8
Draft
simPod wants to merge 46 commits into
Draft
Conversation
When scalar overrides are not set explicitly, they are discovered by scanning the types config. Since the executor resolves built-in scalars through Schema::getType() on essentially every operation, the first such lookup fully resolves a lazily provided types callable - silently making the documented lazy schema pattern (typeLoader + lazy types) eager for every schema, including those that define no overrides at all. Passing scalar overrides explicitly (including an empty array for none) through the new scalarOverrides option skips the scan entirely, keeping lazy type loading lazy. The default behaviour of discovering overrides by scanning types is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Lexer::readString previously decoded and validated one UTF-8 character at a time via Lexer::readChar(). Replace this with a bulk scan using strcspn() to copy runs of bytes that need no special handling (i.e. anything other than a quote, backslash, or control character) in a single native call, falling back to the original per-character logic only for quotes, line terminators, control characters, and escape sequences. Care is taken to keep $this->position (a decoded-character count, not a byte count) accurate for multi-byte UTF-8 content. TAB (0x09), a legal, unescaped SourceCharacter per the GraphQL spec, is excluded from the stop-byte set so it is scanned through like any other ordinary character instead of being individually inspected. Visitor::visitInParallel previously called extractVisitFn() to resolve the enter/leave callback for the current visitor and node kind on every single node, for every wrapped visitor - an O(nodes x visitors) number of array lookups, even though a callback's identity for a given (visitor, kind, direction) triple never changes during a traversal. Since NodeKind exposes a small, fixed set of kinds, precompute this mapping once per call (O(visitors x kinds)) instead. Additionally, replace func_get_args() plus argument unpacking in the enter/leave dispatcher closures with the five explicit, typed parameters they are always called with (matching Visitor::visit()'s single call site), avoiding func_get_args()'s per-call overhead. Add a regression test for the TAB-handling edge case described above.
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…eprecated-input-values
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ted-input-values Fix client schema deprecated input values
🤖 Generated with Claude Code
The enter/leave callback caches were previously prefilled only for NodeKind constants, silently skipping generic and kind-specific visitors for Nodes with custom (non-NodeKind) kinds. Switch to lazy, per-direction caches that resolve and store a callback (or `false` as a "no callback" sentinel) on first use, keyed by any kind string. Also fixes a pre-existing undefined-array-key warning in visit() when traversing a Node with a custom kind. Add regression tests covering generic and kind-specific visitors for a custom Node kind.
- Use a HEREDOC for the tab-character test source, deriving the expected value from it via substr() so input and expectation can't drift apart. - Type-hint $path/$ancestors as array in visitInParallel()'s enter/leave closures, and document $key/$parent via @phpstan-param since PHP 7.4 doesn't support union types. - Add @var docblocks describing the shape of the enter/leave callback caches. - Replace the lazily-built $stopBytes runtime computation in Lexer::readString() with a precomputed STRING_STOP_BYTES constant.
The visit() undefined-array-key warning fix for custom Node kinds, along with its regression tests, has been split out into a separate PR (webonyx#1952) since it is unrelated to the Lexer/Visitor optimization work here.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Deep checks (must be a ScalarType named after a built-in scalar) now run during schema validation rather than in the SchemaConfig setter. The setter keeps only a dev-time assert to catch basic misuse and narrow the type for the name-keyed map, keeping runtime config cheap. Also catches misuse via the public $scalarOverrides property, which bypasses the setter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Visitor::visitInParallel() enter/leave callback caching optimization has been split out into a separate PR (webonyx#1953), so it can be reviewed independently of the Lexer::readString optimization here.
…erride-scan Allow setting per-schema scalar overrides explicitly via SchemaConfig
🤖 Generated with Claude Code
🤖 Generated with Claude Code
PHP's switch uses loose comparison, so `case null` also matches code 0, causing a backslash followed by a raw NUL byte to be treated as EOF instead of an invalid escape sequence. Check for EOF with a strict comparison before the switch instead.
There was a problem hiding this comment.
Pull request overview
This PR refactors AmpFutureAdapter to avoid creating a new AMPHP Fiber for each promise continuation and aggregate operation by composing Futures via map()/catch() and settling a single DeferredFuture instead. This targets lower allocation and event-loop scheduling overhead for wide asynchronous GraphQL result graphs.
Changes:
- Rework
AmpFutureAdapter::then()andAmpFutureAdapter::all()to compose/aggregate futures withoutAmp\async()/Amp\Future\await(). - Add tests for future unwrapping behavior, rejection handling, key preservation in
all(), and aggregate rejection. - Document the performance fix in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/Executor/Promise/AmpFutureAdapterTest.php | Adds/updates coverage for nested-future unwrapping, rejection semantics, key preservation, and aggregate rejection behavior. |
| src/Executor/Promise/Adapter/AmpFutureAdapter.php | Refactors then()/all() to use DeferredFuture + map()/catch() instead of spawning fibers via async(). |
| CHANGELOG.md | Notes the behavioral/performance fix for the Amp adapter under “Unreleased”. |
Comments suppressed due to low confidence (2)
tests/Executor/Promise/AmpFutureAdapterTest.php:185
- AmpFutureAdapter::then() invokes the rejection callback with the rejection Throwable. This zero-argument closure will throw an ArgumentCountError when called, preventing the test from asserting the recovered value.
$resultPromise = $ampAdapter->then(
$promise,
null,
static fn (): Future => $deferred->getFuture()
);
tests/Executor/Promise/AmpFutureAdapterTest.php:100
- AmpFutureAdapter::then() passes the fulfillment value and rejection reason into the respective callbacks. Both callbacks here declare no parameters, so the test will fail with ArgumentCountError rather than asserting the intended RuntimeException behavior.
static function (): void {
throw new \RuntimeException('fulfillment failed');
},
static fn (): string => 'recovered'
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
simPod
force-pushed
the
perf/avoid-fiber-per-amp-continuation
branch
from
July 24, 2026 13:36
de61cc1 to
8857c3d
Compare
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
simPod
force-pushed
the
perf/avoid-fiber-per-amp-continuation
branch
from
July 25, 2026 07:31
67ab737 to
79f8304
Compare
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Cover the single-char escape sequences (\/, \b, \f, \n, \r, \t), the & punctuator token, and multi-byte UTF-8 in readChar(). 🤖 Generated with Claude Code
…sing Optimize Lexer::readString
🤖 Generated with Claude Code
# Conflicts: # CHANGELOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
AmpFutureAdaptercreated an AMPHP Fiber for every promise continuation and aggregate. Wide GraphQL result graphs therefore retained many Fiber states and event-loop callbacks even when the underlying Futures only represented batched work.Decision
Compose and aggregate Futures through
map()andcatch()callbacks, settling oneDeferredFuturewhile retaining Promise fulfillment, rejection, and nested-Future flattening semantics.Consequences
Benchmark
On the same wide GraphQL result graph with 838 parent objects and three Future-backed fields:
Relative to the existing AMPHP adapter, this reduces CPU time by about 19% and peak memory by about 46%. The remaining overhead compared with the synchronous adapter is about 13-15% CPU and 22% memory.