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

[performance] optimized ClasspathDirectory #3433

Merged
merged 2 commits into from
Dec 10, 2024

Conversation

jukzi
Copy link
Contributor

@jukzi jukzi commented Dec 10, 2024

  • java.io.File.list() returns null if the file is not a directory, so File.isDirectory() is not needed.
  • use ConcurrentHashMap instead of Hashtable
  • avoid rawtypes

ClasspathDirectory.directoryList(String) is a hotspot for example during NullTypeAnnotationTest

* java.io.File.list() returns null if the file is not a directory, so
File.isDirectory() is not needed.
* use ConcurrentHashMap instead of Hashtable
* avoid rawtypes

ClasspathDirectory.directoryList(String) is a hotspot for example during
NullTypeAnnotationTest
@jukzi
Copy link
Contributor Author

jukzi commented Dec 10, 2024

This PR reduces NullTypeAnnotationTest from 102 sec to 99 sec
image
image

@@ -360,7 +358,7 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) {
@Override
public void reset() {
super.reset();
this.directoryCache = new Hashtable(11);
this.directoryCache.clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know why teh secondaryTypes are not cleared?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also wondered about that. i decided to only make a performance change here keeping logic as is.

Copy link
Member

Choose a reason for hiding this comment

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

@mpalat : any clue? packageSecondaryTypes field was added in c39467a

Copy link
Contributor

@mpalat mpalat Dec 10, 2024

Choose a reason for hiding this comment

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

@mpalat : any clue? packageSecondaryTypes field was added in c39467a

I believe its a bug - @jukzi could you please add code to clear this also in reset alongwith this PR. If not I can create another PR to address this. please let me know

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

@jukzi jukzi force-pushed the ClasspathDirectory branch from f99d971 to 9c059ca Compare December 10, 2024 14:51
@jukzi jukzi merged commit fcbbaf5 into eclipse-jdt:master Dec 10, 2024
10 checks passed
@jukzi jukzi deleted the ClasspathDirectory branch December 10, 2024 17:26
@mpalat
Copy link
Contributor

mpalat commented Dec 12, 2024

@jukzi There is an issue in this code:
String[] cached = this.directoryCache.computeIfAbsent(qualifiedPackageName, this::computeDirectoryList);
Here this could result in recursive computeIfAbsent() which may end up in IllegalStateException due to recursive update since the doesFileExist also may call this on the same ConcurrentHashMap - This needs to be corrected

raised issue #3445 for the same. Thanks

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

Successfully merging this pull request may close these issues.

5 participants