perf: optimize completion loops #4053
Conversation
✅ Deploy Preview for compassionate-pike-271cb3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋 Supported commandsPlease post this commands in separate comments and only one per comment:
|
|
|
yaacovCR
added
the
PR: polish 💅
yaacovCR
force-pushed
the
optimize-loop
branch
3 times, most recently
from
63e8957
to
50b3fef
Compare
following graphql/graphql-spec#1077 now part of the following PR stack, with the laters PRs extracted from this one #4026: incremental: introduce GraphQLWrappedResult to avoid filtering #4050: perf: allow skipping of field plan generation #4051: perf: introduce completePromisedListItemValue #4052: refactor: introduce completeIterableValue #4053: perf: optimize completion loops #4046: perf: use undefined for empty
yaacovCR
force-pushed
the
optimize-loop
branch
2 times, most recently
from
1873373
to
49d1882
Compare
introducing separate looping functions for when streamUsage is defined
|
After offline discussion with @robrichard i am closing this one for now as not worth the extra slight performance bump by our very synthetic benchmarks, in the very long synchronous list case we’re already faster than the first 17 alpha release and in the asynchronous case we’re still slower either way secondary to incremental delivery, so the small difference from this PR doesn’t really change our performance story overall in the short term, but not immediate future we can think about #4043 as the real path away from the performance penalty involved with incremental delivery although the current plan is not to proceed with that immediately as during the pre-and presumably immediate post release. After incremental delivery, there is significant value in having our reference and limitation be more aligned with the specification changes. #4043 add an optimization that diverges somewhat from the specification, and so we may adopt it within the reference implementation, but the thought is at least not to do so immediately |
by hoisting streamUsage checks out of the loop, requires introducing two versions of the looping functions
depends on #4052