GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Feature: sync resolvers by DaddyWarbucks · Pull Request #3504 · feathersjs/feathers

Conversation

@DaddyWarbucks

@DaddyWarbucks DaddyWarbucks commented Jun 22, 2024

Copy link
Copy Markdown
Member

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.

const count = 10000
const asyncResolvers = 1
const syncResolvers = 10
const runs = 10

sync0: 86.879ms
sync1: 72.509ms
sync2: 171.684ms
sync3: 69.843ms
sync4: 67.955ms
sync5: 77.137ms
sync6: 75.034ms
sync7: 79.12ms
sync8: 77.653ms
sync9: 78.587ms

async0: 180.141ms
async1: 173.855ms
async2: 169.611ms
async3: 168.678ms
async4: 162.778ms
async5: 177.389ms
async6: 193.163ms
async7: 316.745ms
async8: 157.414ms
async9: 163.333ms

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.

const count = 10000
const asyncResolvers = 10
const syncResolvers = 0
const runs = 10

sync0: 258.305ms
sync1: 124.654ms
sync2: 123.168ms
sync3: 128.291ms
sync4: 126.634ms
sync5: 126.913ms
sync6: 131.84ms
sync7: 134.369ms
sync8: 125.799ms
sync9: 128.844ms

async0: 159.942ms
async1: 167.286ms
async2: 156.082ms
async3: 156.891ms
async4: 321.241ms
async5: 177.017ms
async6: 160.917ms
async7: 155.687ms
async8: 149.171ms
async9: 146.085ms

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ramijarrar, eXigentCoder, and marshallswain reacted with heart emoji ramijarrar reacted with eyes emoji
All reactions
  • ❤️ 3 reactions
  • 👀 1 reaction
DaddyWarbucks added 2 commits June 22, 2024 12:27
@DaddyWarbucks
Update resolver
2f1147c
@DaddyWarbucks
Update test
ceac56e
@DaddyWarbucks DaddyWarbucks changed the title Feature/sync resolvers Feature: sync-resolvers Jun 22, 2024
@DaddyWarbucks DaddyWarbucks changed the title Feature: sync-resolvers Feature: sync resolvers Jun 22, 2024
@DaddyWarbucks
DaddyWarbucks requested a review from daffl July 12, 2024 15:58
@DaddyWarbucks
DaddyWarbucks marked this pull request as draft July 12, 2024 15:58
@DaddyWarbucks
DaddyWarbucks requested a review from marshallswain July 12, 2024 15:58
@DaddyWarbucks

DaddyWarbucks commented Jul 12, 2024

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.

marshallswain reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@daffl

daffl commented Jul 15, 2024

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daffl daffl Awaiting requested review from daffl
@marshallswain marshallswain Awaiting requested review from marshallswain

At least 1 approving review is required to merge this pull request.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@DaddyWarbucks @daffl
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.