Skip to content
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

Huge performance gap due to reading unnecessary metadata #515

Closed
Bananeweizen opened this issue Apr 12, 2023 · 2 comments · Fixed by #681
Closed

Huge performance gap due to reading unnecessary metadata #515

Bananeweizen opened this issue Apr 12, 2023 · 2 comments · Fixed by #681
Assignees

Comments

@Bananeweizen
Copy link
Collaborator

Bananeweizen commented Apr 12, 2023

#242 introduced some eclipse-metadata.yml. To my understanding that is not needed at all. AFAIK only the sevntu checks have that yaml file in their jar (because it was introduced with that PR). And even there it's not needed, because the sevntu modules are perfectly found and displayed by the normal checkstyle-metadata. Deleting that yaml related code doesn't make any difference for me. (I've debugged the entire metadata loading with the sevntu eclipse plugin in my workspace, and I can configure and execute them as before).

At the same time, scanning all classes on the classpath for those yaml files takes an eternity and is the main responsible for the slow initialization of metadata on first access to any rule (remember #375). Below image shows this taking 27.8 seconds of CPU time on my performance laptop.
grafik

Am I missing anything about those yaml files or can we really just get rid of this?

@rnveach
Copy link
Member

rnveach commented Apr 12, 2023

the sevntu checks have that yaml file in their jar
even there it's not needed, because the sevntu modules are perfectly found
Am I missing anything about those yaml files

I believe the purpose of the YAML was to provide extra data about the checks being presented in the metadata. I was not really involved in it's planning and development, but @romani should be able to confirm.

https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/eclipsecs-sevntu-plugin/src/com/github/sevntu/eclipse-metadata.yml
For example, eclipse-cs puts all the checks under categories. What category name to use for example is not provided in the other metadata. The YML has the name and descriptions to be used for this.
Sonar needs similar things. https://github.com/checkstyle/sonar-checkstyle/blob/eef33ccf76f73a35bb8579137ab003592dba722b/src/main/java/org/sonar/plugins/checkstyle/metadata/CheckstyleMetadata.java#L91-L92

the sevntu modules are perfectly found and displayed by the normal checkstyle-metadata

There are issues with sevntu since 8.36.1 and an issue has been sitting for a while. I tried providing fixes for the issue, but I believe my PRs were reverted because of some issues with the changes.
#316
sevntu-checkstyle/sevntu.checkstyle#857

scanning all classes on the classpath

Just so your aware, main Checkstyle does the same scanning when it needs to find a module that is not the FQCP and is not one of Checkstyle's main modules (shorthand or full name) ( https://checkstyle.org/checks.html ).

====

Your picture cuts off on how far down specifically the slowdown goes. I am curious if the specific call is something from CS or is connected to the YML.

If it is XmlMetaReader#readAllModulesIncludingThirdPartyIfAny (https://github.com/checkstyle/checkstyle/blob/54616f662130aa0ca5afe45f72d36cdefb78a781/src/main/java/com/puppycrawl/tools/checkstyle/meta/XmlMetaReader.java) then this doesn't read the YML, but all the metadata of the checks.
Example: https://github.com/checkstyle/checkstyle/blob/54616f662130aa0ca5afe45f72d36cdefb78a781/src/main/resources/com/puppycrawl/tools/checkstyle/meta/checks/annotation/AnnotationLocationCheck.xml

@romani
Copy link
Member

romani commented Jul 22, 2023

I though that all thirparty providers need to provide such details for eclipse-cs:
https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/7c5e2b668d191ff727687e9e381a08b044940aeb/eclipsecs-sevntu-plugin/src/com/github/sevntu/eclipse-metadata.yml#L1-L6

If we can optimize something wihtout loosing functionality - I am agree.

Looks like this yml was only for decaration purposes - sevntu-checkstyle/sevntu.checkstyle#822
previosly such data was in some other file(s). It might still there (I remember we discussed not removing old files for compatibility for some period of time), there is why it might works.

If we see way to improve, lets do this.

Bananeweizen added a commit to Bananeweizen/eclipse-cs that referenced this issue Mar 31, 2024
* don't scan the com.* package hierarchy
* calculate the common root packages of all known packages with checks
and metadata instead
* scans only the ...puppycrawl... and net.fs...samplechecks packages in
a default developer runtime now

fixes checkstyle#515
@Bananeweizen Bananeweizen self-assigned this Mar 31, 2024
Bananeweizen added a commit that referenced this issue Apr 1, 2024
* Classgraph is used in other Eclipse projects like Xtext, too, and is
actively maintained.
* Now finds the sevntu YML files again (which the previous reflections
based code did not find at all).

fixes #515
fixes #682
Bananeweizen added a commit that referenced this issue Apr 2, 2024
* Classgraph is used in other Eclipse projects like Xtext, too, and is
actively maintained.
* Now finds the sevntu YML files again (which the previous reflections
based code did not find at all).

fixes #515
fixes #682
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 a pull request may close this issue.

3 participants