Community
Participate
Working Groups
STEPS TO REPRO 1) create a Java project 2) create the following class in it: package a; public class MyClass { public String getName() { return null; } } 3) try to refactor getName() method name to getSomething() OBSERVED BEHAVIOUR The "checking preconditions" phase takes tens of seconds to complete in the test project in an almost empty workspace. Using a real-world project in a workspace with about 50 open projects, it takes AGES (that is: tens of minutes). The problems seems to rely on the name of the method, which is "getName()". If the name is something else, the "checking precondition" phase is slow, but succeeds within some tens of seconds even in the huge workspace case. EXPECTED BEHAVIOUR I don't know what the "checking preconditions" phase does, but if getName() is public and MyClass is in a project which is not referenced by any other project, I think the refactoring process should be almost instantaneous, especially because the test project is made up of just MyClass class... In a real-world case I would expect it to terminate quickly anyway...
The dump test has been performed using Eclipse 3.7 on a almost new workspace (no Java Projects in it). The real-world case was reproduced with Eclipse 3.6.2. While waiting for "checking preconditions" I tried to capture some thread dumps. Here is one: Stack trace: org.eclipse.jdt.internal.core.index.DiskIndex.addQueryResults(DiskIndex.java:203) org.eclipse.jdt.internal.core.index.Index.query(Index.java:138) org.eclipse.jdt.internal.core.search.matching.SuperTypeReferencePattern.queryIn(SuperTypeReferencePattern.java:272) org.eclipse.jdt.core.search.SearchPattern.findIndexMatches(SearchPattern.java:2295) org.eclipse.jdt.internal.core.search.matching.MatchLocator.findIndexMatches(MatchLocator.java:338) org.eclipse.jdt.internal.core.search.PatternSearchJob.search(PatternSearchJob.java:96) org.eclipse.jdt.internal.core.search.SubTypeSearchJob.search(SubTypeSearchJob.java:44) org.eclipse.jdt.internal.core.search.PatternSearchJob.execute(PatternSearchJob.java:63) org.eclipse.jdt.internal.core.search.processing.JobManager.performConcurrentJob(JobManager.java:276) org.eclipse.jdt.internal.core.hierarchy.IndexBasedHierarchyBuilder.searchAllPossibleSubTypes(IndexBasedHierarchyBuilder.java:523) org.eclipse.jdt.internal.core.hierarchy.IndexBasedHierarchyBuilder.determinePossibleSubTypes(IndexBasedHierarchyBuilder.java:406) org.eclipse.jdt.internal.core.hierarchy.IndexBasedHierarchyBuilder.build(IndexBasedHierarchyBuilder.java:120) org.eclipse.jdt.internal.core.hierarchy.TypeHierarchy.compute(TypeHierarchy.java:300) org.eclipse.jdt.internal.core.hierarchy.TypeHierarchy.refresh(TypeHierarchy.java:1263) - locked org.eclipse.jdt.internal.core.hierarchy.TypeHierarchy@2b77f99d org.eclipse.jdt.internal.core.CreateTypeHierarchyOperation.executeOperation(CreateTypeHierarchyOperation.java:90) org.eclipse.jdt.internal.core.JavaModelOperation.run(JavaModelOperation.java:728) org.eclipse.jdt.internal.core.JavaModelOperation.runOperation(JavaModelOperation.java:788) org.eclipse.jdt.internal.core.BinaryType.newTypeHierarchy(BinaryType.java:918) org.eclipse.jdt.internal.corext.refactoring.rename.RippleMethodFinder2.getCachedHierarchy(RippleMethodFinder2.java:321) org.eclipse.jdt.internal.corext.refactoring.rename.RippleMethodFinder2.findAllRippleMethods(RippleMethodFinder2.java:274) org.eclipse.jdt.internal.corext.refactoring.rename.RippleMethodFinder2.getAllRippleMethods(RippleMethodFinder2.java:168) org.eclipse.jdt.internal.corext.refactoring.rename.RippleMethodFinder2.getRelatedMethods(RippleMethodFinder2.java:161) org.eclipse.jdt.internal.corext.refactoring.rename.RenameMethodProcessor.initializeMethodsToRename(RenameMethodProcessor.java:236) org.eclipse.jdt.internal.corext.refactoring.rename.RenameMethodProcessor.doCheckFinalConditions(RenameMethodProcessor.java:360) org.eclipse.jdt.internal.corext.refactoring.rename.RenameVirtualMethodProcessor.doCheckFinalConditions(RenameVirtualMethodProcessor.java:143) org.eclipse.jdt.internal.corext.refactoring.rename.JavaRenameProcessor.checkFinalConditions(JavaRenameProcessor.java:46) org.eclipse.ltk.core.refactoring.participants.ProcessorBasedRefactoring.checkFinalConditions(ProcessorBasedRefactoring.java:224) org.eclipse.ltk.core.refactoring.Refactoring.checkAllConditions(Refactoring.java:160) org.eclipse.jdt.internal.ui.refactoring.RefactoringExecutionHelper$Operation.run(RefactoringExecutionHelper.java:80) org.eclipse.jdt.internal.core.BatchOperation.executeOperation(BatchOperation.java:39) org.eclipse.jdt.internal.core.JavaModelOperation.run(JavaModelOperation.java:728) org.eclipse.core.internal.resources.Workspace.run(Workspace.java:1975) org.eclipse.jdt.core.JavaCore.run(JavaCore.java:4777) org.eclipse.jdt.internal.ui.actions.WorkbenchRunnableAdapter.run(WorkbenchRunnableAdapter.java:106) org.eclipse.jface.operation.ModalContext$ModalContextThread.run(ModalContext.java:121) Please note that I have an Athlon 64 X2 5200+ processor with 4GB RAM and a Corsair Force F60 SSD in it, so I think the system performance is adeguate. Using 64-bit builds of Eclipse and a Java 1.6.0_22 64-bit JRE.
(In reply to comment #1) > The dump test has been performed using Eclipse 3.7 on a almost new workspace > (no Java Projects in it). Sorry, I meant "the dumb test has been performed...".
Wow, looks real slow indeed! :( Satyam, can you please check? Thanks!
getName() is present in many classes in the JVM's rt.jar. Refactor calls search for method declarations of getName() and not MyClass#getName(), causing the slow down. Markus, is there a reason for this?
(In reply to comment #4) Yes. The Rename refactoring has to walk up and down the hierarchy to find all methods that are connected to the renamed method. That's because the method could be inherited from multiple supertypes, and we have to find the whole "ripple" of methods that have to be renamed together. Since walking down the type hierarchy is extremely expensive (need to create subtype hierarchies), the current implementation first finds all methods with matching signatures and then connects them via supertype hierarchies.
Looking at the code again, I think the expensive part is this situation: package a; public class MyClass implements I { public String getXXX() { return null; } } interface I { public String getXXX(); } interface J { public String getXXX(); } interface Both extends I, J { } The problem is that J#getXXX() also needs to be renamed, but we cannot find out whether there's a "connector" like interface "Both" without creating subtype hierarchies. Taking this bug to JDT/UI, since the problem is in our implementation. We have to check if we can avoid processing the rt.jar in this case. Maybe fixing the "//TODO: would only need subtype hierarchies of all top-of-ripple relatedTypesToProcess" in RippleMethodFinder2 would also help.
Hi Nikolay, here's another bug for you.
I am going to give it a go. Wish me luck.
Good luck! :-D And, seriously, thanks in advance!
Hello, Can I ask if RippleMethodFinderTests is supposed to be working? It seems that for some reason on my machine when I run all the unit tests some of them fail because they interfere with each other. However when I run each test individually they pass. It is a little tricky to know if I have broken stuff.
Created attachment 236488 [details] proposed patch This contribution complies with http://www.eclipse.org/legal/CoO.php It seems to me there is no need to search rt.jar and other system jars for ripple methods. So I have changed the code not to do that. I ran all the unit tests that use RippleMethodFinder2 and all of them passed. The 2nd change which is very similar is to create a RefactoringSearchScope that only search in sources. Seeing as we are refactoring it seemed to me we don't care about things we cannot refactor (binary sources). Is my reasoning sound?
Created attachment 236491 [details] proposed patch This contribution complies with http://www.eclipse.org/legal/CoO.php Forgot to update the headers of the changed files.
Looks like the 2nd change breaks BinaryReferenceTests. The only reason refactorings search binary sources is to report potential errors. I still think the 1st change is valid though. It does speed things up and it doesn't break any tests.
Noopur, please ignore my patch. I have a much better idea.
Comment on attachment 236491 [details] proposed patch (In reply to Nikolay Metchev from comment #14) > Noopur, please ignore my patch. I have a much better idea.
My idea was to use structural search instead of the SearchEngine. So I tried replacing RippleMethodFinder2.findAllDeclarations with a traversal up and down the hierarchy to find all overrides. This sped things along but several unit tests failed including RenameVirtualMethodInClassTests.testGenerics1. This prompted me to file bug 420055. I think for now I have to wait for bug 420055 to be fixed before I can proceed here. I am going to try to fix it myself but it doesn't look like it is a quick fix.
Created attachment 236802 [details] proposed patch This contribution complies with http://www.eclipse.org/legal/CoO.php I have spent some time profiling and here is what I have discovered. The slow bit is definitely the building of the hierarchy. So if we can reduce the number of time we have to do that we should get a performance boost. At the moment RippleMethodFinder2 in order to find all declarations it first uses the search engine to find all methods with the desired signature and then performs a union find in order to determine if they are "married" to the original method. In order to perform the union find it has to invoke the hierarchy building for every search result. Lets call this Method 1. With this patch I have added the option of finding all declarations in a different way. By building the hierarchy of the original method and traversing it up and down in order to find any married methods. This then does not need to perform the subsequent union find. Lets call this Method 2. To test if the new option is implemented correctly I ran all the unit tests with it enabled. That is how I discovered bug 420055. Clearly Method 1 will perform better than method 2 if there are very few declarations that match the initial search and the source method lives is in a very extensive hierarchy. Method 2 will perform better if the target declaration is in a very simple hierarchy. With this patch I have decided to use a heuristic approach based on the number of search results that come back from the search engine. If the number of declarations is more than 20 It uses Method 2 otherwise the implementation remains unchanged. This will not break any unit tests because the new code will only be executed in one test case that is also added as part of this patch. I removed the fDeclarations field. The contents of this field get put into fTypeToMethod so that should make a slight difference in a few select corner cases. Also I changed the RippleMethodFinderTests to extend RefactoringTest and added it to AllRefactoringTests. This solved the problem of unit tests interfering with each other (not sure why AbstractCUTestCase tests interfere with each other, perhaps there is a bug somewhere). Also in this patch are a couple of commented out test cases for bug 420055. I look forward to hearing feedback.
I have thought about my code and I have realized there is a flaw in it. I will upload a new patch when I have figured out the correct solution.
Comment on attachment 236802 [details] proposed patch Nikolay, please take your time for coming up with a correct solution. The focus is now on Java 8 work, hence further feedback/review would also take some time.
Hello Noopur, I was hoping that my patch for bug 99622 be committed first before I carry on with this bug. That patch changes RippleMethodFinder2 significantly enough to make it a necessary prerequisite.
*** Bug 517109 has been marked as a duplicate of this bug. ***
The problem is still there in 4.6.3. Nikolay, do you still plan to work on the patch? If not, we should re-assign this bug back to the JDT team.
After 4 years of waiting for someone at eclipse to take notice of my contributions I finally gave up and have to conclude that the project is under resourced and effectively dead. I've moved to Idea after 15 years of championing eclipse.
New Gerrit change created: https://git.eclipse.org/r/116813
OK, I browsed through the code and Nikolay's ideas. They sound good to me. While profiling the code, I noticed some sub-optimal use of collections, for which I created a small patch. This helps a bit, but nothing too drastic. Once we merge this, I'll go with Nikolay's idea to remove the system binaries from the method search (Comment 11). This gives a massive boost and doesn't seem to be too dangerous. I imagine 99.9% of the user base will never run into a problem with this change, to be on the safe side I guess we can add a preference. If the above changes go well, I'll profile again. I'll determine whether we must go with Nikolay's idea to try a different strategy if the current one finds too many hits before building hierarchies (Comment 17). This is a bit heavy-weight as far as changes go, and I'll need some extra time to put the idea into code (I don't see the patches anymore).
(In reply to Simeon Andreev from comment #25) > Once we merge this, I'll go with Nikolay's idea to remove the system > binaries from the method search (Comment 11). With that, will you still detect when a user tries to rename a method that overrides a method from the JRE? > This gives a massive boost and > doesn't seem to be too dangerous. I imagine 99.9% of the user base will > never run into a problem with this change, to be on the safe side I guess we > can add a preference. A user would first need to know that they are part of the "0.1%", in order to toggle that preference, but when checks are disabled they may never notice. Doesn't sound like any added safety to me ;p
I must confess I haven't seen the code in question yet. Simeon, can you briefly describe the current implementation and why it searches through the binaries? I guess only to detect if the target method name has a clash with some super interface? Now let assume we skip the comprehensive search all together, do a quick change and *after* the change check if we have a collision - wouldn't this be faster for 99.9% of users? And for the case of a clash we do a quick rollback? Also I imagine we can pick up the low hanging fruit and don't do a search if the current type is not implementing/extending anything?
(In reply to Stephan Herrmann from comment #26) > (In reply to Simeon Andreev from comment #25) > > Once we merge this, I'll go with Nikolay's idea to remove the system > > binaries from the method search (Comment 11). > > With that, will you still detect when a user tries to rename a method that > overrides a method from the JRE? Trying to rename overriden system library methods still yields an error message, due to a check in RenameMethodProcessor.doCheckFinalConditions which delegates to Checks.isAvailable. This in turn will check if the method is read only, yielding an error. I tried this e.g. with overriding a method from FileInputStream. > > This gives a massive boost and > > doesn't seem to be too dangerous. I imagine 99.9% of the user base will > > never run into a problem with this change, to be on the safe side I guess we > > can add a preference. > > A user would first need to know that they are part of the "0.1%", in order > to toggle that preference, but when checks are disabled they may never > notice. Doesn't sound like any added safety to me ;p To be honest, I have trouble imagining the error case here. As mentioned, inheriting from a JRE class and renaming a method fails. I think a developer would need to work in a project that contains JRE sources, e.g. java.util.List, and rename a method. If, supposedly, this goes through, then all the binaries which depend on the previous definition become invalid. I can only imagine this to be a problem for a JRE developer, if they use Eclipse, and they rename internal methods.
(In reply to Andrey Loskutov from comment #27) > I must confess I haven't seen the code in question yet. Simeon, can you > briefly describe the current implementation and why it searches through the > binaries? I guess only to detect if the target method name has a clash with > some super interface? Please check Comment 17 from Nikolay Metchev, Nikolay explained what the code does: > At the moment RippleMethodFinder2 in order to find all declarations it first > uses the search engine to find all methods with the desired signature and > then performs a union find in order to determine if they are "married" to > the original method. In order to perform the union find it has to invoke the > hierarchy building for every search result. Please check also Comment 5 and Comment 6 from Markus Keller, those explain why all occurrences are listed (and necessary). Though personally I don't agree with the statement in Comment 6, that also Interface J should be changed. Clearly the method which is being renamed is I.getXXX. If the developer got themselves in the situation where they implement two different interfaces with the same method, from my perspective they already lost. > Now let assume we skip the comprehensive search all together, do a quick > change and *after* the change check if we have a collision - wouldn't this > be faster for 99.9% of users? And for the case of a clash we do a quick > rollback? Theoretically the code already does this. It checks that the source can still compile. I need to check whether it will check this *for all changed sources*. If so this sounds relatively safe. An example where this wouldn't working on the Eclipse platform, and changing e.g. jdt.core, without having e.g. jdt.ui open. If renaming API in this situation, its possible to break jdt.ui sources which are not open, since those will not be renamed. Those will not be validated for compile errors. Maybe such cases are easier to check than the current code. In that case it should be possible to check explicitly for this case, and rely on the does-it-compile-after-rename-check for the rest. > Also I imagine we can pick up the low hanging fruit and don't do a search if > the current type is not implementing/extending anything? Yes, that can definitely be done. It would complicate the code a bit more though, and I'm not sure how much we gain. I think we should aim to improve the code for all cases, and if we reach a brick-wall, we can resort to this option.
Looks like return types of methods are ignored, this may explain why so many getName hits are seen. I don't see a way around this though, e.g. due to interface I { Number get(); } interface J extends I { Double get(); } IMethod doesn't allow for easy checking if its return type has a specific super type? IMethod.getReturnType only returns a string...
New Gerrit change created: https://git.eclipse.org/r/116888
I think Andrey's idea is quite simple to extend to more common cases, see https://git.eclipse.org/r/#/c/116888. Today I "foolishly" refactor renamed a dispose() method in a class which didn't derive from *anything*. The operation takes 90s for our code base. With the change from the commit, the operation takes 10s. The patch is very ham handed, it will compute a hierarchy of all supposedly unrelated methods and then cut it with the hierarchy of the method under rename. With some set operations / a bit more union finding, I believe its possible to improve on this, so that a bit more complex cases also benefit.
Unfortunately I have little to no time to invest here, I'll try to find some in the last two weeks of March. Hopefully enough to propagate the current change, which already speeds up simple cases a lot.
Created attachment 273370 [details] Cursory performance measurements for change 116888, showing about 4x improvement for simple cases.
Dear JDT maintainers, I've prepared a patch which takes simple renaming cases into account, reducing rename time to 1/4 of its original time (for said simple cases). This is likely patch 1 out of 2 patches to fix this defect. The next step would be to fully implement "Method 2" suggested by Nikolay Metchev in Comment 17 (so far the improvement heavily depends on this method, but does not fully implement it). If you find time to review and hopefully merge, please do so. Best regards and thanks, Simeon
Gerrit change https://git.eclipse.org/r/116888 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=a6bb576e5e165d7ca3352666330ceb194bf723c5
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
(In reply to Eclipse Genie from comment #38) > This bug hasn't had any activity in quite some time. Maybe the problem got > resolved As someone who just tried to rename method "get", the bug is definitely here in 2023-12. Moreover the process cannot be cancelled so killing eclipse is the only way to recover. It's pretty obvious that only the types connected by hierarchy to the current class needs to be considered, but I appreciate it might not be trivial to use this information...
