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

perf: Replace most sticky regexs with pure parser combinators #52

Merged
merged 21 commits into from
Feb 13, 2025

Conversation

kitten
Copy link
Member

@kitten kitten commented Feb 13, 2025

Summary

I was a little bored and distracting myself from another parsing-related task, which made me curious whether recent V8 changes make this change worthwhile — and it does seem pretty impactful.

This removes most sticky regexes for manual character peeks, and attempts to be as efficient as possible in only checking an upcoming character once, if possible. The exceptions here are:

  • values (since after null | true | false an enum value may follow)
  • float parts after integers (since this is just not worth it while keeping the size small)
  • on in inline fragments
    In a few select sections we're still checking a character twice, but this seems to be the minimal amount of operations.
Benchmarks before Benchmarks after
Before After

The benchmarks above are run only with parser.bench.ts active, since running multiple benchmarks in one Vitest run skews the results. It'd still show this branch as faster by approximately the same percentage, but the result is less stable.

  • When removing the graphql benchmarks we arrive at: ~157,000op/s -> ~210,000ops/s (+/- 5,000)
  • When benchmarking against tiny documents the difference shrinks down a lot however, as we already experienced in prior PRs

(Funny observation: It seems that GraphQL's lookahead for selections seems to run into similar problems that we optimised away already. All three GraphQL.js parsers can be observed to do worse the more fields there are — especially when starting child selection sets — without any additional syntax but fields)

Bundle size impact (after a few manual tweaks) is positive and a reduction overall, but essentially identical with a full Terser run anyway.

Before:
4.83 kB (dist/graphql.web.mjs gzip-only)
3.98 kB (dist/graphql.web.mjs minzipped)

After: 
4.73 kB (dist/graphql.web.mjs gzip-only)
3.97 kB (dist/graphql.web.mjs minzipped)

Set of changes

  • Replace valueRe with a large switch statement that checks the first character manually
  • Manually parse non-block StringValue nodes without a regex (since they're really simple)
    • We mark complex strings (with escape chars) immediately too while doing this
  • Replace intRe with a manual digit parser (since first char - is known, this is trivial)
  • Wrap floatPartRe regex in an if-statement check since prior char is already loaded
  • Replace nameRe with manual parsing
  • Replace selection parsing regex with manual parsing
  • Replace operation definition regex with name() parser
  • Simplify input Source | string check
  • Inline operation definition parser
  • Tweak Terser config (seems to make benchmarks slightly more stable for some reason)
  • Move intrusive '{' checks for selection sets into separate function (doesn't impact perf)

Copy link

changeset-bot bot commented Feb 13, 2025

🦋 Changeset detected

Latest commit: 50a7d5f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@0no-co/graphql.web Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@kitten kitten merged commit 2984e7b into main Feb 13, 2025
2 checks passed
@github-actions github-actions bot mentioned this pull request Feb 13, 2025
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

Successfully merging this pull request may close these issues.

2 participants