extract out publisher #3784
Closed
extract out publisher #3784
Conversation
This file contains 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
✅ Deploy Preview for compassionate-pike-271cb3 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
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
added a commit
to yaacovCR/graphql-js
that referenced
this pull request
Nov 24, 2022
depends on graphql#3784 The proposed new publisher does not use the event loop to manage AsyncRecord dependencies => and so if multiple items within a stream are released from publishing because their parent has just been published, they are all released at once. Another difference is that different sets are used to store the AsyncRecords that are pending vs ready for publishing, etc. This provides a performance benefit in that on a call to next, the set of all AsyncRecords is not inspected. As a side-effect of this change, the incremental array is ordered by which items are ready for delivery first, and not by the initial document. The subscribe algorithm used does not use Promise.race -- this may also be beneficial as the implementation of Promise.race within V8 has a known memory leak for long-running promises. (see https://bugs.chromium.org/p/v8/issues/detail?id=9858)
yaacovCR
added a commit
to yaacovCR/graphql-js
that referenced
this pull request
Nov 25, 2022
depends on graphql#3784 The proposed new publisher does not use the event loop to manage AsyncRecord dependencies => and so if multiple items within a stream are released from publishing because their parent has just been published, they are all released at once. Another difference is that different sets are used to store the AsyncRecords that are pending vs ready for publishing, etc. This provides a performance benefit in that on a call to next, the set of all AsyncRecords is not inspected. As a side-effect of this change, the incremental array is ordered by which items are ready for delivery first, and not by the initial document. The subscribe algorithm used does not use Promise.race -- this may also be beneficial as the implementation of Promise.race within V8 has a known memory leak for long-running promises. (see https://bugs.chromium.org/p/v8/issues/detail?id=9858)
yaacovCR
added a commit
to yaacovCR/graphql-js
that referenced
this pull request
Nov 25, 2022
depends on graphql#3784 The proposed new publisher does not use the event loop to manage AsyncRecord dependencies => and so if multiple items within a stream are released from publishing because their parent has just been published, they are all released at once. Another difference is that different sets are used to store the AsyncRecords that are pending vs ready for publishing, etc. This provides a performance benefit in that on a call to next, the set of all AsyncRecords is not inspected. As a side-effect of this change, the incremental array is ordered by which items are ready for delivery first, and not by the initial document. The subscribe algorithm used does not use Promise.race -- this may also be beneficial as the implementation of Promise.race within V8 has a known memory leak for long-running promises. (see https://bugs.chromium.org/p/v8/issues/detail?id=9858)
yaacovCR
added a commit
to yaacovCR/graphql-js
that referenced
this pull request
Nov 25, 2022
depends on graphql#3784 The proposed new publisher does not use the event loop to manage AsyncRecord dependencies => and so if multiple items within a stream are released from publishing because their parent has just been published, they are all released at once. Another difference is that different sets are used to store the AsyncRecords that are pending vs ready for publishing, etc. This provides a performance benefit in that on a call to next, the set of all AsyncRecords is not inspected. As a side-effect of this change, the incremental array is ordered by which items are ready for delivery first, and not by the initial document. The subscribe algorithm used does not use Promise.race -- this may also be beneficial as the implementation of Promise.race within V8 has a known memory leak for long-running promises. (see https://bugs.chromium.org/p/v8/issues/detail?id=9858)
yaacovCR
force-pushed
the
extract-publisher
branch
from
26002c8 to
189519a
Compare
yaacovCR
added a commit
to yaacovCR/graphql-js
that referenced
this pull request
Nov 25, 2022
Depends on graphql#3784 The proposed new Publisher: 1. does not use the event loop to manage AsyncRecord dependencies 2. uses separate sets to store pending vs ready AsyncRecords 3. does not use `Promise.race` ## No event loop for managing AsyncRecord dependencies The current Publisher wraps every AsyncRecord result in a promise that only resolves after the parent AsyncRecord resolves. If multiple items within a stream are released for publishing because their parent has just been published, the stream cannot be in its entirety until after all of these promises unwind. The new Publisher keeps track of dependencies manually. When an AsyncRecord is pushed by the publisher, all of its dependent AsyncRecords are synchronously pushed, repeating as necessary, without using the event loop. ## Separate sets for pending vs ready AsyncRecords The current publisher inspects all pending AsyncRecords whenever any of them resolves. All that are completed are added to the response. The new Publisher moves AsyncRecords from the pending set to the ready set as they are pushed, so that on each call to next, only the ready set must be iterated. As a side-effect of this change, the incremental array is ordered by which items are ready for delivery first, and not by the initial document. This seems like a worthwhile tradeoff, and is still adherent to the spec, as far as I can tell. ## No `Promise.race` The old Publisher uses `Promise.race` as the trigger to determine whether payloads are ready. The new Publisher uses a single triggering promise that is triggered whenever an AsyncRecord is pushed, and then reset. This may be beneficial as the implementation of `Promise.race` within V8 has a known memory leak for long-running promises. (see https://bugs.chromium.org/p/v8/issues/detail?id=9858). An alternative would be to utilize @brainkim 's memory-safe version detailed in that issue.
yaacovCR
added a commit
to yaacovCR/graphql-spec
that referenced
this pull request
Nov 27, 2022
|
This PR corresponds to a proposed PR on the existing "incremental delivery" spec PR: |
yaacovCR
added a commit
to yaacovCR/graphql-js
that referenced
this pull request
Nov 27, 2022
Depends on graphql#3784 The proposed new Publisher: 1. does not use the event loop to manage AsyncRecord dependencies 2. uses separate sets to store pending vs ready AsyncRecords 3. does not use `Promise.race` ## No event loop for managing AsyncRecord dependencies The current Publisher wraps every AsyncRecord result in a promise that only resolves after the parent AsyncRecord resolves. If multiple items within a stream are released for publishing because their parent has just been published, the stream cannot be in its entirety until after all of these promises unwind. The new Publisher keeps track of dependencies manually. When an AsyncRecord is pushed by the publisher, all of its dependent AsyncRecords are synchronously pushed, repeating as necessary, without using the event loop. ## Separate sets for pending vs ready AsyncRecords The current publisher inspects all pending AsyncRecords whenever any of them resolves. All that are completed are added to the response. The new Publisher moves AsyncRecords from the pending set to the ready set as they are pushed, so that on each call to next, only the ready set must be iterated. As a side-effect of this change, the incremental array is ordered by which items are ready for delivery first, and not by the initial document. This seems like a worthwhile tradeoff, and is still adherent to the spec, as far as I can tell. ## No `Promise.race` The old Publisher uses `Promise.race` as the trigger to determine whether payloads are ready. The new Publisher uses a single triggering promise that is triggered whenever an AsyncRecord is pushed, and then reset. This may be beneficial as the implementation of `Promise.race` within V8 has a known memory leak for long-running promises. (see https://bugs.chromium.org/p/v8/issues/detail?id=9858). An alternative would be to utilize @brainkim 's memory-safe version detailed in that issue.
yaacovCR
added a commit
to yaacovCR/graphql-js
that referenced
this pull request
Nov 27, 2022
Depends on graphql#3784 The proposed new Publisher: 1. does not use the event loop to manage AsyncRecord dependencies 2. uses separate sets to store pending vs ready AsyncRecords 3. does not use `Promise.race` ## No event loop for managing AsyncRecord dependencies The current Publisher wraps every AsyncRecord result in a promise that only resolves after the parent AsyncRecord resolves. If multiple items within a stream are released for publishing because their parent has just been published, the stream cannot be in its entirety until after all of these promises unwind. The new Publisher keeps track of dependencies manually. When an AsyncRecord is pushed by the publisher, all of its dependent AsyncRecords are synchronously pushed, repeating as necessary, without using the event loop. ## Separate sets for pending vs ready AsyncRecords The current publisher inspects all pending AsyncRecords whenever any of them resolves. All that are completed are added to the response. The new Publisher moves AsyncRecords from the pending set to the ready set as they are pushed, so that on each call to next, only the ready set must be iterated. As a side-effect of this change, the incremental array is ordered by which items are ready for delivery first, and not by the initial document. This seems like a worthwhile tradeoff, and is still adherent to the spec, as far as I can tell. ## No `Promise.race` The old Publisher uses `Promise.race` as the trigger to determine whether payloads are ready. The new Publisher uses a single triggering promise that is triggered whenever an AsyncRecord is pushed, and then reset. This may be beneficial as the implementation of `Promise.race` within V8 has a known memory leak for long-running promises. (see https://bugs.chromium.org/p/v8/issues/detail?id=9858). An alternative would be to utilize @brainkim 's memory-safe version detailed in that issue.
yaacovCR
force-pushed
the
extract-publisher
branch
3 times, most recently
from
064a5e9 to
f92d86d
Compare
yaacovCR
added a commit
to yaacovCR/graphql-js
that referenced
this pull request
Jan 3, 2023
Depends on graphql#3784 The proposed new Publisher: 1. does not use the event loop to manage AsyncRecord dependencies 2. uses separate sets to store pending vs ready AsyncRecords 3. does not use `Promise.race` ## No event loop for managing AsyncRecord dependencies The current Publisher wraps every AsyncRecord result in a promise that only resolves after the parent AsyncRecord resolves. If multiple items within a stream are released for publishing because their parent has just been published, the stream cannot be in its entirety until after all of these promises unwind. The new Publisher keeps track of dependencies manually. When an AsyncRecord is pushed by the publisher, all of its dependent AsyncRecords are synchronously pushed, repeating as necessary, without using the event loop. ## Separate sets for pending vs ready AsyncRecords The current publisher inspects all pending AsyncRecords whenever any of them resolves. All that are completed are added to the response. The new Publisher moves AsyncRecords from the pending set to the ready set as they are pushed, so that on each call to next, only the ready set must be iterated. As a side-effect of this change, the incremental array is ordered by which items are ready for delivery first, and not by the initial document. This seems like a worthwhile tradeoff, and is still adherent to the spec, as far as I can tell. ## No `Promise.race` The old Publisher uses `Promise.race` as the trigger to determine whether payloads are ready. The new Publisher uses a single triggering promise that is triggered whenever an AsyncRecord is pushed, and then reset. This may be beneficial as the implementation of `Promise.race` within V8 has a known memory leak for long-running promises. (see https://bugs.chromium.org/p/v8/issues/detail?id=9858). An alternative would be to utilize @brainkim 's memory-safe version detailed in that issue.
yaacovCR
force-pushed
the
extract-publisher
branch
from
f92d86d to
dce5f9d
Compare
yaacovCR
added a commit
to yaacovCR/graphql-js
that referenced
this pull request
Jan 3, 2023
Depends on graphql#3784 The proposed new Publisher: 1. does not use the event loop to manage AsyncRecord dependencies 2. uses separate sets to store pending vs ready AsyncRecords 3. does not use `Promise.race` ## No event loop for managing AsyncRecord dependencies The current Publisher wraps every AsyncRecord result in a promise that only resolves after the parent AsyncRecord resolves. If multiple items within a stream are released for publishing because their parent has just been published, the stream cannot be in its entirety until after all of these promises unwind. The new Publisher keeps track of dependencies manually. When an AsyncRecord is pushed by the publisher, all of its dependent AsyncRecords are synchronously pushed, repeating as necessary, without using the event loop. ## Separate sets for pending vs ready AsyncRecords The current publisher inspects all pending AsyncRecords whenever any of them resolves. All that are completed are added to the response. The new Publisher moves AsyncRecords from the pending set to the ready set as they are pushed, so that on each call to next, only the ready set must be iterated. As a side-effect of this change, the incremental array is ordered by which items are ready for delivery first, and not by the initial document. This seems like a worthwhile tradeoff, and is still adherent to the spec, as far as I can tell. ## No `Promise.race` The old Publisher uses `Promise.race` as the trigger to determine whether payloads are ready. The new Publisher uses a single triggering promise that is triggered whenever an AsyncRecord is pushed, and then reset. This may be beneficial as the implementation of `Promise.race` within V8 has a known memory leak for long-running promises. (see https://bugs.chromium.org/p/v8/issues/detail?id=9858). An alternative would be to utilize @brainkim 's memory-safe version detailed in that issue.
yaacovCR
force-pushed
the
extract-publisher
branch
from
dce5f9d to
c537db4
Compare
yaacovCR
added a commit
to yaacovCR/graphql-js
that referenced
this pull request
Jan 5, 2023
Depends on graphql#3784 The proposed new Publisher: 1. does not use the event loop to manage AsyncRecord dependencies 2. performs as much work as possible synchronously 2. uses separate sets to store pending vs ready AsyncRecords 3. does not use `Promise.race` -- No event loop for managing AsyncRecord dependencies The current Publisher wraps every AsyncRecord result in a promise that only resolves after the parent AsyncRecord resolves. If multiple items within a stream are released for publishing because their parent has just been published, the stream cannot be in its entirety until after all of these promises unwind. The new Publisher keeps track of dependencies manually. When an AsyncRecord is pushed by the publisher, all of its dependent AsyncRecords are synchronously pushed, repeating as necessary, without using the event loop. In general, the new publisher aims to perform as much work as possible synchronously. -- Separate sets for pending vs ready AsyncRecords The current publisher inspects all pending AsyncRecords whenever any of them resolves. All that are completed are added to the response. The new Publisher moves AsyncRecords from the pending set to the ready set as they are pushed, so that on each call to next, only the ready set must be iterated. As a side-effect of this change, the incremental array is ordered by which items are ready for delivery first, and not by the initial document. This seems like a worthwhile tradeoff, and is still adherent to the spec, as far as I can tell. -- No `Promise.race` The old Publisher uses `Promise.race` as the trigger to determine whether payloads are ready. The new Publisher uses a single triggering promise that is triggered whenever an AsyncRecord is pushed, and then reset. This may be beneficial as the implementation of `Promise.race` within V8 has a known memory leak for long-running promises. (see https://bugs.chromium.org/p/v8/issues/detail?id=9858). An alternative would be to utilize @brainkim 's memory-safe version detailed in that issue.
|
this was reworked and merged as #3903 |
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.
Motivation:
To elaborate on number 2:
Currently, emitting a stream utilizes the event loop to ensure that all stream items are emitted in order, with an extra microtask for each stream item. Utilizing the event loop in this way will be slower than doing so synchronously -- for cases that allow the latter.
In other words, if a late completing early stream item completes and many later stream items are then available for publishing; we currently waterfall through promise resolution for each stream item, rather than adding all the now available incremental results.
Extracting out our current publisher logic allows us to iterate on a proposal that potential has feature improvements such as a resolution of the above potential performance issue.