Community
Participate
Working Groups
Build Identifier: I20100603-1500 I've tested eclipse 3.6 with one of my project. eclipse 3.5 do a full refresh of the project in few seconds, eclipse 3.6 do a full refresh of the same workspace in several minutes, making eclipse 3.6 is useless. Reproducible: Always Steps to Reproduce: 1. checkout the sources: svn checkout http://phpreboot.googlecode.com/svn/trunk/ phpreboot-read-only 2. in eclipse, Project > Clean ...
Jay, please investigate.
Rémi, Are you using default settings ? Anything special about your project ? APT ?
(In reply to comment #2) > Rémi, > > Are you using default settings ? I use the default settings. Also I've forgotten to say that you have to generate some files (some AST nodes in fact) before compile it in eclipse. To generate thoses file just type: ant jar > Anything special about your project ? > APT ? no APT but it's a runtime interpreter/compiler so it uses parameterized generics visitors. Eclipse is especially slow when updating one of those visitors, it seems it generates a lot a garbage because after waiting for eclipse compiler completion, I have to wait the completion of a full GC :( Rémi
And it also requires a fairly recent JDK7 build (b94+) Rémi
Initial characterization work by Jay & Satyam (Thanks!) pointed to the method verifier and I have further tracked it down to the fix made for bug# 293615. The performance degradation gets introduced by the change made to org.eclipse.jdt.internal.compiler.lookup.MethodVerifier15.detectNameClash(MethodBinding, MethodBinding, boolean) and by rolling back *just* this change I can see that performance problem goes away. In terms of characterization: this problem comes up because there are ~258 methods of the same name (visit) in the class VisitorCopier.java and its base class Visitor.java. Looking for a name clash the way it is done results in a O(n^2) algorithms with a high n. So while this problem is not common enough, it is also not a corner case. Investigating how to speed up the method ...
Created attachment 171390 [details] Patch under consideration Jay & Satyam: This brings the performance back to what it was at 3.5.2 time. Please verify that this patch fixes the performance problem in your setup too. Rémi, are you in a position to apply this source patch and validate the fix ? TIA. This patch is still under test. Basically, bug # 293615 was about eclipse erroneously emitting a name clash error in some scenarios where it should not. As bug# 293615 comment#9 described - if type X has correctly overridden a super type Y's method foo, then we should not look for a clash between any other foo X may have and foo of Y that was overridden. In this scenario Y's foo is completely replaced and is out of the picture. So the fix for 293615 introduced some checks to see if Y' foo was indeed overridden by some other foo of X. It is this check that is proving to be expensive in this case since there are two many foo's (~258 methods with the same name). (It is also worth noting that this check got introduced deep down in a call stack, the outer layers of which have their own loops) The current patch fixes the problem in a very simple way by deferring these checks that help avoid an incorrect error report to be carried out only if that incorrect error is indeed going to be reported. If in the course of execution, we are not going to report this incorrect error at all (due to other conditions/requirements not being met,) then we don't need to pay the penalty of additional checks.
(In reply to comment #6) > Created an attachment (id=171390) [details] > Patch under consideration > > Jay & Satyam: This brings the performance back to what it was > at 3.5.2 time. Please verify that this patch fixes the > performance problem in your setup too. > Yes, with this patch, performance for building this project does look as good as 3.5.2.
(In reply to comment #6) > Created an attachment (id=171390) [details] > Patch under consideration I have deliberately kept this patch simple so it is immediately obvious and explicit what is changing. The only method undergoing change is org.eclipse.jdt.internal.compiler.lookup.MethodVerifier15.detectNameClash(MethodBinding, MethodBinding, boolean) Before the fix for bug# 293615 this method used to look like: (from version 1.106 of MethodVerifier15.java) boolean detectNameClash(MethodBinding current, MethodBinding inherited) { MethodBinding original = inherited.original(); // can be the same as inherited if (!current.areParameterErasuresEqual(original)) return false; problemReporter(current).methodNameClash(current, inherited.declaringClass.isRawType() ? inherited : original); return true; } and with the current patch it looks like: boolean detectNameClash(MethodBinding current, MethodBinding inherited, boolean treatAsSynthetic) { MethodBinding methodToCheck = inherited; MethodBinding original = methodToCheck.original(); // can be the same as inherited if (!current.areParameterErasuresEqual(original)) return false; if (!treatAsSynthetic) { // For a user method, see if current class overrides the inherited method. If it does, // then any grievance we may have ought to be against the current class's method and // NOT against any super implementations. https://bugs.eclipse.org/bugs/show_bug.cgi?id=293615 // https://bugs.eclipse.org/bugs/show_bug.cgi?id=315978 : we now defer this rather expensive // check to just before reporting (the incorrect) name clash. In the event there is no name // clash to report to begin with (the common case), no penalty needs to be paid. MethodBinding[] currentNamesakes = (MethodBinding[]) this.currentMethods.get(inherited.selector); if (currentNamesakes.length > 1) { // we know it ought to at least one and that current is NOT the override for (int i = 0, length = currentNamesakes.length; i < length; i++) { MethodBinding currentMethod = currentNamesakes[i]; if (currentMethod != current && doesMethodOverride(currentMethod, inherited)) { methodToCheck = currentMethod; break; } } } } original = methodToCheck.original(); // can be the same as inherited if (!current.areParameterErasuresEqual(original)) return false; original = inherited.original(); // For error reporting use, inherited.original() problemReporter(current).methodNameClash(current, inherited.declaringClass.isRawType() ? inherited : original); return true; } essentially pushing all the incorrect message avoidance code to just before the problem report in the 1.106 version.
All JDT Core tests pass. Olivier, please review.
Patch looks good.
Rémi, Thanks for the bug report, Unfortunately, it is a little late for 3.6, so is being targetted for 3.6.1. So the options are: - Wait for 3.6.1, - Wait for 3.6 maintenance stream builds to kick in and grab an early build as it becomes available. - If your need is more urgent and blocking, we can explore a feature patch specifically to address the need. Let us know what would work -- Thanks.
(In reply to comment #11) > Rémi, Thanks for the bug report, Unfortunately, it is a little late for 3.6, > so is being targetted for 3.6.1. > > So the options are: > > - Wait for 3.6.1, > - Wait for 3.6 maintenance stream builds to kick in and grab an early > build as it becomes available. > - If your need is more urgent and blocking, we can explore a feature > patch specifically to address the need. > > Let us know what would work -- Thanks. Thank you, for the quick fix. But none of the options above are satisfactory for me. The code of the visitors is generated by Tatoo which is a parser generator I've developed with two colleages. It is used as a teaching tool and by some companies and labs (mostly french ones) for different projects. This parser generator is not used a lot but have more or less fifty regular users plus all the students of 3 differents CS courses. I would prefer the patch integrated in 3.6 instead of having to send email explaining why they can't use eclipse 3.6 on the code generated by Tatoo and have to wait eclipse 3.6.1. And changing the way the visitor are generated is not an option because It will break the backward compatibility. Rémi
The test case has two classes VisitorCopier (subclass) and Visitor (superclass) each with 258 methods (essentially) of the name visit with differing parameters presumably distributed over the different Node types of the problem domain. In order to get a picture of how the computational times grows vs number of overloaded methods I sliced the input program into different sizes and measured the time: Number of overloaded methods build time in the two classes (seconds) <= 75 1 = 100 6 = 125 18 = 150 34 = 175 53 = 200 89 = 225 145 = 258 (original) 247 While any degradation is bad, the real pinch is after the 150 mark, getting progressively worse after that.
> The code of the visitors is generated by Tatoo which is a parser generator > I've developed with two colleages. > It is used as a teaching tool and > by some companies and labs (mostly french ones) for different projects. > This parser generator is not used a lot but have more or less > fifty regular users plus all the students of 3 differents CS courses. Rémi, what triggers the generation of these classes ? I see a prominent comment in these files saying these are generated and not meant to be edited. So that would rule out the performance problem rearing its head in reconcile and incremental build scenarios. How often do these files change ?
Patch looks good
(In reply to comment #14) > > The code of the visitors is generated by Tatoo which is a parser generator > > I've developed with two colleages. > > It is used as a teaching tool and > > by some companies and labs (mostly french ones) for different projects. > > This parser generator is not used a lot but have more or less > > fifty regular users plus all the students of 3 differents CS courses. > > Rémi, what triggers the generation of these classes ? The user run the ant script (the task named enbf in the build.xml) > I see a prominent > comment in these files saying these are generated and not meant to be > edited. So that would rule out the performance problem rearing its head > in reconcile and incremental build scenarios. How often do these files > change ? It changes each time a user change the grammar and re-generate the AST and the visitors. But these visitors are also subclasses in order to code semantics passes over the AST (see Evaluator, Typecheck or Gen) and each time one of these classes are updated it trigger the bug. Rémi
Patch looks good to me.
(In reply to comment #12) > I would prefer the patch integrated in 3.6 instead of > having to send email explaining why they can't use eclipse 3.6 > on the code generated by Tatoo and have to wait eclipse 3.6.1. As explained, we cannot do that now. This is too late for 3.6. The platform has to be done now to let the Helios release train to build on top of it. We will provide a feature patch on the jdt/core update site to let you easily install the patch that will fix it. This is the best we can do so close to a release. The regression was introduced in 3.6M4. It is just unfortunate that we didn't get a report for it before 3.6RC4.
(In reply to comment #18) > (In reply to comment #12) > > I would prefer the patch integrated in 3.6 instead of > > having to send email explaining why they can't use eclipse 3.6 > > on the code generated by Tatoo and have to wait eclipse 3.6.1. > As explained, we cannot do that now. This is too late for 3.6. The platform has > to be done now to let the Helios release train to build on top of it. > We will provide a feature patch on the jdt/core update site to let you easily > install the patch that will fix it. > This is the best we can do so close to a release. The regression was introduced > in 3.6M4. It is just unfortunate that we didn't get a report for it before > 3.6RC4. Okay, I understand. I suppose that bugs are part of our life :) Rémi
Could you please submit a test case that we could use as a regression test in the performance test suite? We need that test case to be clean from the IP side. Thanks.
(In reply to comment #20) > We need that test case to be clean from the IP side. I meant that we need to get the test case using Bugzilla attachment to make sure this is EPL compliant. Thanks.
(In reply to comment #21) > (In reply to comment #20) > > We need that test case to be clean from the IP side. > I meant that we need to get the test case using Bugzilla attachment to make > sure this is EPL compliant. Rémi, The two files Visitor.java and VisitorCopier.java were sufficient to reproduce the problem. If you can base a test case on these and submit it as an attachment that would be swell - Thanks.
Created attachment 172108 [details] test-case that shows the bug
(In reply to comment #23) > Created an attachment (id=172108) [details] > test-case that shows the bug Thanks Rémi. Satyam, Can you please add a test based on this to the performance harness ? Thanks ! The following code generates the test case in a form suitable to add to the test suite: public class Clazz { public static void main(String[] args) { System.out.println("\"public class EclipseVisitorBug {\\n\" +" ); for (int i = 0; i < 500; i++) { System.out.println("\" public static class A" + i + "{}\\n\" +"); } System.out.println(""); System.out.println("\" static class Visitor<R> {\\n\" +"); for (int i = 0; i < 500; i++) { System.out.println("\" public R visit(A" + i + " a) { return null;}\\n\" +"); } System.out.println(""); System.out.println("\" static class AnotherVisitor extends Visitor {\\n\" +"); for (int i = 0; i < 500; i++) { System.out.println("\" public R visit(A" + i + " a) { return null; }\\n\" +"); } System.out.println("\"}\\n\""); } }
Created attachment 172481 [details] Patch for the Performance test Frederic, Could you please look at this performance test and let me know your comments?
Created attachment 172482 [details] Patch for the Performance test The last patch didn't have the new file that was added. This file is compiled by the test added. Updated the copyright year also.
Patch with the source fix (from comment#6) released in HEAD for 3.7 M1 in 3.6 maintenance stream for 3.6.1. Regression test will be released separately by Satyam shortly (Thanks!)
> Regression test will be released separately by Satyam shortly (Thanks!) bug# 317771 has been raised with 3.6.1 as target for this task. Satyam, please follow up there - Thanks.
(In reply to comment #12) > I would prefer the patch integrated in 3.6 instead of > having to send email explaining why they can't use eclipse 3.6 > on the code generated by Tatoo and have to wait eclipse 3.6.1. Rémi, Can you grab the maintenance build in order to get the fix or you want a feature patch? I know you wanted the fix for 3.6.0, but this could not be done. The maintenance builds are running every week. The first one after 3.6.0 contains the fix. Let me know if it is ok for you to use one of these maintenance builds.
(In reply to comment #29) > (In reply to comment #12) > > I would prefer the patch integrated in 3.6 instead of > > having to send email explaining why they can't use eclipse 3.6 > > on the code generated by Tatoo and have to wait eclipse 3.6.1. > Rémi, > > Can you grab the maintenance build in order to get the fix or you want a > feature patch? > I know you wanted the fix for 3.6.0, but this could not be done. The > maintenance builds are running every week. The first one after 3.6.0 contains > the fix. Let me know if it is ok for you to use one of these maintenance > builds. Hi Olivier, I've tested with I20100706-0800 and it's okay. The need for a patch is less important because I've explained to my users why they can not use eclipse 3.6. Most of them answer by saying that they will wait 3.6.1. Rémi
Verified for 3.7M1 using build I20100802-1800.
Change status to RESOLVED as this needs to be reverified for 3.6.1.
Verified for 3.6.1 using build M20100825-0800.
