-
-
Notifications
You must be signed in to change notification settings - Fork 570
Optimize Deferred execution
#1805
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
SummaryThis PR optimizes memory usage for deferred execution while preserving the DataLoader pattern that was broken in #1790. ProblemPR #1790 attempted to optimize memory but broke DataLoader batching - batch loads executed incrementally before all IDs were collected, causing null values with 600+ items. SolutionReduces memory usage by replacing closure-based queue storage with direct object/array storage:
Tests Added
Verification
|
Store promise references and arrays in the queue instead of closures to reduce memory footprint. This approach: - Stores the executor callable on the SyncPromise instance rather than capturing it in a closure - Queues the promise reference itself (smaller than a closure) - Uses arrays instead of closures in enqueueWaitingPromises() - Preserves DataLoader batching pattern (no premature queue processing) Key changes: - Add $executor property to SyncPromise for storing the executor - Update __construct() to store executor and queue $this - Update runQueue() to handle promise refs, arrays, and callables - Add processWaitingTask() for processing waiting promise arrays - Update enqueueWaitingPromises() to queue arrays instead of closures This maintains full compatibility with the DataLoader pattern since all Deferred executors are still queued BEFORE any are executed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@shadowhand @zimzat @joshbmarshall Can you give this a review? |
|
I loaded it up on the system that was having problems with v15.27.0 It still has issues with v15.27.0 tag, but this is working for me at 1954f81 For my query, the memory usage is identical for this pull request as for master branch. But I think it's because I have nested resolvers: event date > event > event dates I think it's waiting for all the event dates to be queued before resolving, which is how I'd expect it to be. I don't think that can be managed! |
|
@spawnia This looks promising; I don't see any issues with load order or breaking batches. Below are some extra thoughts based on my own trial-error: The different signatures in the queue feels concerning. I had started experimenting with the idea of switching out the anonymous functions with invokable classes. Your implementation of Using https://gist.github.com/zimzat/310e9121225ef18640fad2f4ab675ad4 |
Reduces peak memory usage by using SplFixedArray instead of regular arrays for promise waiting queue items. SplFixedArray has a lower memory footprint for fixed-size data. Addresses feedback from @zimzat in #1805 (comment) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@zimzat I did follow up on your suggestion to use Can you elaborate on what specifically you find concerning? The methods in |
The implicit array shape versus an explicit type check of a named and structured class. It's not a big deal, especially here and internally to the class (like you've said, this isn't technically a public API). The |
Store waiting task data (callbacks, state, result) directly on the child SyncPromise instead of wrapping in SplFixedArray. This simplifies the queue's generic type from `SplQueue<self|SplFixedArray<mixed>>` to `SplQueue<self>`. Memory benchmark shows ~6.5% reduction for Deferred + then() chains: - Single Deferred: 528 → 608 bytes (+80, due to new properties) - Single then(): 888 → 792 bytes (-96, no SplFixedArray wrapper) - 5000 Deferred + then(): 9.6MB → 9.0MB (-620KB net savings) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@zimzat I did another round which should hopefully alleviate your concern. Benchmarks look good too, what do you think? |
|
@spawnia That looks nice; a simple signature, self-contained handling, and still a massive drop in memory usage in the simple many deferred (no then) scenario. Thanks! |
Benchmark Results SummaryRan benchmarks comparing this branch against Key Changes
Memory Improvements
Execution Time
SummaryThe branch achieves significant memory savings (up to 9%) for heavy deferred workloads while also improving execution time for the most demanding benchmarks ( The microbenchmarks with simple promise chains show some CPU overhead, likely due to structural changes that benefit memory at a small cost to simple cases. For real-world GraphQL queries with many nested deferreds, the branch performs better overall. |
|
Next steps for me:
|
|
Lookin' good! |
|
I tested this in a private project with a couple of thousand GraphQL tests, no issues. Can't say anything about real life impact though. |
|
Lighthouse and a large project of mine work. |
|
Released with https://github.com/webonyx/graphql-php/releases/tag/v15.29.0. |
|
very early at the moment, but a number of my sites have gone down because of this update. I've reverted back to v15.28.0 for now which gets them going. I'll have to get you details but this is causing issues |
|
My sentry is reporting: Call to undefined method GraphQL\Deferred::runQueue() vendor/overblog/dataloader-php/lib/promise-adapter/src/Adapter/WebonyxGraphQLSyncPromiseAdapter.php in Overblog\PromiseAdapter\Adapter\WebonyxGraphQLSyncPromiseAdapter::await at line 115 public function await($promise = null, $unwrap = false)
{
if (null === $promise) {
Deferred::runQueue();
SyncPromise::runQueue();
$this->cancellers = [];
return null;
}
$promiseAdapter = $this->getWebonyxPromiseAdapter(); |
This reverts commit 0685035.
| public const PENDING = 0; | ||
| public const FULFILLED = 1; | ||
| public const REJECTED = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋🏾 I think this is BC break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://github.com/webonyx/graphql-php?tab=readme-ov-file#versioning:
Elements that belong to the public API of this package are marked with the @api PHPDoc tag. Those elements are thus guaranteed to be stable within major versions. All other elements are not part of this backwards compatibility guarantee and may change between minor or patch versions.
Resolves #1803
Follow up to #1790