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.

Sign up for GitHub

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

Jump to bottom

performance: avoid O(n^2) in PDEJavaHelper #747

Merged
merged 1 commit into from
Sep 26, 2023
Merged

performance: avoid O(n^2) in PDEJavaHelper #747

merged 1 commit into from
Sep 26, 2023

Conversation

jukzi
Copy link
Contributor

@jukzi jukzi commented Sep 18, 2023

findPackageFragmentRoot() searches through all PackageFragment Roots and is called for every libPaths. This can be slow due to involved file access.
see eclipse-jdt/eclipse.jdt.core#303

Instead call getAllPackageFragmentRoots() only once, index the result and use O(1) hash access.

@github-actions
Copy link

github-actions bot commented Sep 18, 2023 •
edited
Loading

Test Results

   261 files   -      12     261 suites   - 12   43m 41s ⏱️ - 21m 30s
3 341 tests +       1  3 308 ✔️  -        1  30 💤 ±  0  3 ❌ +3 
4 895 runs   - 5 422  4 850 ✔️  - 5 376  42 💤  - 48  3 ❌ +3 

For more details on these failures, see this check.

Results for commit 42c0a49. ± Comparison against base commit f5c8301.

♻️ This comment has been updated with latest results.

HannesWell
HannesWell reviewed Sep 18, 2023
View reviewed changes
Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

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

Isn't the caching better done in the IJavaProject implementation?
Then all callers benefit from it immediatly and the cache has to be build only once. And the impl probably also knows best when to invalidate the cache.

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/PDEJavaHelper.java Outdated Show resolved Hide resolved
@jukzi
Copy link
Contributor Author

jukzi commented Sep 19, 2023

Isn't the caching better done in the IJavaProject implementation?

i don't know who this could be implemented as the result currently depends on files/directory on filesystem, which could have changed.

Then all callers benefit from it immediatly and the cache has to be build only once. And the impl probably also knows best when to invalidate the cache.

together with eclipse-jdt/eclipse.jdt.ui#796 i just adapted all known repeated callers.

@jukzi
Copy link
Contributor Author

jukzi commented Sep 19, 2023

@vogella please undo "merge" and use rebase instead

@HannesWell
Copy link
Member

Isn't the caching better done in the IJavaProject implementation?

i don't know who this could be implemented as the result currently depends on files/directory on filesystem, which could have changed.

If it is querying the file-system over and over again it would be an even better reason to cache it for all since we both know that this is slow.

Is it querying the files directly through standard java API or through Eclipse IResources? Because in case of the later a IResourceChangeListener could be registered to get notified about deltas in the file-system.

@jukzi
Copy link
Contributor Author

jukzi commented Sep 20, 2023

Is it querying the files directly through standard java API or through Eclipse IResources?

no - see stacktrace in eclipse-jdt/eclipse.jdt.core#303 - java.io

@jukzi jukzi force-pushed the getAllPackageFragmentRoots branch from 1bd180a to ce63951 Compare September 20, 2023 06:12
@HannesWell
Copy link
Member

Is it querying the files directly through standard java API or through Eclipse IResources?

no - see stacktrace in eclipse-jdt/eclipse.jdt.core#303 - java.io

Too bad.
In generell one could use native Filesystem-Hooks, but that's probably a bit too complicated for this purpose.

HannesWell
HannesWell reviewed Sep 20, 2023
View reviewed changes
ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/PDEJavaHelper.java
if (classRootPath != null) {
rootsByPath.put(classRootPath, classpathRoot);
}
}
ListIterator<IPath> li = libPaths.listIterator();
while (li.hasNext()) {
Copy link
Member

Choose a reason for hiding this comment

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

Since you already touched this, could you please convert this while loop and if you want the outer for loop over all projects into an enhanced for loop?
Btw. I wonder why there is no quick-fix that suggest to convert this iterator+while into an enhanced for-loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is a cleanup, that could do this. but it currently fails. let's wait till that is fixed: eclipse-jdt/eclipse.jdt.ui#798

@HannesWell
Copy link
Member

In general wonder if the number of libraries is that often greater than one.
For a simple project base.getPluginBase().getLibraries() returns an empty array and thus libPaths only contains the project's path as single element.

For In that case it is probably faster to keep the current behavior since caching will then probably be more expensive because it collects all roots in any case, even if the first one would already match. Plus the memory overhead.

A all pleasing solution would probably be something like:

Function<IPath, IPackageFragmentRoot> findRootWithPath;                                 
if (libPaths.size() == 1) {                                                     
	findRootWithPath = p -> {                                                           
		try {                                                                   
			return jp.findPackageFragmentRoot(p);                               
		} catch (JavaModelException e) {                                        
			return null;                                                        
		}                                                                       
	};                                                                          
} else {                                                                        
	Map<IPath, IPackageFragmentRoot> rootsByPath = new HashMap<>();             
	for (IPackageFragmentRoot classpathRoot : jp.getAllPackageFragmentRoots()) {
		IPath classRootPath = classpathRoot.getPath();                          
		if (classRootPath != null) {                                            
			rootsByPath.put(classRootPath, classpathRoot);                      
		}                                                                       
	}                                                                           
	findRootWithPath = rootsByPath::get;                                                
}                                                                               
Optional<IPackageFragment> fragment = libPaths.stream().map(findRootWithPath).filter(Objects::nonNull)
	.map(root -> root.getPackageFragment(packageName)).filter(IPackageFragment::exists).findFirst();
if (fragment.isPresent()) {
	return fragment.get();
}

Although it is now much longer than before.

@jukzi
Copy link
Contributor Author

jukzi commented Sep 25, 2023

Although it is now much longer than before.

Thats not an even a performance improvement. findPackageFragmentRoot internally calls getAllPackageFragmentRoots anyway.

@jukzi jukzi force-pushed the getAllPackageFragmentRoots branch from ce63951 to 5256249 Compare September 25, 2023 08:52
@EcljpseB0T @HannesWell
performance: avoid O(n^2) in PDEJavaHelper
42c0a49 
findPackageFragmentRoot() searches through all PackageFragment Roots and
is called for every libPaths. This can be slow due to involved file
access.
see eclipse-jdt/eclipse.jdt.core#303

Instead call getAllPackageFragmentRoots() only once, index the result
and use O(1) hash access.
@HannesWell HannesWell force-pushed the getAllPackageFragmentRoots branch from 5256249 to 42c0a49 Compare September 25, 2023 22:45
@HannesWell
Copy link
Member

Although it is now much longer than before.

Thats not an even a performance improvement. findPackageFragmentRoot internally calls getAllPackageFragmentRoots anyway.

OK, then just disregard my remark and AFAICT this looks fine to me.

Maybe it would be worth to consider in JDT to make findPackageFragmentRoot() a short-circuit operation?

@jukzi jukzi merged commit a6601a5 into eclipse-pde:master Sep 26, 2023
11 of 14 checks passed
@jukzi jukzi deleted the getAllPackageFragmentRoots branch September 26, 2023 07:19
@jukzi
Copy link
Contributor Author

jukzi commented Sep 26, 2023

Maybe it would be worth to consider in JDT to make findPackageFragmentRoot() a short-circuit operation?

PRs welcome. i can review.

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

@HannesWell HannesWell HannesWell left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jukzi @HannesWell @EcljpseB0T