-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Issue #12809: Improve performance of lambda processing #12814
Conversation
cd00ad3
to
26f5021
Compare
@nrmancuso should we developer our own certified method of testing performance? Ex: https://github.com/rnveach/checkstyle/commits/more_audits |
ec8d60e
to
63c0e0d
Compare
@nrmancuso I find it hard to see if there is anything changing when you force-push. Should I retest this? And is there anything missing that blocks this from moving out of the |
@dreis2211 you are good, next step for you is to use it after we release :) I have some reports to generate and share and a few other odds and ends here. |
src/main/java/com/puppycrawl/tools/checkstyle/JavaAstVisitor.java
Outdated
Show resolved
Hide resolved
52c5fa9
to
a4ed3fc
Compare
@dreis2211 this should be final iteration of code, feel free to test for performance again
@rnveach this is a good idea, but I am not sure how to execute it. When I make grammar changes, I sometimes analyze the grammar using the ANTLR tool, but from a holistic perspective, I am not sure how to do this. |
* The tokens here are technically expressions, but should | ||
* not return an EXPR token as their root. | ||
*/ | ||
private static final int[] EXPRESSIONS_WITH_NO_EXPR_ROOT = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made this a static field so that we do not have to create array with each method call.
private DetailAstImpl buildExpressionNode(ParseTree exprNode) { | ||
final DetailAstImpl expression = visit(exprNode); | ||
|
||
final DetailAstImpl exprRoot; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaned up this code a bit, but this is basically what was in the visitExpression method.
Performance looks good still @nrmancuso |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok to merge
Closes #12809
Reports:
AST regression test report:
https://nrmancuso.github.io/reports/issue-12809/2023-03-18-T-00-03-28/antlr-report/index.html
Check regression reports:
https://nrmancuso.github.io/reports/issue-12809/2023-03-18-T-00-03-28/part2-report/index.html
https://nrmancuso.github.io/reports/issue-12809/2023-03-18-T-00-03-28/part4-report/index.html
https://nrmancuso.github.io/reports/issue-12809/2023-03-18-T-00-03-28/sevntu-check-regression_part_1-report/index.html
https://nrmancuso.github.io/reports/issue-12809/2023-03-18-T-00-03-28/part6-report/index.html
https://nrmancuso.github.io/reports/issue-12809/2023-03-18-T-00-03-28/part3-report/index.html
https://nrmancuso.github.io/reports/issue-12809/2023-03-18-T-00-03-28/sevntu-check-regression_part_2-report/index.html
https://nrmancuso.github.io/reports/issue-12809/2023-03-18-T-00-03-28/part5-report/index.html
https://nrmancuso.github.io/reports/issue-12809/2023-03-18-T-00-03-28/part1-report/index.html