graphql-js icon indicating copy to clipboard operation
graphql-js copied to clipboard

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

Open freiksenet opened this issue 1 year ago • 10 comments

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.

freiksenet avatar Jan 11 '24 09:01 freiksenet

СС @yaacovCR

vladar avatar Jan 11 '24 10:01 vladar

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

yaacovCR avatar Jan 11 '24 23:01 yaacovCR

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

yaacovCR avatar Jan 11 '24 23:01 yaacovCR

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

freiksenet avatar Jan 19 '24 01:01 freiksenet

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 avatar Jan 23 '24 13:01 yaacovCR

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 avatar Jan 30 '24 13:01 yaacovCR

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

yaacovCR avatar Jan 30 '24 20:01 yaacovCR

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 avatar Mar 12 '24 05:03 yaacovCR

further improved performance with latest canary https://github.com/graphql/graphql-js/pull/4026#issuecomment-1990229366

-- but still not as good as alpha.2

yaacovCR avatar Mar 17 '24 19:03 yaacovCR

Hi @freiksenet and @vladar!

Are you able to test again with your benchmarks using the latest canary release from https://github.com/graphql/graphql-js/pull/4026#issuecomment-2002512289

...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 avatar Mar 19 '24 08:03 yaacovCR