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

only scan relevant classpaths for metadata #681

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

Bananeweizen
Copy link
Collaborator

  • 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

This still finds all relevant metadata files, but takes 480 ms instead of 12 seconds on my laptop.

fixes #515

@rnveach
Copy link
Member

rnveach commented Mar 31, 2024

@rnveach
Copy link
Member

rnveach commented Mar 31, 2024

Unrelated but Reflections should be removed. It doesn't work as expected on newer JREs and the repo is abandoned. I brought this up before and to main repo as well.
#659 (comment)

@Bananeweizen
Copy link
Collaborator Author

@rnveach It finds that package when the sevntu plugin is running:
grafik

However, neither on master nor on my branch any metadata file is found by scanning the classpath.

Regarding the reflections library: If you know a good alternative, then please raise an issue for replacing reflections.org. Don't be confused, I will probably add it to the target platform in another PR soon, where I'm working on consuming the checkstyle.jar instead of the checkstyle-all.jar. Adding it is only to move the dependency to this project instead of taking it transitively from checkstyle.

@rnveach
Copy link
Member

rnveach commented Mar 31, 2024

However, neither on master nor on my branch any metadata file is found by scanning the classpath.

I assume this is related to other issues/comments on the matter as sevntu has been broken for a while. I am just confused if you say it won't load the YML or if it won't load something else?

If you know a good alternative

I believe Reflections right now is just doing the very basic load (common java code), so I would recommend doing that. No extra library needed. I do not remember the specifics as it has been a while since I originally looked into it.

@Bananeweizen
Copy link
Collaborator Author

I am just confused if you say it won't load the YML or if it won't load something else?

It doesn't find any YML. So there is no loading.

@rnveach
Copy link
Member

rnveach commented Apr 1, 2024

Thanks. So this probably related to the reflections library. Since it doesn't work properly in Java 9+, it isn't finding anything.

* 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 checkstyle#515
fixes checkstyle#682
@Bananeweizen
Copy link
Collaborator Author

@rnveach I've changed the implementation, and now the classgraph library is used instead. It finds the sevntu checks, but unfortunately it still uses 7 seconds of CPU time. Nevertheless I consider this change fixing both the performance gap as well as the reflections issue.

@Bananeweizen
Copy link
Collaborator Author

I'm going to merge this and the 2 PRs that are based on it. Feel free to still review and comment, so I can address comments separately, if necessary. However, I have some more classpath related changes (like trying to use the slim checkstyle.jar and to remove the guava dependencies) coming up, and all these changes are in conflict with each other because they change how the different plugin projects depend on each other.

@Bananeweizen Bananeweizen merged commit 60e51a1 into checkstyle:master Apr 2, 2024
7 checks passed
@Bananeweizen Bananeweizen deleted the performance branch April 2, 2024 10:20
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 this pull request may close these issues.

Huge performance gap due to reading unnecessary metadata
2 participants