Skip to content
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

Alternative spec implementation of deduplicated incremental delivery #1077

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

yaacovCR
Copy link
Contributor

This is an alternative to my most recent PR #1052 with an eye to describing the algorithm in a bit less detail to decrease the size of the spec edits. We also introduce the idea of an observable stream to which events can be pushed using a callback.

This is an alternative as well to @benjie's most recent proposal at #1074.

The main differences as I see them within the given proposal are as follows.

This PR:

  1. will kick off incremental entries shared between sibling defers and unique to sibling defers in parallel rather than waiting for the former to complete.
  2. generates a path-independent field plan for each field within the operation; implementations can use this to memoize/deduplicate if using defer on list items; as well as persisting this across operations.
  3. provides a single-line difference in implementation between early and delayed execution; the algorithm is otherwise the same.

This is also of course an alternative to #742 -- the original PR without deduplication.

Copy link

netlify bot commented Feb 12, 2024

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit a7ca644
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/6612ca4c23a444000832fb74
😎 Deploy Preview https://deploy-preview-1077--graphql-spec-draft.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@yaacovCR yaacovCR marked this pull request as draft February 12, 2024 15:04
@yaacovCR yaacovCR force-pushed the deduplicate5 branch 2 times, most recently from f8d3cf3 to f20ce04 Compare February 13, 2024 20:09
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Feb 13, 2024

UPDATED 2024-02-22:

I've separated out stream from this PR for the sake of simplicity; the commit adding defer comes at the tail end of this PR, with some restructuring and moving happening in the initial commits.

The diff for the defer commit is +322 and -51, on net an addition of 271 lines.

For comparison, proposal at #1074 has a diff of +328 and −45, on an addition of 283 lines. (That is the total for the entirety of #1074, as that PR is set to be pulled into a branch already containing essentially the same restructuring work.)

So this proposal by length is on par with #1074, but we also get the additional features mentioned above: (1) parallel execution of all incremental entries (including where sibling defers have overlapping and unique fields), (2) a demonstration of the path-independence of field collection and grouped field set partition, and (3) explicit handling of early execution via 1-line if statements.

To make this magic happen, we do have to expand a bit our spec language. In particular, we add the ability to pass around an event emitter to the data execution layer that can be used to defer execution of deferred fragments until the incremental result stream signals that it has released a fragment as pending. We otherwise retain the simple functional style of the remainder of the spec => and we don't perform any mutations.

I will be updating this PR, probably via rebasing. If something significant changes, I will try to highlight it in the comments here, but basically the plan is just to further simplify, cut down on verbiage, etc.

@yaacovCR yaacovCR force-pushed the deduplicate5 branch 8 times, most recently from d1a5495 to c904162 Compare February 21, 2024 18:09
@yaacovCR
Copy link
Contributor Author

Here are some initial visualizations of what the graph in YieldIncrementalResults might look like

For operation:

{
  someFieldA
  ... @defer(label: "IntermediateComponent") {
    someFieldB
    ... @defer(label: "SlowComponent") {
      someFieldC
    }
  }
}
graph TD
    A1((IntermediateComponent)) --> B1{"{ someFieldB }"}
    A1 --> A2((SlowComponent))
    A2 --> B2{"{ someFieldC }"}

For operation:

{
  someFieldA
  ... @defer(label: "Sibling1") {
    someFieldB
    someFieldX
  }
  ... @defer(label: "Sibling2") {
    someFieldB
    someFieldY
  }
}
graph TD
    A1((Sibling1)) --> B1{"{ someFieldX }"}
    A1 --> B2{"{ someFieldB }"}
    A2((Sibling2)) --> B2
    A2 --> B3{"{ someFieldY }"}

yaacovCR added a commit to graphql/graphql-js that referenced this pull request Apr 18, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants