OverlappingFieldsCanBeMergedRule: Fix performance degradation

[AaronMoat]

Update(from @IvanGoncharov): In addition to reversing #3457 this PR improves performance further, see below discussion.

Resolves #3955 (at least greatly mitigates it)

Effectively reverts #3457 which introduces a performance degradation found in #3955. This pull request was identified in a git bisect. The performance issue results in the potential for DOS attacks.

I think there are potential performance increases still available in this file, but this is a simple enough fix for the immediate problem.

The following query:

const maliciousQuery = `{ ${'hello '.repeat(2000)}}`;

now takes 564ms on my machine, and on main takes 12s.

[netlify]

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	2991112
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/64f771eb500b3b00086f752e
😎 Deploy Preview	https://deploy-preview-3958--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[AaronMoat]

Hi @graphql/graphql-js-reviewers (I can't seem to ping this list - so @martijnwalraven @Cito @yaacovCR @IvanGoncharov @saihaj) any chance we could get some eyes on this? 🙏

[tadhglewis]

I can create a issue as this can be done in a separate PR but it's somewhat concerning this regression was possible in the first place. Would be great to have some perf tests in place.

[github-actions]

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

[github-actions]

@github-actions run-benchmark

@AaronMoat Please, see benchmark results here: https://github.com/graphql/graphql-js/actions/runs/6011999302/job/16306504508#step:6:1

[yaacovCR]

If I’m interpreting the benchmark results correctly, I am not seeing a performance improvement with this fix…

[AaronMoat]

I don't think this specific scenario is in the benchmarks.

[yaacovCR]

I don't think this specific scenario is in the benchmarks.

I guess what I’m wondering is (1) where the crossover point is, as performance seems better on main with our current tests but worse in your above scenario, and (2) whether the better mitigation is to use the maxTokens option when parsing?

graphql-js/src/language/parser.ts

Line 92 in b12dcff

maxTokens?: number | undefined;

That’s just my own 2 cents, would really love @IvanGoncharov take

[AaronMoat]

The note on maxTokens is an interesting one, noting it's linear. I'd argue this is nonlinear, even with this change.

[IvanGoncharov]

@AaronMoat Can you please add a benchmark to this PR otherwise this problem can reappear in the future.

[github-actions]

@github-actions run-benchmark

@AaronMoat Something went wrong, please check log.

[github-actions]

@github-actions run-benchmark

@AaronMoat Something went wrong, please check log.

[AaronMoat]

Unsure what's going on with the benchmarks; it seems to have not even got up to the new one this time.

[IvanGoncharov]

@AaronMoat Sorry for delay, I spend some time playing with this code and came up with 2991112 it further improves performance:

It reduces memory usage by ~40%.

[IvanGoncharov]

@AaronMoat If you need this fix in v16, please a create separate PR for:
https://github.com/graphql/graphql-js/tree/16.x.x

[AaronMoat]

Thanks @IvanGoncharov for taking that and running with it -- raised #3967 for 16.x.x