Community
Participate
Working Groups
Created attachment 182599 [details] screenshot I20101028-1441 Steps - Select the following 2 packages in PE org.eclipse.jdt.internal.ui.text.spelling org.eclipse.ui.texteditor.spelling - F4 - Couple of interfaces are repeated in the Type Hierarchy (see attached screenshots) This happens only when both packages are in different projects, and there is some class that implements the interface in both packages. But if both packages are in same project there is no duplication. Simple test case *Project X* package p; public interface IA { } package p; class A implements IA { void foo() { } } package p; class B implements IA { void foo() { } } *Project Y* package q; import p.IA; class C implements IA { void foo() { } } package q; import p.IA; class D implements IA { void foo() { } } Is the duplication deliberate and supposed to mean anything? Or it is just a bug?
Looks like a bug to me. Raksha please investigate.
Created attachment 182687 [details] Patch for UI From the example above, the Interface IA occurs in the hierarchy twice as the Region based hierarchy itself contains the same interface twice when the packages are in different projects but occurs only once when the packages containing types implementing the same interface in the same project. We can fix it in UI with a single line fix but the actual fix will have to be done in Core while calculating the region based hierarchy. We call the core API from org.eclipse.jdt.internal.ui.typehierarchy.TraditionalHierarchyViewer.TraditionalHierarchyContentProvider.getRootTypes(...) The core API org.eclipse.jdt.core.ITypeHierarchy.getRootInterfaces() which in turn calls org.eclipse.jdt.internal.core.hierarchy.TypeHierarchy.getAllInterfaces() returns the interface twice when the packages containing types implementing the same interface are in different projects. Here's the stack trace to how the interfaces are added to the hierarchy: Thread [Worker-8] (Suspended (breakpoint at line 200 in TypeHierarchy)) RegionBasedTypeHierarchy(TypeHierarchy).addInterface(IType) line: 200 RegionBasedHierarchyBuilder(HierarchyBuilder).connect(IGenericType, IType, IType, IType[]) line: 170 HierarchyResolver.reportHierarchy(IType, TypeDeclaration, ReferenceBinding) line: 532 HierarchyResolver.resolve(Openable[], HashSet, IProgressMonitor) line: 809 RegionBasedHierarchyBuilder.createTypeHierarchyBasedOnRegion(HashMap, IProgressMonitor) line: 90 RegionBasedHierarchyBuilder.build(boolean) line: 59 RegionBasedTypeHierarchy.compute() line: 97 RegionBasedTypeHierarchy(TypeHierarchy).refresh(IProgressMonitor) line: 1263 CreateTypeHierarchyOperation.executeOperation() line: 90 CreateTypeHierarchyOperation(JavaModelOperation).run(IProgressMonitor) line: 728 CreateTypeHierarchyOperation(JavaModelOperation).runOperation(IProgressMonitor) line: 788 JavaCore.newTypeHierarchy(IRegion, WorkingCopyOwner, IProgressMonitor) line: 3980 TypeHierarchyLifeCycle.createTypeHierarchy(IJavaElement[], IProgressMonitor) line: 314 TypeHierarchyLifeCycle.doHierarchyRefresh(IJavaElement[], IProgressMonitor) line: 328 TypeHierarchyLifeCycle.doHierarchyRefreshBackground(IJavaElement[], IProgressMonitor) line: 269 TypeHierarchyLifeCycle$2.run(IProgressMonitor) line: 222 Worker.run() line: 54 Dani, should I move this bug to Core and use the patch temporarily?
(In reply to comment #2) The bug is in the implementation of ITypeHierarchy.getRootInterfaces(), moving to JDT/Core. We don't need a workaround in JDT/UI, since this is not major, not a new problem, and JDT/Core has enough time to fix it for 3.7.
Jay, please investigate.
Created attachment 182787 [details] Proposted patch with test Raksha, could you please see if this patch is enough?
(In reply to comment #5) > Created an attachment (id=182787) [details] [diff] > Proposted patch with test > > Raksha, could you please see if this patch is enough? Yep, that would remove duplicacy. Thanks Jay.
(In reply to comment #5) > Created an attachment (id=182787) [details] [diff] Please make sure this doesn't introduce a performance regression. Unlike the field this.rootClasses, the list this.interfaces may contain a lot of elements, and if addInterface(..) is called often, then this could be a bottleneck (e.g. for a type hierarchy on the whole rt.jar). As an alternative, you could probably use the typeToSuperInterfaces map to quickly check whether the type is already known. That would replace the O(n) ArrayList#contains(..) with better scaling hash lookup.
(In reply to comment #7) > (In reply to comment #5) > > Created an attachment (id=182787) [details] [details] [diff] > > Please make sure this doesn't introduce a performance regression. Unlike the > field this.rootClasses, the list this.interfaces may contain a lot of elements, > and if addInterface(..) is called often, then this could be a bottleneck (e.g. > for a type hierarchy on the whole rt.jar). > > As an alternative, you could probably use the typeToSuperInterfaces map to > quickly check whether the type is already known. That would replace the O(n) > ArrayList#contains(..) with better scaling hash lookup. Thanks for the suggestion, Markus. I ran some performance tests and did notice some noticeable performance dip, albeit small, with the existing patch as is. Doing the contains check against typeToSuperInterfaces and inside HierarchyBuilder#connect() keeps the performance degrade negligible, if at all any. I will collect some numbers and post the new patch.
Created attachment 183646 [details] Updated patch This patch has lesser performance impact, if any. A region based hierarchy construction on rt.jar produces numbers like below. With HEAD: Elapsed Process: 5.86m CPU Time: 4.07m With the patch: Elapsed Process: 5.91m CPU Time: 4.06m Subsequent runs produces varying numbers at times the patch producing better numbers. But on an average, things haven't changed much with the patch.
Released in HEAD for 3.7 M4.
.
Verified.
