Skip to content

Add JSON annotation in internal/gqlparser/ast to Position fields #7509

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

Merged

Conversation

robmyersrobmyers
Copy link
Contributor

Annotate internal/gqlparser/ast structs not to include Position when marshaled to JSON.
This makes the JSON roundtrip succeed without allocating a ton of memory for JSON fields that will be subsequently pruned.

# Without this change:
$ time ./reproducer-pre-fix
Now passing 1262568 bytes to builtinGraphQLParseSchema() to reproduce the issue in ast.InterfaceToValue()
Alloc = 4159 MiB   TotalAlloc = 5625 MiB   Sys = 5584 MiB  NumGC = 18
Alloc = 8312 MiB   TotalAlloc = 11169 MiB  Sys = 11125 MiB NumGC = 19
Alloc = 8312 MiB   TotalAlloc = 11169 MiB  Sys = 11125 MiB NumGC = 19
Alloc = 16615 MiB  TotalAlloc = 22247 MiB  Sys = 22209 MiB NumGC = 20
Alloc = 16615 MiB  TotalAlloc = 22247 MiB  Sys = 22209 MiB NumGC = 20
Alloc = 16615 MiB  TotalAlloc = 22247 MiB  Sys = 22209 MiB NumGC = 20
Alloc = 38765 MiB  TotalAlloc = 44397 MiB  Sys = 44377 MiB NumGC = 20
Alloc = 33223 MiB  TotalAlloc = 44397 MiB  Sys = 44377 MiB NumGC = 21

# With this change:
$ time ./reproducer-post-fix
Now passing 1262568 bytes to builtinGraphQLParseSchema() to reproduce the issue in ast.InterfaceToValue()

real       0m0.545s
user       0m0.201s
sys        0m0.028s

I have also submitted this change for inclusion in the upstream gqlparser project.

Copy link
Member

@anderseknert anderseknert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing find! Excellent work 👏 I hope it's accepted upstream too.

As for testing, I guess adding a test just to verify that parsing some schemas don't cause infinite recursion (as would happen before the fix) might make some sense?

@robmyersrobmyers
Copy link
Contributor Author

robmyersrobmyers commented Apr 14, 2025

It didn't turn out to be a cycle as I previously speculated. It's just using a ton of memory because each JSON node includes a copy of the entire original source file. In the AST this is just a pointer to a single copy, but when JSON marshaled each Position includes the whole source file.

plot

I'll add a self contained test to the graphql caching PR based off this updated reproducer.

EDIT: The commit with the test.

Copy link
Contributor

@johanfylling johanfylling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! 😃

Annotate internal/gqlparser structs not to include Position when marshaled
to JSON.  This makes the JSON roundtrip succeed without allocating a ton of
memory for JSON fields that will be subsequently pruned.

Upstream PR: vektah/gqlparser#364

$ time ./reproducer-pre-fix
Now passing 1262568 bytes to builtinGraphQLParseSchema() to reproduce the issue in ast.InterfaceToValue()
Alloc = 4159 MiB	TotalAlloc = 5625 MiB	Sys = 5584 MiB	NumGC = 18
Alloc = 8312 MiB	TotalAlloc = 11169 MiB	Sys = 11125 MiB	NumGC = 19
Alloc = 8312 MiB	TotalAlloc = 11169 MiB	Sys = 11125 MiB	NumGC = 19
Alloc = 16615 MiB	TotalAlloc = 22247 MiB	Sys = 22209 MiB	NumGC = 20
Alloc = 16615 MiB	TotalAlloc = 22247 MiB	Sys = 22209 MiB	NumGC = 20
Alloc = 16615 MiB	TotalAlloc = 22247 MiB	Sys = 22209 MiB	NumGC = 20
Alloc = 38765 MiB	TotalAlloc = 44397 MiB	Sys = 44377 MiB	NumGC = 20
Alloc = 33223 MiB	TotalAlloc = 44397 MiB	Sys = 44377 MiB	NumGC = 21

$ time ./reproducer-post-fix
Now passing 1262568 bytes to builtinGraphQLParseSchema() to reproduce the issue in ast.InterfaceToValue()

real	0m0.545s
user	0m0.201s
sys	0m0.028s

Signed-off-by: Rob Myers <[email protected]>
@robmyersrobmyers robmyersrobmyers force-pushed the gqlparser_json_position branch from 3ddc2a0 to a10f918 Compare April 16, 2025 14:11
Copy link

netlify bot commented Apr 16, 2025

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit 4a05feb
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/67ffcdd017f2a000088912c9
😎 Deploy Preview https://deploy-preview-7509--openpolicyagent.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.

@johanfylling johanfylling merged commit 56c9aa5 into open-policy-agent:main Apr 16, 2025
50 of 51 checks passed
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.

3 participants