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

incremental: remove filtering from rewrite #4032

Closed
wants to merge 2 commits into from

Conversation

yaacovCR
Copy link
Contributor

depends on #4026

removed filtering again

Copy link

Hi @yaacovCR, 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

Copy link

netlify bot commented Mar 21, 2024

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit a043386
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/660fde76ee34ba0008b5bce7
😎 Deploy Preview https://deploy-preview-4032--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 configuration.

@yaacovCR

This comment has been minimized.

Copy link

@github-actions publish-pr-on-npm

@yaacovCR The latest changes of this PR are available on NPM as
graphql@17.0.0-alpha.3.canary.pr.4032.4fb41fe3e1f2b4b27437138d6d7d4763c1992e7a
Note: no gurantees provided so please use your own discretion.

Also you can depend on latest version built from this PR:
npm install --save graphql@canary-pr-4032

@yaacovCR
Copy link
Contributor Author

Looks like this PR is about 5-10% slower than #4026 using the supermassive benchmark from #4008.

As usual, the results are pretty noisy, what I am most interested in is the run for already parsed queries, and the noise seems low enough to get a comparison.

So @robrichard, the question is whether we can optimize this further and make up that ground and whether the 5-10% differential is worth the divergence from the spec edits.

This PR:

yaaco@Yaacov-15 MINGW64 ~/Documents/GitHub/graphitation/packages/supermassive (main)
$ yarn run benchmark
yarn run v1.22.19
$ node --require ts-node/register ./src/benchmarks/index.ts
Query parsing
graphql-js x 160,596 ops/sec ±3.96% (74 runs sampled)
Query Running
graphql-js - string queries x 44,479 ops/sec ±1.92% (73 runs sampled)
graphql-js - parsed queries x 71,218 ops/sec ±2.52% (73 runs sampled)
Done in 20.70s.

yaaco@Yaacov-15 MINGW64 ~/Documents/GitHub/graphitation/packages/supermassive (main)
$ yarn run benchmark
yarn run v1.22.19
$ node --require ts-node/register ./src/benchmarks/index.ts
Query parsing
graphql-js x 153,906 ops/sec ±4.54% (69 runs sampled)
Query Running
graphql-js - string queries x 45,403 ops/sec ±1.98% (78 runs sampled)
graphql-js - parsed queries x 74,677 ops/sec ±2.39% (68 runs sampled)
Done in 20.63s.

yaaco@Yaacov-15 MINGW64 ~/Documents/GitHub/graphitation/packages/supermassive (main)
$ yarn run benchmark
yarn run v1.22.19
$ node --require ts-node/register ./src/benchmarks/index.ts
Query parsing
graphql-js x 162,527 ops/sec ±4.45% (73 runs sampled)
Query Running
graphql-js - string queries x 45,545 ops/sec ±1.88% (74 runs sampled)
graphql-js - parsed queries x 72,569 ops/sec ±2.06% (71 runs sampled)
Done in 21.02s.

yaaco@Yaacov-15 MINGW64 ~/Documents/GitHub/graphitation/packages/supermassive (main)
$ yarn run benchmark
yarn run v1.22.19
$ node --require ts-node/register ./src/benchmarks/index.ts
Query parsing
graphql-js x 151,312 ops/sec ±4.19% (68 runs sampled)
Query Running
graphql-js - string queries x 44,999 ops/sec ±1.88% (72 runs sampled)
graphql-js - parsed queries x 73,783 ops/sec ±2.13% (72 runs sampled)
Done in 20.55s.

With #4026:

yaaco@Yaacov-15 MINGW64 ~/Documents/GitHub/graphitation/packages/supermassive (main)
$ yarn run benchmark
yarn run v1.22.19
$ node --require ts-node/register ./src/benchmarks/index.ts
Query parsing
graphql-js x 151,039 ops/sec ±4.55% (69 runs sampled)
Query Running
graphql-js - string queries x 43,278 ops/sec ±5.44% (69 runs sampled)
graphql-js - parsed queries x 77,934 ops/sec ±1.89% (68 runs sampled)
Done in 20.52s.

yaaco@Yaacov-15 MINGW64 ~/Documents/GitHub/graphitation/packages/supermassive (main)
$ yarn run benchmark
yarn run v1.22.19
$ node --require ts-node/register ./src/benchmarks/index.ts
Query parsing
graphql-js x 159,241 ops/sec ±3.79% (73 runs sampled)
Query Running
graphql-js - string queries x 45,811 ops/sec ±2.12% (71 runs sampled)
graphql-js - parsed queries x 77,033 ops/sec ±2.00% (70 runs sampled)
Done in 20.60s.

yaaco@Yaacov-15 MINGW64 ~/Documents/GitHub/graphitation/packages/supermassive (main)
$ yarn run benchmark
yarn run v1.22.19
$ node --require ts-node/register ./src/benchmarks/index.ts
Query parsing
graphql-js x 157,320 ops/sec ±3.99% (72 runs sampled)
Query Running
graphql-js - string queries x 43,644 ops/sec ±4.40% (70 runs sampled)
graphql-js - parsed queries x 77,631 ops/sec ±1.81% (71 runs sampled)
Done in 20.95s.

yaaco@Yaacov-15 MINGW64 ~/Documents/GitHub/graphitation/packages/supermassive (main)
$ yarn run benchmark
yarn run v1.22.19
$ node --require ts-node/register ./src/benchmarks/index.ts
Query parsing
graphql-js x 158,876 ops/sec ±3.38% (72 runs sampled)
Query Running
graphql-js - string queries x 40,389 ops/sec ±8.26% (63 runs sampled)
graphql-js - parsed queries x 78,750 ops/sec ±2.36% (68 runs sampled)
Done in 20.58s.

@yaacovCR yaacovCR added the PR: polish 💅 PR doesn't change public API or any observed behaviour label Mar 21, 2024
@yaacovCR yaacovCR changed the title incremental: remove filtering again incremental: remove filtering from rewrite Mar 21, 2024
@yaacovCR

This comment has been minimized.

Copy link

@github-actions publish-pr-on-npm

@yaacovCR The latest changes of this PR are available on NPM as
graphql@17.0.0-alpha.3.canary.pr.4032.8bcdcea90e0a24432a78270866c27e0db6a2ae4d
Note: no gurantees provided so please use your own discretion.

Also you can depend on latest version built from this PR:
npm install --save graphql@canary-pr-4032

@yaacovCR yaacovCR force-pushed the remove-filtering-again branch 3 times, most recently from 5fe0b2f to f2ec6a0 Compare March 28, 2024 21:40
@yaacovCR
Copy link
Contributor Author

image

take-two = #4026
remove-filtering-again = this PR

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Apr 5, 2024

I've actually gotten some better results with this, so I am going to fold this into #4026. We can always re-introduce filtering again if need be, but it would be nice if the implementation could match the spec.

@yaacovCR yaacovCR closed this Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: polish 💅 PR doesn't change public API or any observed behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant