Feature: sync resolvers - #3504
Draft
DaddyWarbucks wants to merge 2 commits into
Draft
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
|
I know this needs to fix one test before its actually ready. But I wanted to get @daffl and @marshallswain eyes on it for some feedback. Ya'll let me know any thoughts/questions/concerns. |
Sorry, something went wrong.
Copy link
Copy Markdown
Member
|
Thank you for putting this up. It's kind of interesting that things like this don't seem to get optimized by V8. |
Sorry, something went wrong.
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
Reviewers
At least 1 approving review is required to merge this pull request.
2 participants
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.
See: #3492
This PR is a WIP and a proof of concept. I have added a new _resolve method and added a test to compare the speed of this method compared to the current resolve method. The new _resolve method does not use Promise.all to resolve properties. Instead, it uses a basic for loop and done callback. This ensures resolvers that are not actually promises are not scheduled on the event loop.
When running the tests for the package you will see some console.time results. I purposefully left the test configure in a manner that favored syncResolvers. This should accurately illustrate the problem with assuming all resolvers are promises. But you can configure these settings however you would like.
Interestingly, even when using all asyncResolvers the _resolve method is faster. I have noticed this when writing similar updates for yupjs and feathers-fletching. I speculate its because the callback pattern can pop promises off the loop as it goes and Promise.all has to keep them all in the loop until they are done? Pure speculation.
The _resolve method passes all tests but one. You can rename it to resolve (comment/rename real resolve method). It fails one test around virtuals. I played with it and couldn't get it, but I am confident its solvable.
I am also curious if feathersjs/hooks could benefit from a similar treatment. I am not sure how it would work with the next-able hooks, but that may even make it easier. This would allow entire Feathers applications to get a performance boost if hooks like disallow, etc aren't consuming the event loop.