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

Current main is up to 50% slower than previous major version #4008

Closed
freiksenet opened this issue Jan 11, 2024 · 11 comments
Closed

Current main is up to 50% slower than previous major version #4008

freiksenet opened this issue Jan 11, 2024 · 11 comments

Comments

@freiksenet
Copy link
Contributor

freiksenet commented Jan 11, 2024

Based on our benchmarks, latest implementation is up to 50% slower than the one in last 17 alpha. The slowness is based on extra work done after collectFields - buildFieldPlan. I think it's quite a major regression that should be addressed before it's published as a release.

@vladar
Copy link

vladar commented Jan 11, 2024

СС @yaacovCR

@yaacovCR
Copy link
Contributor

Are you comparing latest main to https://www.npmjs.com/package/graphql/v/17.0.0-alpha.3 or to v16.x?

@yaacovCR
Copy link
Contributor

If you are available to walk me through your benchmark and how you have excluded collectfields/buildfieldplan => as far as I can tell that’s mostly what changed from alpha 3

@freiksenet freiksenet changed the title Current master is up to 50% slower than previous major version Current main is up to 50% slower than previous major version Jan 19, 2024
@freiksenet
Copy link
Contributor Author

Our GraphQL executor is based on graphql-js, latest update that added defer/stream was based on alpha.3 that had no regressions. When I ported the latest code that was added in main it became significantly slower. I then measure the latest main and it was the same regression in graphql-js.

Here is our benchmark suite (execution is the only important one for this):
https://github.com/microsoft/graphitation/tree/main/packages/supermassive/src/benchmarks

@yaacovCR
Copy link
Contributor

Our GraphQL executor is based on graphql-js, latest update that added defer/stream was based on alpha.3 that had no regressions. When I ported the latest code that was added in main it became significantly slower. I then measure the latest main and it was the same regression in graphql-js.

Here is our benchmark suite (execution is the only important one for this): https://github.com/microsoft/graphitation/tree/main/packages/supermassive/src/benchmarks

That’s very helpful. I hope to have time to look into this.

@yaacovCR
Copy link
Contributor

These are my results using your benchmarks for graphql-js only on my machine, not scientific, but seems to show the problem is between alpha-2 and alpha-3. I ran the benchmarks x2 each for main, alpha-3, alpha-2....

More investigation to follow....

I am also curious how you pinpointed that the problem is outside collectFields / buildFieldPlan.

main

Query parsing
graphql-js x 119,524 ops/sec ±5.70% (66 runs sampled)
Query Running
graphql-js - string queries x 36,796 ops/sec ±3.26% (76 runs sampled)
graphql-js - parsed queries x 65,519 ops/sec ±2.22% (80 runs sampled)

Query parsing
graphql-js x 123,590 ops/sec ±5.93% (64 runs sampled)
Query Running
graphql-js - string queries x 41,672 ops/sec ±1.62% (79 runs sampled)
graphql-js - parsed queries x 71,277 ops/sec ±1.21% (81 runs sampled)

alpha3

Query parsing
graphql-js x 121,244 ops/sec ±6.78% (66 runs sampled)
Query Running
graphql-js - string queries x 38,568 ops/sec ±1.89% (76 runs sampled)
graphql-js - parsed queries x 64,601 ops/sec ±1.56% (79 runs sampled)

Query parsing
graphql-js x 126,291 ops/sec ±5.05% (65 runs sampled)
Query Running
graphql-js - string queries x 39,484 ops/sec ±1.31% (78 runs sampled)
graphql-js - parsed queries x 66,810 ops/sec ±0.97% (82 runs sampled)

alpha2

Query parsing
graphql-js x 128,254 ops/sec ±5.07% (68 runs sampled)
Query Running
graphql-js - string queries x 49,546 ops/sec ±2.53% (73 runs sampled)
graphql-js - parsed queries x 112,815 ops/sec ±1.70% (79 runs sampled)

Query parsing
graphql-js x 126,365 ops/sec ±5.34% (68 runs sampled)
Query Running
graphql-js - string queries x 48,240 ops/sec ±3.08% (72 runs sampled)
graphql-js - parsed queries x 113,762 ops/sec ±1.25% (81 runs sampled)

@yaacovCR
Copy link
Contributor

Just to point out the initially obvious, the major change between the alphas was the introduction of deduplicated incremental delivery.

@yaacovCR
Copy link
Contributor

looks like #4026 improves performance on alpha3 in the direction of alpha2 (note that today my machine is overall smaller, compare to below runs for alpha.2 and alpha.3 rather than the above ops/sec).

definitely might be further room for improvement there!

(would be great if we could get benchmarking into the PR checks/workflow)

graphql@17.0.0-alpha.3.canary.pr.4026.5922420b3b235970ee230497190e28c8290c8f16 (#4026)
Query parsing
graphql-js x 79,078 ops/sec ±4.41% (63 runs sampled)
Query Running
graphql-js - string queries x 21,023 ops/sec ±2.66% (66 runs sampled)
graphql-js - parsed queries x 36,197 ops/sec ±2.56% (64 runs sampled)

alpha.3
Query parsing
graphql-js x 83,228 ops/sec ±4.85% (63 runs sampled)
Query Running
graphql-js - string queries x 18,558 ops/sec ±3.17% (61 runs sampled)
graphql-js - parsed queries x 27,912 ops/sec ±2.05% (67 runs sampled)

alpha2
Query parsing
graphql-js x 81,141 ops/sec ±4.05% (63 runs sampled)
Query Running
graphql-js - string queries x 23,374 ops/sec ±3.77% (55 runs sampled)
graphql-js - parsed queries x 43,548 ops/sec ±2.09% (70 runs sampled)

@yaacovCR
Copy link
Contributor

further improved performance with latest canary #4026 (comment)

-- but still not as good as alpha.2

@yaacovCR
Copy link
Contributor

yaacovCR commented Mar 19, 2024

Hi @freiksenet and @vladar!

Are you able to test again with your benchmarks using the latest canary release from #4026 (comment)

...on my machine this latest canary essentially pulls even with alpha.2

It is also at the limit of my ideas about how to improve further.

Thanks so much for opening this issue!

@yaacovCR
Copy link
Contributor

yaacovCR commented Oct 1, 2024

Closing, feel free to reopen as necessary.

@yaacovCR yaacovCR closed this as completed Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants