Skip to content

A more memory efficient AstPrinter #4028

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 4, 2025
Merged

A more memory efficient AstPrinter #4028

merged 1 commit into from
Jul 4, 2025
+16 −1

Conversation

bbakerman
Copy link
Member

No description provided.

@bbakerman bbakerman mentioned this pull request Jun 28, 2025
Open
@bbakerman bbakerman added this to the 25.x breaking changes milestone Jun 28, 2025
@github-actions GitHub Actions
Copy link
Contributor

Test Results

  318 files   -   632    318 suites   - 632   2m 46s ⏱️ - 5m 39s
4 884 tests  -   312  4 874 ✅  -   313  10 💤 + 1  0 ❌ ±0 
4 973 runs   - 9 940  4 963 ✅  - 9 921  10 💤  - 19  0 ❌ ±0 

Results for commit 83ec4d5. ± Comparison against base commit aef8032.

This pull request removes 462 and adds 128 tests. Note that renamed tests count towards both. 
	?
                __schema { types { fields { args { type { name fields { name }}}}}}
                __schema { types { fields { type { name fields { name }}}}}
                __schema { types { inputFields { type { inputFields { name }}}}}
                __schema { types { interfaces { fields { type { interfaces { name } } } } } }
                __schema { types { name} }
                __type(name : "t") { name }
                a1: __schema { types { name} }
                a1: __type(name : "t") { name }
                a2 :  __type(name : "t1") { name }
…

graphql.ScalarsBooleanTest ‑ parseValue throws exception for invalid input <java.lang.Object@236f1f73>
graphql.ScalarsBooleanTest ‑ serialize throws exception for invalid input <java.lang.Object@5400402d>
graphql.ScalarsIDTest ‑ parseValue allows any object via String.valueOf <java.lang.Object@186b53de>
graphql.ScalarsIDTest ‑ serialize allows any object via String.valueOf <java.lang.Object@3109b25e>
graphql.ScalarsIntTest ‑ parseValue throws exception for invalid input <java.lang.Object@3452f5e6>
graphql.ScalarsIntTest ‑ serialize throws exception for invalid input <java.lang.Object@74ccba17>
graphql.execution.instrumentation.TracingInstrumentationTest ‑ do not trace introspection information [testExecutionStrategy: <graphql.execution.AsyncExecutionStrategy@78fa1c7e fieldCollector=graphql.execution.FieldCollector@16226377 executionStepInfoFactory=graphql.execution.ExecutionStepInfoFactory@68eb2246 dataFetcherExceptionHandler=graphql.execution.SimpleDataFetcherExceptionHandler@18f531b9 resolvedType=graphql.execution.ResolveType@30c4d735>, #0]
graphql.execution.instrumentation.TracingInstrumentationTest ‑ do not trace introspection information [testExecutionStrategy: <graphql.execution.AsyncSerialExecutionStrategy@4b0ce7a fieldCollector=graphql.execution.FieldCollector@35f94fac executionStepInfoFactory=graphql.execution.ExecutionStepInfoFactory@23124b59 dataFetcherExceptionHandler=graphql.execution.SimpleDataFetcherExceptionHandler@57821091 resolvedType=graphql.execution.ResolveType@55c3c705>, #1]
graphql.execution.instrumentation.TracingInstrumentationTest ‑ tracing captures timings as expected [testExecutionStrategy: <graphql.execution.AsyncExecutionStrategy@5acd6f1c fieldCollector=graphql.execution.FieldCollector@3c87551b executionStepInfoFactory=graphql.execution.ExecutionStepInfoFactory@711d977f dataFetcherExceptionHandler=graphql.execution.SimpleDataFetcherExceptionHandler@55cba574 resolvedType=graphql.execution.ResolveType@64cf855e>, #0]
graphql.execution.instrumentation.TracingInstrumentationTest ‑ tracing captures timings as expected [testExecutionStrategy: <graphql.execution.AsyncSerialExecutionStrategy@187b6d97 fieldCollector=graphql.execution.FieldCollector@ee8d0a5 executionStepInfoFactory=graphql.execution.ExecutionStepInfoFactory@41f022de dataFetcherExceptionHandler=graphql.execution.SimpleDataFetcherExceptionHandler@5ef759ab resolvedType=graphql.execution.ResolveType@1d238037>, #1]
…
This pull request skips 1 test. 
graphql.schema.fetching.LambdaFetchingSupportTest ‑ different class loaders induce certain behaviours

xuorig
xuorig approved these changes Jun 28, 2025
View reviewed changes
Copy link

@xuorig xuorig left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you!

As an aside:

It could be great to add an API that allows users to pass their own string builder for flexibility. That allows things like reserving the capacity needed, or reusing string builders, which we both currently do in a custom printer.

    public static StringBuilder printAst(Node node, StringBuilder out) {
        printAstTo(node, out);
        return out
    }

@bbakerman
Copy link
Member Author

bbakerman commented Jun 29, 2025 •
edited
Loading

Thank you!

As an aside:

It could be great to add an API that allows users to pass their own string builder for flexibility. That allows things like reserving the capacity needed, or reusing string builders, which we both currently do in a custom printer.

    public static StringBuilder printAst(Node node, StringBuilder out) {
        printAstTo(node, out);
        return out
    }

We have

    public static void printAstTo(Node<?> node, Appendable appendable) {

and StringBuilder is an Appendable so thats already possible

StringBuilder out = new StringBuilder();
AsPrinter.printAstTo(node1,out);
AsPrinter.printAstTo(node2,out);

out.append("more strings")

Maybe not super fluent but possible

@bbakerman bbakerman merged commit f3c1a49 into master Jul 4, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xuorig xuorig xuorig approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
25.x breaking changes
Development

Successfully merging this pull request may close these issues.
2 participants
@bbakerman @xuorig