Skip to content

AstPrinter performance improvements #2729

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
bbakerman merged 1 commit into from
Feb 17, 2022
Merged

AstPrinter performance improvements #2729

bbakerman merged 1 commit into from
Feb 17, 2022

Conversation

@bbakerman
Copy link
Member

@bbakerman bbakerman commented Feb 17, 2022 •
edited
Loading 

Benchmark                                           Mode  Cnt     Score     Error  Units
AstPrinterBenchmark.benchMarkAstPrinterThroughput  thrpt   15  3715.744 ± 102.575  ops/s
AstPrinterBenchmark.benchMarkAstPrinterAvgTime      avgt   15     0.282 ±   0.022  ms/op


Benchmark                                           Mode  Cnt     Score     Error  Units
AstPrinterBenchmark.benchMarkAstPrinterThroughput  thrpt   15  4468.229 ± 153.149  ops/s
AstPrinterBenchmark.benchMarkAstPrinterAvgTime      avgt   15     0.220 ±   0.015  ms/op

ps - this is the benchmarks but with printCompact - its still MUCH faster

Benchmark                                                  Mode  Cnt     Score     Error  Units
AstPrinterBenchmark.benchMarkAstPrinterCompactThroughput  thrpt   15  8555.174 ± 298.140  ops/s
AstPrinterBenchmark.benchMarkAstPrinterThroughput         thrpt   15  4734.532 ± 213.044  ops/s
AstPrinterBenchmark.benchMarkAstPrinterAvgTime             avgt   15     0.208 ±   0.011  ms/op
AstPrinterBenchmark.benchMarkAstPrinterCompactAvgTime      avgt   15     0.119 ±   0.004  ms/op

bbakerman
bbakerman commented Feb 17, 2022
View reviewed changes
src/main/java/graphql/language/AstPrinter.java
return "";
}
return start + maybeString + (!isEmpty(end) ? end : "");
return new StringBuilder().append(start).append(maybeString).append(!isEmpty(end) ? end : "").toString();
Copy link
Member Author

@bbakerman bbakerman Feb 17, 2022

Choose a reason for hiding this comment

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

This actually makes a difference in the benchmarks - not a lot but some

Copy link
Contributor

@jord1e jord1e Feb 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This may no longer be true when compiling with JDK 9, just throwing it in
https://openjdk.java.net/jeps/280

I do not know how long you want to stay on JDK 8 though, for now it is an improvement

bbakerman
bbakerman commented Feb 17, 2022
View reviewed changes
src/main/java/graphql/language/AstPrinter.java
}

private String indent(String maybeString) {
if (isEmpty(maybeString)) {
Copy link
Member Author

@bbakerman bbakerman Feb 17, 2022

Choose a reason for hiding this comment

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

it was never empty - ever

@bbakerman bbakerman added this to the 18.0 milestone Feb 17, 2022
@bbakerman bbakerman merged commit 981f840 into master Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jord1e jord1e jord1e left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

18.0

Development

Successfully merging this pull request may close these issues.

3 participants

@bbakerman @jord1e