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

ASTs should be appended with comment nodes only when javadoc checks are present. ASTs shouldn't be walked if there are no corresponding types of checks #4393

Closed
ps-sp opened this Issue May 31, 2017 · 6 comments

Comments

3 participants
@ps-sp
Collaborator

ps-sp commented May 31, 2017

Taken from this comment. It seems that hidden comment nodes are appended and then the complete AST (with javadoc) is walked even when there are no javadoc checks. Calls to walk method shouldn't be made if corresponding checks are not there (ordinary - javadoc).

Here are the related statistics. (produced using jprofiler).

program arguments: -c sun_checks.xml -o NUL contribution\checkstyle-tester\repositories\guava
had removed all the checks which would lead to javadoc parsing, i.e checks which have isCommentNodesRequired() returning true

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach May 31, 2017

Member

https://github.com/checkstyle/checkstyle/blob/de50d3465849b83d25910590e1f5f39a25fe6e2c/src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java#L180-#L184
hidden comment nodes are appended and then the complete AST (with javadoc) is walked

Since this code is in TreeWalker, it has nothing to with Javadocs.
Javadocs aren't generated until we actually add a Javadoc check and reach a valid javadoc node in visitToken.
The code in question more closely relates to normal AbstractChecks that need to be comment aware, which return true from isCommentNodesRequired (like TodoCommentCheck and yes the Javadocs).

Since we are already separating comment-aware checks and non-comment aware checks, all we need to do is check if commentChecks is empty and skip the appending and walking.

This should be an easy patch, but I don't think it will be common for most users to not have atleast 1 of our ~12 comment-aware checks and javadoc checks.
I am fine with this change.

Member

rnveach commented May 31, 2017

https://github.com/checkstyle/checkstyle/blob/de50d3465849b83d25910590e1f5f39a25fe6e2c/src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java#L180-#L184
hidden comment nodes are appended and then the complete AST (with javadoc) is walked

Since this code is in TreeWalker, it has nothing to with Javadocs.
Javadocs aren't generated until we actually add a Javadoc check and reach a valid javadoc node in visitToken.
The code in question more closely relates to normal AbstractChecks that need to be comment aware, which return true from isCommentNodesRequired (like TodoCommentCheck and yes the Javadocs).

Since we are already separating comment-aware checks and non-comment aware checks, all we need to do is check if commentChecks is empty and skip the appending and walking.

This should be an easy patch, but I don't think it will be common for most users to not have atleast 1 of our ~12 comment-aware checks and javadoc checks.
I am fine with this change.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 1, 2017

Member

@ps-sp , true to say it was originally designed to skip walk if no javadoc checks are used in config as it cause enormous performance hit - http://roman-ivanov.blogspot.com/2016/02/performance-problem-while-using-checks.html .

But looks like we addressed the most obvious part and missed that append action, walk will do nothing as there is no javadoc. So I presume you speak about optimization to avoid appending tokens. That is valid.

Please update issue title to more clear on target of optimization. Please update description to show in details place of problem. In other case we need to prove issue by CLI.

in PR please put some values how much time this optimization reduce execution on guava project, with no google config withtout javadoc Checks.

@rnveach ,

but I don't think it will be common for most users to not have atleast 1 of our ~12 comment-aware checks and javadoc checks.

yes, but I usually recommend users to have two configurations - for code and javadoc ; and run javadoc less frequently on code.

Member

romani commented Jun 1, 2017

@ps-sp , true to say it was originally designed to skip walk if no javadoc checks are used in config as it cause enormous performance hit - http://roman-ivanov.blogspot.com/2016/02/performance-problem-while-using-checks.html .

But looks like we addressed the most obvious part and missed that append action, walk will do nothing as there is no javadoc. So I presume you speak about optimization to avoid appending tokens. That is valid.

Please update issue title to more clear on target of optimization. Please update description to show in details place of problem. In other case we need to prove issue by CLI.

in PR please put some values how much time this optimization reduce execution on guava project, with no google config withtout javadoc Checks.

@rnveach ,

but I don't think it will be common for most users to not have atleast 1 of our ~12 comment-aware checks and javadoc checks.

yes, but I usually recommend users to have two configurations - for code and javadoc ; and run javadoc less frequently on code.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 1, 2017

Member

I usually recommend users to have two configurations - for code and javadoc

@ps-sp We can also skip processing ordinary visits if ordinaryChecks is empty for full comment configurations.
If absolutely no checks are specified, we should still do parse as a check that code is parsable by grammar.

Member

rnveach commented Jun 1, 2017

I usually recommend users to have two configurations - for code and javadoc

@ps-sp We can also skip processing ordinary visits if ordinaryChecks is empty for full comment configurations.
If absolutely no checks are specified, we should still do parse as a check that code is parsable by grammar.

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Jun 1, 2017

Collaborator

@romani @rnveach acknowledged.

Collaborator

ps-sp commented Jun 1, 2017

@romani @rnveach acknowledged.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 1, 2017

Member

@rnveach

If absolutely no checks are specified, we should still do parse as a check that code is parsable by grammar.

that is not good for user. This is good for us only, but we always can use the most light Check to force parsing.
Lets skip parsing also if no Checks are defined, code for java and javadoc checks will be the same - consistent.

Member

romani commented Jun 1, 2017

@rnveach

If absolutely no checks are specified, we should still do parse as a check that code is parsable by grammar.

that is not good for user. This is good for us only, but we always can use the most light Check to force parsing.
Lets skip parsing also if no Checks are defined, code for java and javadoc checks will be the same - consistent.

@ps-sp ps-sp changed the title from Micro Optimization: Avoiding walking the trees unnecessarily to ASTs should be appended with comment nodes only when javadoc checks are present. ASTs shouldn't be walked if there are no corresponding types of checks Jun 1, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 4, 2017

Issue #4393: Comment nodes to be inserted only when javadoc checks ar…
…e present. ASTs not to be walked if there are no corresponding type of checks

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 5, 2017

Issue #4393: Comment nodes to be inserted only when javadoc checks ar…
…e present. ASTs not to be walked if there are no corresponding type of checks

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 5, 2017

Issue #4393: Comment nodes to be inserted only when javadoc checks ar…
…e present. ASTs not to be walked if there are no corresponding type of checks

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 5, 2017

Issue #4393: ASTs to be generated and walked only when there are corr…
…esponding type of checks. No parsing if no checks are specified

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 5, 2017

Issue #4393: ASTs to be generated and walked only when there are corr…
…esponding type of checks. No parsing if no checks are specified

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 5, 2017

Issue #4393: ASTs to be generated and walked only when there are corr…
…esponding type of checks. No parsing if no checks are specified

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 5, 2017

Issue #4393: ASTs to be generated and walked only when there are corr…
…esponding type of checks. No parsing if no checks are specified

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 5, 2017

Issue #4393: ASTs to be generated and walked only when there are corr…
…esponding type of checks. No parsing if no checks are specified

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 5, 2017

Issue #4393: ASTs to be generated and walked only when there are corr…
…esponding type of checks. No parsing if no checks are specified

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Jun 5, 2017

Issue #4393: ASTs to be generated and walked only when there are corr…
…esponding type of checks. No parsing if no checks are specified

@Vladlis Vladlis moved this from In Progress to Review in Javadoc style coverage and parser optimization Jun 6, 2017

rnveach added a commit that referenced this issue Jun 7, 2017

Issue #4393: ASTs to be generated and walked only when there are corr…
…esponding type of checks. No parsing if no checks are specified
@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 7, 2017

Member

Fix is merged

Member

rnveach commented Jun 7, 2017

Fix is merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment