Community
Participate
Working Groups
Created attachment 86773 [details] proposed patch against HEAD (v_831) While implementing bug 61185 I would like to use a HierarchyScope with more fine grained control. The use case is to provide a type selection dialog constrained to subtypes of a given type and constrained to the current project. For that purpose HierarchyScope would have to be enhanced in to regards: (1) add a flag 'onlySubtypes' (2) add a field 'javaProject' For passing the javaProject down to the search engine also new API in SearchEngine and BasicSearchEngine would be required. Please see attached patch for details.
Also when working on bug 61185 I observed that the TypeNameMatchRequestorWrapper would also capture member types of types in the hierarchy, because on certain control flows scope.encloses() is only called for the IResource (-> matches) but not for the nested contained types (-> don't match). Note that acceptType() is the first method to actually retrieve the ITypes, so checking can probably _not_ happen earlier up the call stack. See this attachment for where I sensed the need for an additional call to scope.encloses(IJavaElement): https://bugs.eclipse.org/bugs/attachment.cgi?id=86635 Is answering member types the desired behavior in other use cases? Is this a bug and should be changed globally?
> Is answering member types the desired behavior in other use cases? > Is this a bug and should be changed globally? See also bug 215196. I can't think of a case where it would be useful to get member types when using a hierarchy scope. It would be OK for me to remove the "or members of the types in this hierarchy", especially since this spec has not been implemented correctly (at least in HEAD, I have not checked history).
While we are at it, I figured I would raise the question. Is there any way to do the HierarchySearch with two Hierarchies? In PDE we are using the HierarchySearch to quickly get a list of the valid java classes for an extension. Though most class type extensions specify either 0 or 1 classes/interfaces users must extend/implement, there is the option to make the user extend a class and implement an interface. In this case, it would be nice to show only the classes that extend the specified class and implemented the specified interface. I know this is probably outside the design of the HierarchyScope, but thought I would ask to see if we could achieve the desired outcome with minor changes.
Note that no API is really needed. One can create a hierarchy on a given type in a given Java project, then create a Java search scope that uses the subtypes in this hierarchy. E.g. IType focus = ...; IJavaProject project = ...; ITypeHierarchy hierarchy = focus.newTypeHierarchy(project, pm); IType[] subTypes = hierarchy.getAllSubTypes(focus); IJavaSearchScope scope = SearchEngine.createJavaSearchScope(subTypes); For the 2 hierarchies scope problem, you simply need to build the 2 hierarchies and intersect the subtypes from the 2 hierarchies, and create a Java search scope with these subtypes.
(In reply to comment #4) > Note that no API is really needed... hm, that's interesting! Would the effect be the same also in terms of performance (time&space)? One more plus: we could even use a ProgressMonitor to give feedback for longer searching (if relevant) ;-)
(In reply to comment #5) > Would the effect be the same also in terms of performance (time&space)? I believe so, since this we create a hierarchy inside the scope and keep it in the scope. And getAllSubTypes(...) performance is negligible comparing to the hierarchy creation performance.
OK, I tried the suggestion from comment 4, but actually it's not exactly what is needed here, since SearchEngine.createJavaSearchScope(subTypes) creates a scope that will also contain all nested classes within subTypes. What we need is compatible classes only. See also bug 215196 where a similar problem is reported against the hierarchy scope. In contrast to HierarchyScope I believe that createJavaSearchScope is right in considering nested types, too. So, if we go via createJavaSearchScope we'd like a variant with a new flag 'includeNested', which looks funny since the purpose of the method seems to be to find contained elements. From here I would think the original proposal might still be preferable (including a fix for 215196).
Agreed than my proposal from comment 4 doesn't work. We will add your proposal time permitting (tests need to be written first).
> OK, I tried the suggestion from comment 4, but actually it's not exactly what > is needed here, ... Stephan, thanks for following up with the JDT request to make sure it is what PDE is looking for! I appreciate all the time and effort you are putting into this.
Created attachment 89335 [details] updated patch with initial test (In reply to comment #8) > ... tests need to be written first ... I've updated the patch to include an initial test. Does this look like what you had in mind? Do we need more like that? So far, I'm testing the 'onlySubtypes' flag but not constraining the scope to a JavaProject. While writing this test, I found that my previous patch did not correctly handle multiple types within the same compilation unit. The updated patch handles that, too. The scope does not include the focus type (which is perfect for bug 61185). Hope that will be OK for other uses, too.
Now that M6 is out, this patch from January failed to make it into Ganymede? That's sad, because a good solution for bug 61185 actually depends on this. Bug 61185 in turn has been open since 2004-05-06. I really wish, I could have helped and did invest some time in this.
Sorry Stephan. We ran out of time to even review your patch. And my understanding was that a workaround had been implemented for bug 61185, so I didn't see this enhancement as high priority.
Created attachment 111360 [details] patch updated to HEAD Although currently marked as closed, bug 61185 still waits for the enhancement from the patch proposed here. See bug 61185 comment 23 for how the current solution is incomplete. The patch in this bug fixes 2 of the 3 mentioned problems. I'd appreciate any comments on the patch, which I just updated to HEAD for your convenience ;-)
(In reply to comment #13) > Created an attachment (id=111360) [details] > patch updated to HEAD > > Although currently marked as closed, bug 61185 still waits for > the enhancement from the patch proposed here. > See bug 61185 comment 23 for how the current solution is incomplete. > > The patch in this bug fixes 2 of the 3 mentioned problems. > > I'd appreciate any comments on the patch, which I just updated to HEAD > for your convenience ;-) > Thanks for the update. Here are my feedback on this patch: 1) IMO, the working copy owner should be also an argument of the new API method. Either simply added to the proposed method or add an other new API method which would have it. E.g. public static IJavaSearchScope createHierarchyScope( IJavaProject project, IType type, boolean onlySubtypes, WorkingCopyOwner owner) throws JavaModelException { 2) Not sure to understand why you used a HashSet subTypes in HierarchyScope. My understanding was that having created the hierarchy using the ITypeHierarchy.getAllSubtypes(...) method would have been enough, hence no need to modify the encloses(...) methods to get the correct answers... However, I'm not an expert in this code area... Jerome, could you please give your mind/advice on the proposed patch and my initial remarks? Thanks
(In reply to comment #14) > Here are my feedback on this patch: > 2) Not sure to understand why you used a HashSet subTypes in HierarchyScope. > My understanding was that having created the hierarchy using the > ITypeHierarchy.getAllSubtypes(...) method would have been enough, hence no need > to modify the encloses(...) methods to get the correct answers... Thanks for commenting. What you suggest wouldn't suffice because encloses(IJavaElement) doesn't use the result of getAllSubtypes (stored in field 'types'), but goes back to calling this.hierarchy.contains(type), which gives the wrong result for the requested feature. Needing to add an additional lookup I decided to use a HashSet, rather than iterating the 'types' array each time encloses is executed. (for comparison: encloses(String) uses an iterating strategy). Note that the loop, which I re-use for filling the HashSet, was already there. In order to reduce confusion (and footprint?) I'd suggest the opposite: convert field 'types' into a local of method buildResourceVector(), because that's how it is currently used(!).
(In reply to comment #14) > 1) IMO, the working copy owner should be also an argument of the new API > method. Either simply added to the proposed method or add an other new API > method which would have it. > E.g. > public static IJavaSearchScope createHierarchyScope( > IJavaProject project, > IType type, > boolean onlySubtypes, > WorkingCopyOwner owner) throws JavaModelException { Agreed. This API should replace the one proposed in comment 13. (In reply to comment #15) > In order to reduce confusion (and footprint?) I'd suggest the opposite: > convert field 'types' into a local of method buildResourceVector(), > because that's how it is currently used(!). Agreed. Having 'types' defined as a field seems overkill. For the spec of createHierarchyScope(...) I would explained a little bit more what 'onlySubtypes' means. In particular, if 'true' is 'type' also included'. Maybe give a small example. Also I'm surprised to see "or members of the types in this hierarchy." I thought you wanted compatible types only. I believe member if the types in the hierarchy will not be compatible with the focus type. Finally, this also needs tests. It looks like the patch contains some test data, but no tests.
(In reply to comment #16) Sorry for the delay, I'm currently attending http://oss2008.dti.unimi.it :) > For the spec of createHierarchyScope(...) I would explained a little bit more > what 'onlySubtypes' means. In particular, if 'true' is 'type' also included'. > Maybe give a small example. OK, I will expand the documentation a bit. > Also I'm surprised to see "or members of the types in this hierarchy." I > thought you wanted compatible types only. I believe member if the types in the > hierarchy will not be compatible with the focus type. This piece is inherited from the sibling method SearchEngine#createHierarchyScope(IType). The sentence reflects the current behavior, but as you assume correctly, it's not what we actually want. The intended behavior additionally depends on bug 215196. (But this patch on its own already improves the situation) > Finally, this also needs tests. It looks like the patch contains some test > data, but no tests. I'm sorry, the updated patch is broken. You may see the (terse) test implemention in attachment 89335 [details], or wait for an new patch (will upload by the end of this week). If you tell me that the test looks OK, just more needed, I will go ahead writing a few more variants.
Created attachment 114603 [details] Improved patch I finally found some time to improve the patch: 1. Since isolating this issue from bug 215196 creates an inconsistent situation (as pointed out in comment 16) I decided to address both issues in one patch. Instead of the quick&dirty proposal in attachment 86635 [details] in bug 61185 comment 13 I now propose a new API in IJavaSearchScope with default implementation in AbstractJavaSearchScope: /** * Extra check whether a given type is enclosed, after the * containing resource has already been found to be enclosed. * * @param type the given type. * @return <code>true</code> if the type is in this scope or if no * additional check is required after checking the resource. * @since 3.5 */ public boolean enclosesTypeOfEnclosedResource(IType type); HierarchyScope overrides the default implementation to perform additional checks if so required (depending on a new flag field 'noMemberTypes'). This solves the issue of incompatible types contained in the scope. 2. I added the WorkingCopyOwner parameter as suggested in comment 14. 3. I updated and expanded the JavaDoc a bit. 4. I converted HierarchyScope#types into a local variable to avoid the impression that this variable is used across method calls (see comment 15). 5. I updated the tests (test code was missing from previous patch): using the same test data the new functionality is tested along three different call chains. This should solve all issues in this bug plus the scenarios described in bug 215196. To see the old behavior (with 3 of 4 new tests failing) simply disable the field assignments in the new ctor of HierarchyScope. Future: Currently the new API from (1) is only called from TypeNameMatchRequestorWrapper. I'm not sure if other clients of TypeNameMatchRequestor should eventually benefit from this API, too. I looks like a AllTypeDeclarationsVisitor - local type in BasicSearchEngine - might be a candidate for future change. By construction all old scenarios should not be affected by these changes but only if a HierarchyScope is created using the new API, the new behavior applies. PS: I found the reason for the broken patch I previously attached: "create patch" silently dropped the file JavaSearchTests.java due to incompatible encodings of special character '§'! (see bug 214085 comment 4).
(In reply to comment #18) Unfortunately you cannot add a new method to an interface that is intended to be implemented by clients. If you show API Problems in the Problems view, you get the following error: Description Resource Path Location Type The method org.eclipse.jdt.core.search.IJavaSearchScope.enclosesTypeOfEnclosedResource(IType) in an interface that is intended to be implemented has been added IJavaSearchScope.java org.eclipse.jdt.core/search/org/eclipse/jdt/core/search line 79 Compatibility Problem
Created attachment 115095 [details] patch without change in IJavaSearchScope (In reply to comment #19) > Unfortunately you cannot add a new method to an interface that is intended to > be implemented by clients. Indeed unfortunate. I updated the patch to use some of those tricks I keep telling my students to avoid (static, instanceof, cast) ;-P > If you show API Problems in the Problems view, you > get the following error: [...] I didn't succeed to see this error. Probably, because I don't have a proper API baseline (tried the SDK I'm running, but then it is probably missing the necessary API meta data, right?). Is there a FAQ somewhere that tells me how to setup the API tooling for HEAD development? Or can API-baselines be shared as files??
ping :) anything else I could/should improve here?
(In reply to comment #20) > > If you show API Problems in the Problems view, you > > get the following error: [...] > > I didn't succeed to see this error. Probably, because I don't have a > proper API baseline (tried the SDK I'm running, but then it is > probably missing the necessary API meta data, right?). > Sorry, it looks like I missed your last comment. To setup an API baseline, simply follow Help > Contents > Plug-in Development Environment Guide > Tasks > API Tooling > Setting up an API baseline and point at a 3.4 Eclipse SDK. Otherwise, for the patch itself: - I don't understand "The Java elements resulting from a search with this scope will be types in this hierarchy." Does it mean that it won't work if I search for method declarations using this scope? What about if I search for references? - Why using 2 fields: private boolean onlySubtypes= false; private HashSet subTypes; // only set when onlySubtypes==true when only one ('subTypes') would be needed - Why do you need HierarchyScope.enclosesTypeOfEnclosedResource ? Ie. why HierarchyScope.encloses(IJavaElement) is not sufficient? - We try to only have 1 assert per test (even if some of our tests have several asserts). - Also please follow the FAQ at http://wiki.eclipse.org/JDT_Core_Committer_FAQ#How_should_I_format_my_code.3F
Created attachment 119647 [details] updated patch > Sorry, it looks like I missed your last comment. No problem ;-) New patch containing the following significant changes: 1. Split the large test into smaller tests. This revealed a problem with encloses() on an uninitialized scope (wrongly included focus type). Fixed. 2. Added more tests regarding member/enclosing types and reg. a method. This revealed more inconsistencies a la bug 215196. Added more analysis to encloses() to also find members types and enclosing types (unless 'noMemberTypes=true' was requested). Note, that this breaks existing tests (here: testMethodDeclaration04). But if consistency should be achieved, I'm afraid this change cannot be avoided (see bug 215196 for details). Leaving the broken test for you to accept or reject the new outcome. Bug 215196 suggests to expect nested but _not_ enclosing types. Instead, my patch adjusts HierarchyScope to mimic the existing behavior of searchAllTypeNames(), so we have to accept enclosing types, too. More answers: > - I don't understand "The Java elements resulting from a search with this > scope will be types in this hierarchy." Does it mean that it won't work if > I search for method declarations using this scope? What about if I search > for references? I see. I was trying to say, that member *types* are not considered. I revert to the original wording The Java elements resulting from a search with this scope will be types in this hierarchy, or members of the types in this hierarchy. and leave the detail to the paragraph describing the effect of 'noMemberTypes'. > - Why using 2 fields: > private boolean onlySubtypes= false; > private HashSet subTypes; // only set when onlySubtypes==true > when only one ('subTypes') would be needed Just matters of taste: I tend to trust an explicit boolean flag more than checking a reference for null (which could mean very different things). New patch follows your suggestion. > - Why do you need HierarchyScope.enclosesTypeOfEnclosedResource ? Ie. why > HierarchyScope.encloses(IJavaElement) is not sufficient? Well, after the changes for item (2) above this method is indeed obsolete, leaving only one instanceof check in TypeNameMatchRequestorWrapper#accept. > - Also please follow the FAQ at > http://wiki.eclipse.org/JDT_Core_Committer_FAQ#How_should_I_format_my_code. I'm not 100% sure what you mean (I've seen the FAQ before) but I tried to stay closer to the formatting found in those files (although my monitor is not wide enough to display a 180 character method signature on one line :( ) (oops, "var= val;" was JDT/UI style ;-) Lastly regarding API tooling: > To setup an API baseline, > simply follow Help > Contents > Plug-in Development Environment Guide > > Tasks > API Tooling > Setting up an API baseline > and point at a 3.4 Eclipse SDK. I did exactly what is described there, but it doesn't let me see the error message. I repeated the procedure today on a different machine, and this time API tools complain: The major version should be incremented in version 3.5.0.qualifier, since API breakage occurred since version 3.4.2.v_883_R34x In the quickfix details it complains that tens or hundrets of classes have been removed (which are not). And still I'm not seeing the expected error if I add a method to IJavaSearchScope plus implementing classes. Does any of this sound like I should file a bug against API tooling? Or am I doing something really stupid? (I'm running SDK 3.4.1 with source projects from HEAD).
ping ;-) Is it already too late for M5? Should we try to get this into M6?
Sorry, it is getting harder and harder for me to find time to review patches. Anyway I'll try to find some time this week and post comments here.
1. Two errors in Problems view: Description Resource Path Location Type Missing @since tag on createHierarchyScope(IJavaProject, IType, boolean, boolean, WorkingCopyOwner) SearchEngine.java org.eclipse.jdt.core/search/org/eclipse/jdt/core/search line 194 @since tag problem The import org.eclipse.jdt.core.search.IJavaSearchScope is never used HierarchyScope.java org.eclipse.jdt.core/search/org/eclipse/jdt/internal/core/search line 21 Java Problem 2. HierarchyScope.buildResourceVector() Changed type.resource() to type.getResource(). Why? 3. TypeNameMatchRequestorWrapper.acceptType(int, char[], char[], char[][], String, AccessRestriction) a. Why the special check for HierarchyScope? b. Test looks bogus: the type is accepted if it is not included in the scope c. What are the performance implication? (e.g. in the Open Type dialog case)
Created attachment 123432 [details] updated patch Jerome, thanks for your comments. I appreciate your taking the time. > 1. Two errors in Problems view: Fixed. (the @since tag problem I didn't see, since I never got API tooling to work correctly :( ) > 2. HierarchyScope.buildResourceVector() > Changed type.resource() to type.getResource(). Why? Just to get rid of the cast to JavaElement. I see now that this trades a cast for one more method call. Behavior is the same. Please feel free to change back, if you prefer so. > 3. TypeNameMatchRequestorWrapper.acceptType(int, char[], char[], char[][], > String, AccessRestriction) > a. Why the special check for HierarchyScope? To limit the change to the use case at hand. Should other kinds of scope be affected, too? I have no indications for that. > b. Test looks bogus: the type is accepted if it is not included in the scope Test is saying: if (isHierarchScope implies encloses) ... encoding (a => b) as (!a || b). (Not does not apply to encloses). > c. What are the performance implication? (e.g. in the Open Type dialog case) I'm not sure how to measure this, indeed. However, Open Type dialog shouldn't be affected, because non-hierarchy scope only eval the 'instanceof', not the 'encloses' part. I've fixed the two errors (1) and updated to head. Regarding (2) you may choose, regarding (3) I see no need for action, which is also backed by the tests.
Please let's not again miss the deadline for API freeze? My latest patch should to the best of my knowledge satisfy all your comments.
Jérôme, could you please give your comments on the last patch from Stephan and then reassign to Srikanth? Thanks.
Frédéric, Since this concerns search, could you please review the patch?
(In reply to comment #30) > Frédéric, > > Since this concerns search, could you please review the patch? > I'm still seeing several issues with the last patch: 1) There's one failure in search tests: JavaSearchTests.testMethodDeclaration04() 2) Point 2) of comment 26: IMO, there's more than that as the getResource() returns null for an external resources although resource() does not. Hence, I would prefer this change to be reverted... 3) Point 3.a) of comment 26 is definitely annoying: I ran the added tests and see that the additional call to encloses() method was added to make testSearchScope15() working. But the search all type names should only uses scopes to figure out what indexes have to be looked up. Adding a call to ask the scope if it encloses the accepted type would break this behavior and is not acceptable for performances potential issues as noticed Jerome - and I fully agree with this argument. So, IMO this change should also be reverted. I'm definitely sorry but my opinion is that this patch cannot be accepted like this for these reasons. Please fix the failing test and revert changes 2) and 3) and I'll be happy to review the next patch.
Frédéric, thanks for your comments. I will answer one item at a time. (In reply to comment #31) > I'm still seeing several issues with the last patch: > > 1) There's one failure in search tests: > JavaSearchTests.testMethodDeclaration04() Yes, this breakage is intrinsic as mentioned in comment 23 (item 2). It hold that the new outcome (with my patch applied) is actually the CORRECT outcome wrt the spec of SearchEngine#createHierarchyScope(IType): " ... The Java elements resulting from a search with this scope will be types in this hierarchy, or members of the types in this hierarchy." The crux is: this portion "or members of" is implemented inconsistently as reported in bug 215196. I.e., depending on cache state the hierarchy scope can currently produce either outcome!! The said test happens to test the variant that is against the spec. With my patch in place the decision no longer depends on cache state but on an explicit flag. To see the old test result simply change from SearchEngine.createHierarchyScope(type) to SearchEngine.createHierarchyScope(null, type, false, true, null) I recognize that this makes the proposed change a sensitive issue, but how to procede with the current inconsistency/bug ?
(In reply to comment #31) > 2) Point 2) of comment 26: IMO, there's more than that as the getResource() > returns null for an external resources although resource() does not. > Hence, I would prefer this change to be reverted... Sorry, I missed that difference. So I agree to revert this part.
(In reply to comment #31) > 3) Point 3.a) of comment 26 is definitely annoying: I ran the added tests and > see that the additional call to encloses() method was added to make > testSearchScope15() working. But the search all type names should only uses > scopes to figure out what indexes have to be looked up. Adding a call to ask > the scope if it encloses the accepted type would break this behavior and > is not acceptable for performances potential issues as noticed Jerome - and > I fully agree with this argument. So, IMO this change should also be > reverted. OK, this is the most difficult to answer. If I get you right, you're saying that for performance reasons, searchAllTypeNames() must not use scope.encloses(). From the current implementation this seems to lead to the conclusion that an index based search for types of a hierarchy without members and enclosings is impossible to implement. :( The problem is that the resource vector of a hierarchy scope ("resourcePaths") covers more types than the set of contained types ("types" as retrieved from the type hierarchy). If search is based on resources only, we MUST admit ALL types in the given resources. The check that I inserted sits exactly at the only location that I could find that has all required information at hand to filter out unwanted types that are contained in resources of the hierarchy. At the point where HierarchyScope.encloses(IJavaElement) detects a false positive the stack looks like this (parameters omitted for readability): 9. HierarchyScope.encloses() line: 336 8. TypeNameMatchRequestorWrapper.acceptType() line: 112 7. BasicSearchEngine$4.acceptIndexMatch() line: 1364 6. MultiTypeDeclarationPattern(SearchPattern).acceptMatch() line: 299 5. MultiTypeDeclarationPattern(SearchPattern).findIndexMatches() line: 2124 4. MatchLocator.findIndexMatches() line: 264 3. PatternSearchJob.search() line: 97 2. PatternSearchJob.execute() line: 63 1. IndexManager(JobManager).performConcurrentJob() line: 276 0. BasicSearchEngine.searchAllTypeNames() line: 1374 Here frame 5 iterates over all index matches. Frame 6 checks for each match whether the containing resource is contained in the hierarchy (ignoring that actual type): scope.encloses(documentPath) So we invoke frame 7 once for each type in a matching document. Only in frame 8 we retrieve the corresponding IType, but this contains false positives, as mentioned. The best attempt I could imagine is guarding the call to this.scope.encloses(type) with a check whether filtering of members/enclosing is desired. If not, skip the test. If you don't indicate differently, I will prepare an updated patch with this additional guard plus the revert promised in comment 33. If, OTOH, I would revert this change, my patch would further aggrevate the existing inconsistency of bug 215196.
(In reply to comment #32) It's OK for me to change the test expected result (In reply to comment #33) OK (In reply to comment #34) > (In reply to comment #31) > > If I get you right, you're saying that for performance reasons, > searchAllTypeNames() must not use scope.encloses(). From the current > implementation this seems to lead to the conclusion that an index based > search for types of a hierarchy without members and enclosings is > impossible to implement. :( > Not impossible, but surely would have worst performance than current implementation. The search all type names functionality design is entirely focused on performance aspect. The main reason for that is that it's mainly used by the Open Type dialog and Code Assist and both should perform as fast as possible even with a huge number of types in the workspace... > The problem is that the resource vector of a hierarchy scope ("resourcePaths") > covers more types than the set of contained types ("types" as retrieved > from the type hierarchy). > If search is based on resources only, we MUST admit ALL types in the > given resources. > Yes, you must admit all types, but your requestor can filter member types after. The will only slow your specific functionality and not the basic ones... > The check that I inserted sits exactly at the only location that > I could find that has all required information at hand to filter out > unwanted types that are contained in resources of the hierarchy. > > At the point where HierarchyScope.encloses(IJavaElement) detects a false > positive the stack looks like this (parameters omitted for readability): > > 9. HierarchyScope.encloses() line: 336 > 8. TypeNameMatchRequestorWrapper.acceptType() line: 112 > 7. BasicSearchEngine$4.acceptIndexMatch() line: 1364 > 6. MultiTypeDeclarationPattern(SearchPattern).acceptMatch() line: 299 > 5. MultiTypeDeclarationPattern(SearchPattern).findIndexMatches() line: 2124 > 4. MatchLocator.findIndexMatches() line: 264 > 3. PatternSearchJob.search() line: 97 > 2. PatternSearchJob.execute() line: 63 > 1. IndexManager(JobManager).performConcurrentJob() line: 276 > 0. BasicSearchEngine.searchAllTypeNames() line: 1374 > > Here frame 5 iterates over all index matches. > Frame 6 checks for each match whether the containing resource > is contained in the hierarchy (ignoring that actual type): > scope.encloses(documentPath) > So we invoke frame 7 once for each type in a matching document. > Only in frame 8 we retrieve the corresponding IType, but this > contains false positives, as mentioned. > > The best attempt I could imagine is guarding the call to > this.scope.encloses(type) with a check whether filtering of > members/enclosing is desired. If not, skip the test. > If you don't indicate differently, I will prepare an updated patch > with this additional guard plus the revert promised in comment 33. > > If, OTOH, I would revert this change, my patch would further aggrevate > the existing inconsistency of bug 215196. > So, my proposal would be to revert the change done in TypeNameMatchRequestorWrapper and change the requestor of the added test testSearchScope15() which will fail without this change as follow: TypeNameMatchCollector collector = new TypeNameMatchCollector() { public void acceptTypeNameMatch(TypeNameMatch match) { if (scope.encloses(match.getType())) { super.acceptTypeNameMatch(match); } } public String toString(){ return toFullyQualifiedNamesString(); } };
Hi Frederic, (In reply to comment #35) > [...] The search all type names functionality design is entirely > focused on performance aspect. The main reason for that is that it's mainly > used by the Open Type dialog and Code Assist and both should perform as fast as > possible even with a huge number of types in the workspace... So is performance the reason why the spec contains "or members of the types in this hierarchy"? See, e.g., comment 2 for puzzlement about why this is there. That's good to know. > So, my proposal would be to revert the change done in > TypeNameMatchRequestorWrapper and change the requestor of the added test > testSearchScope15() which will fail without this change as follow: > > TypeNameMatchCollector collector = new TypeNameMatchCollector() { > public void acceptTypeNameMatch(TypeNameMatch match) { > if (scope.encloses(match.getType())) { > super.acceptTypeNameMatch(match); > } > } > public String toString(){ > return toFullyQualifiedNamesString(); > } > }; If we follow this road, the original use case would also need a small change in the JDT/UI, because that's where the requestor sits. (Original use case == open type dialog filtered to true sub-types of a given root type). I will attach the required changes re JDT/UI in a minute. It would be great if you could help convince them of this solution. On the other road I'm not yet convinced that my current patch (from comment 27 - with changes as described in comment 34) actually does cause a performance penalty for any mass operation: 1. Unless a hierarchy scope is used only one instanceof check is performed (the performance critical calls to searchAllTypeNames use a workspace scope, right?) 2. Even if a hierarchy scope is used the additional check is only invoked for those types that already passed the index search, which shouldn't be a huge number unless you ask for the hierarchy of Object or Serializable or the like. Note that in bug 61185 we already had to disable searching for the hierarchy of j.l.Object to pretect against OOME, and that was without the patch of this bug. Apart from this handwaving I'd be happy to learn how to use the performance tests to gather useful numbers. Going either road would be fine with me, although I'm still leaning towards implementing this solely in the core to improve consistency (bug 215196).
Created attachment 145834 [details] corresponding jdt/ui patch This patch would change the requestor that is used in the filtered type selection dialog. Note, that the direct requestor (TypeSearchRequestor) doesn't have the scope to check, but TypeItemsFilter can easily do so (and already does in other situations). With this patch accepted by JDT/UI the change to TypeNameMatchRequestorWrapper would be obsolete.
Adding dependency to the corresponding JDT/UI issue.
The chosen approach looks like it's the "right" fix. I have not checked every single change in HierarchyScope for correctness, though. The current behavior of hierarchy scope is inconsistent, and adding the necessary checks in the client code would not be more performant than doing it correctly in jdt.core. Since the change will only affect hierarchy scopes, this won't be an issue for the performance-critical calls to searchAllTypeNames(..).
Created attachment 147723 [details] once more updated patch This patch reflects my thinking with a few updates: * reverted type.getResource() back to ((IJavaElement)type).resource() as requested in comment 31 point 2) * split testMethodDeclaration04 into two cases: - new call explicitly requesting the old behavior - original call with new expected result See discussion in comment 32 * added additional wrapper method HierarchyScope.enclosesFineGrained, so that calls from TypeNameMatchRequestorWrapper will skip tests if no specific tests have been requested. This implements what I proposed in comment 34 bottom part. I believe this is the best we can do to reconcile performance with consistency. * improved javadoc and inline comments Given that Markus doesn't seem overly happy about having the UI work around an inconsistency in the Core, my vote goes for the solution in this patch. If Core and UI agree on moving that one check from the Core to the clients, go ahead, but please let's get this issue off the table, no?
(In reply to comment #40) > Created an attachment (id=147723) [details] > once more updated patch > ... I'll run performance tests with this patch and if no regression is noticeable then I think we could release it...
If Markus is fine with this patch, please proceed.
I've run JDT/Core Search performance tests with this patch and did not see any noticeable regression. So, as my concern about performance was not justified, I'm fine with this last patch... Markus, if you give a +1 on this patch, I would be happy to release it in HEAD and then close this bug long story (Stephan should have a medal for his patience!)...
Looks good and works fine for me. I tested it with PDEJavaHelperUI.selectType(IResource, int, String, String) updated to use searchScope = SearchEngine.createHierarchyScope(javaProject, superType, true, true, null); Just the Javadoc of the new SearchEngine.createHierarchyScope(IJavaProject, ...) needs a bit of polish: - Paragraphs should be separated with <p> (check it in the Javadoc hover) - Could we also allow <code>null</code> for 'project' (meaning a hierarchy for the whole workspace)? It worked fine for me and I think it could be useful. - description of 'owner' needs ", or <code>null</code> if the primary working copy owner should be used" We typically have the overloaded method with the most parameters at the end, so I would move the new one after #createHierarchyScope(IType, WorkingCopyOwner)
Great stuff guys! Is this targeted for Eclipse 3.5 M3?
(In reply to comment #44) > I tested it with PDEJavaHelperUI.selectType(IResource, int, String, String) > updated to use > searchScope = SearchEngine.createHierarchyScope(javaProject, superType, > true, true, null); > Just the Javadoc of the new SearchEngine.createHierarchyScope(IJavaProject, > ...) needs a bit of polish: > - Paragraphs should be separated with <p> (check it in the Javadoc hover) > - Could we also allow <code>null</code> for 'project' (meaning a hierarchy for > the whole workspace)? It worked fine for me and I think it could be useful. > - description of 'owner' needs ", or <code>null</code> if the primary working > copy owner should be used" > > We typically have the overloaded method with the most parameters at the end, so > I would move the new one after #createHierarchyScope(IType, WorkingCopyOwner) Frédéric, please polish the documentation as suggested by Markus and release the code. Thanks to everyone involved in this bug report!
Many thanks also from me :)
(In reply to comment #44) > - Could we also allow <code>null</code> for 'project' (meaning a hierarchy for > the whole workspace)? It worked fine for me and I think it could be useful. Yes, null is OK here, the HierarchyScope explicitly checks for null and uses a workspace type hierarchy if no project is given.
(In reply to comment #46) > Frédéric, please polish the documentation as suggested by Markus and release > the code. > Thanks to everyone involved in this bug report! As I definitely do not want steal the great work that Stephan did here, I think it would be better that he writes the Markus' proposed polishing and posts the final patch...
Created attachment 148863 [details] polished patch > As I definitely do not want steal the great work that Stephan did here, I hadn't looked at it that way ;-) but speaking of credits: I added my name to the license headers (style copied from other files), d'accord? > I think it would be better that he writes the Markus' proposed polishing and > posts the final patch... Sure. Here it is - I also applied his suggestions to BasicSearchEngine (method order) and HierarchyScope (param doc).
Comment on attachment 148863 [details] polished patch Adding your name into the headers was the right thing to do! Hence setting the iplog and review flag on this patch. I'll release it in a few minutes...
Released for 3.6M3 in HEAD stream.
Verified for 3.6M3 using I20091027-0100
