Skip to content

Conversation

@IvanGoncharov
Copy link
Member

Motivation: generalize it and remove code dublication in two places and
also allow to use as public API outside of graphql-js

@netlify
Copy link

netlify bot commented May 25, 2022

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 475bf7d
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/628e803f8ad7790009d769c2
😎 Deploy Preview https://deploy-preview-3605--compassionate-pike-271cb3.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 settings.

@IvanGoncharov
Copy link
Member Author

@github-actions
Copy link

Hi @IvanGoncharov, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM
@IvanGoncharov

This comment has been minimized.

@github-actions
Copy link

@github-actions run-benchmark

@IvanGoncharov Please, see benchmark results here: https://github.com/graphql/graphql-js/runs/6598090837?check_suite_focus=true#step:6:1

@IvanGoncharov
Copy link
Member Author

IvanGoncharov commented May 25, 2022

@IvanGoncharov Please, see benchmark results here: graphql/graphql-js/runs/6598090837?check_suite_focus=true#step:6:1

Sadly, I need to figure out a way to run benchmark reliably on CI after #3604
But here is data from my machine where HEAD is this PR.
image

So performance slightly increases by ~1% due to using a switch instead of multiple if.

@IvanGoncharov IvanGoncharov marked this pull request as ready for review May 25, 2022 19:13
Motivation: generalize it and remove code dublication in two places and also allow to use as public API outside of graphql-js
@IvanGoncharov IvanGoncharov added the PR: feature 🚀 requires increase of "minor" version number label May 27, 2022
@IvanGoncharov IvanGoncharov merged commit 69e1554 into graphql:main May 27, 2022
@IvanGoncharov IvanGoncharov deleted the pr_branch2 branch May 27, 2022 19:19
@yaacovCR
Copy link
Contributor

Because this PR contains two changes ie change to switch and addition of unnecessary check for unions during execution, the benchmark results are not so accurate.

i have suggested adding two helpers. See #3519 (comment)

getObjectField and getCompositeField maybe?

@yaacovCR
Copy link
Contributor

Sadly, I need to figure out a way to run benchmark reliably on CI after #3604

So it seems like #3604 broke benchmarking? Can we revert at least until fixed? Can we consider a 3rd party benchmarking solution as an additional dev-dependency so that we can outsource some of the benchmarking work?

yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this pull request Dec 11, 2025
yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this pull request Dec 11, 2025
yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this pull request Dec 11, 2025
yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this pull request Dec 11, 2025
yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this pull request Dec 11, 2025
yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this pull request Dec 16, 2025
yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this pull request Dec 17, 2025
yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this pull request Dec 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: feature 🚀 requires increase of "minor" version number

2 participants