performance: avoid O(n^2) in PDEJavaHelper

[jukzi]

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]

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]

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.

[jukzi]

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]

@vogella please undo "merge" and use rebase instead

[HannesWell]

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]

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

[HannesWell]

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]

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.

[jukzi]

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]

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]

Although it is now much longer than before.

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

[HannesWell]

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]

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

PRs welcome. i can review.