Community
Participate
Working Groups
This bug is a follow-on to bug 262791, which discussed slow compilation times. Customers complain of running out of memory when compiling large projects with APT enabled. The memory requirement is almost certainly because APT requests, and keeps references to, the ASTs of every type being compiled. This is necessary because processors can request information from any type, including in particular types that cannot be generated due to compilation errors. The JDT makes it so that bindings obtained in different requests are not comparable, so we have to make one big request, do all the processing, and then finish. It is not at all clear how to get around this while maintaining correctness. Possibly we could define a stripped-down version of the AST that contained only enough information for APT to act on; or, maybe we can find a way to reduce the number of ASTs we hang onto. This bug will serve as an umbrella for this investigation.
Ok, I reproduced the problem successfully. Steps I followed: 1) Start Eclipse (I used latest I-build I20090922-0800). 2) Check out org.eclipse.jdt.apt.tests from CVS 3) Export the bundle on your hard drive without generating a jar file. 4) Grab the two files apt.jar and aptext.jar that resides in the exported bundle folder. 5) In a new workspace, disable auto-build and import the project attached above (see attachment to this bug report). 6) Select the project properties 7) Java Compiler>Annotation Processing> Enable project specific option In Java Compiler>Annotation Processing>Factory Path add the two jars in steps 4 as external jars. 8) Force a full build. I got a OOE even if I set up the max memory to -Xmx1024M. I'll continue to investigate. The attached project contains 115 units and 22 of them are bigger than 1M. I am investigating if we can "fix" the issue by removing extra information we don't need from memory.
Has there been any progress on this issue? I'm following up on what was reported by Srinivas Surapaneni in bug 262791. I can lend a hand in the effort but I'd like to know if there is any work that has already been completed or a direction you are heading.
I am back on this issue. I am investigating what can be done without drastically change the way APT works right now. If this doesn't work, I have other ideas, but they are more fundamental changes in the APT architecture. This being said, any help on this topic is welcome. So feel free to contact me if you want to be involved.
Hi Olivier Is there any time frame for the fix/workaround? With our current project, there is no workaround for us. The only option we have is to use the ant builds Thanks Srinivas Surapaneni
I am hoping to have something for M4 if I can find out why a worker is holding to lots of objects I would expect to be gc'ed. I'll spend more time next week on this issue.
Satyam, please investigate. I'll send you the test case I am using shortly.
Created attachment 152025 [details] Proposed patch for 3.5.1 This patch is on top of 3.5.1 and has changes both on APT and JDT/Core. The method statements are probably unnecessary in the ASTs requested by APT. This patch ignores parsing of the method statements depending on the flag that is set by APT. With this patch, this large project works fine even with 512MB heap space. It fails with 256MB though!
Hi Satyam Is this patch going to be in the nightly builds? If it is not, can you provide me instructions on how to apply the patch on the eclipse 3.5.1 and I can test it. Thanks Srinivas
(In reply to comment #8) Srinivas, I have just submitted the patch for the review and it won't go immediately in the nightly build. I will try to give you a drop on top of 3.5.1.
A patch is required for 3.4 maintenance as well.
(In reply to comment #7) > Created an attachment (id=152025) [details] > Proposed patch for 3.5.1 > > This patch is on top of 3.5.1 and has changes both on APT and JDT/Core. Nice work, Satyam! The patch looks good to me. One small question: I find the term "method statements" a little confusing because it is not clear to me whether it refers to the method declaration (which we do need) or the method body (which we don't), and inconsistent with some other usages in the code. Would "method bodies" be more clear? E.g., "IGNORE_METHOD_BODIES", "setIgnoreMethodBodies()", and so forth.
I think this is a good start. But I also believe that the pruning is done too early. If the statements (method bodies) are removed before the method is resolved (when bindings are required), you can end up with false problems being reported on the converted units because method that returns a non-void value won't have any statements and therefore the compiler will issue a problem for each of them about a missing return statement. I think we should do the pruning while converting the compiler ast to a DOM ast. This might require to do additional pruning post conversion to remove the compiler ast statements once the unit has been processed.
Satyam, several things to look at for a new patch: 1) if we prune the method bodies in the compiler ast, we should not resolve the method bodies, but we should still resolve the method headers (get a method binding for the corresponding method declaration) 2) in case the original unit contains problem inside a method body, should we also report such problem on the converted unit if the method bodies are ignored ? 3) we must support the new API on ASTParser for all usages: reconciling, createAST(..) with and without binding resolution. For (2), I don't know what the best answer is. I would say we might just document the new option to say that this might "skip" existing problems inside method bodies. This would be somehow consistent as ignoring method bodies should also ignore problem inside methods. (In reply to comment #11) > Would "method bodies" be more clear? E.g., "IGNORE_METHOD_BODIES", > "setIgnoreMethodBodies()", and so forth. I also prefer IGNORE_METHOD_BODIES and setIgnoreMethodBodies(..) names.
Of course, we should add regression tests for each type of usage of DOM/AST to make sure that the new API is working as expected (tests should cover reconcile operations, createASTs(..) and createAST(..) call).
Created attachment 152566 [details] Proposed patch for 3.5 maintenance stream New patch taking the comments from Olivier and Walter. Here are the changes done in this patch! 1. Renamed to IGNORE_METHOD_BODIES 2. Stopped analyze code and generation code when this flag is set 3. Made this api available even in reconcile and many other paths 4. The statements are parsed and looked for any anonymous classes and if any of them are found in a method, that particular method statements are not discarded. Even in this case, analyze code and generation code will not happen 5. Added some tests 6. Two apt tests have been failing because the generation code is stopped. I don't think the generation is needed and hence modified the tests.. Let me know if this is a problem. Olivier, If this patch is good I can create a patch for 3.4 too.
I don't understand the change in: src/org/eclipse/jdt/apt/tests/TestUtil.java
(In reply to comment #16) > I don't understand the change in: > src/org/eclipse/jdt/apt/tests/TestUtil.java OOps.. that change wasn't supposed to be there... It should have got in accidentally. All the other changes should be good!
I am fine with those APT test changes (except of course the TestUtil change). Another option is that I think there is an "expectingUniqueCompiledClasses" or something like that, that ignores duplicates. The change to jdt.core MANIFEST.MF looks wrong. Does this patch address Java 6 annotation processors (that is, the processing done in org.eclipse.jdt.compiler.apt)? It doesn't look like it - at least, I don't see how ignoreMethodBodies would get set on the parser in that case.
Created attachment 152688 [details] Proposed patch for 3.5 maintenance stream TestUtil and manifest files are removed from this patch!
(In reply to comment #18) > I am fine with those APT test changes (except of course the TestUtil change). > Another option is that I think there is an "expectingUniqueCompiledClasses" or > something like that, that ignores duplicates. > > The change to jdt.core MANIFEST.MF looks wrong. > > Does this patch address Java 6 annotation processors (that is, the processing > done in org.eclipse.jdt.compiler.apt)? It doesn't look like it - at least, I > don't see how ignoreMethodBodies would get set on the parser in that case. Walter, Is the memory for Java 6 processors as bad as for Java 5 processors?
> Walter, Is the memory for Java 6 processors as bad as for Java 5 processors? I think if we have a way to improve things for now, we should use it for all cases (Java 5 and Java 6 processors). I'll review your last patch today.
I tested the patch with the plugins given by Satyam I was able to compile our project with 1300M memory. Before it was not even compiling for 1400m to 1500mb I tried to still lessen the memory, but getting errors in the initializing java tooling Thanks Satyam for providing the plugins for testing
I don't see why we need to have a method deleteMethodBodiesWithoutLocalTypes(..) in TypeDeclaration that is nullifying the statements for each method without local types when we could just not set the statements in the parser itself. Am I missing something ? I'll propose a new patch later today. Sorry I missed that case looking at the patch yesterday.
>I tried to still lessen the memory, but getting errors in the initializing java >tooling What "errors"? OOME?
Patch looks good except for a few cases. I'll propose a new patch.
Created attachment 152956 [details] Proposed fix + regression tests
Satyam, Please take a look. If ok, please adapt the patch for 3.5.2 and 3.4.x. The new API should remain tagged as 3.6. In 3.5.2, we will need a PMC approval for adding a new API. I don't like to tag it as @since 3.5. Same applies for 3.4. Daniel, how do you want to work around the addition of a new API?
I am getting the following error with 1024m memory in the java intialization !ENTRY org.eclipse.core.jobs 4 2 2009-11-19 06:41:54.789 !MESSAGE An internal error occurred during: "Initializing Java Tooling". !STACK 0 java.lang.ArrayIndexOutOfBoundsException: 2 at org.eclipse.emf.common.util.BasicEList.assign(BasicEList.java:124) at org.eclipse.emf.common.util.BasicEList.addUnique(BasicEList.java:424) at org.eclipse.emf.common.util.AbstractEList.add(AbstractEList.java:307) at org.eclipse.emf.common.notify.impl.BasicNotifierImpl$EAdapterList.add(BasicNotifierImpl.java:142) at org.eclipse.wst.common.componentcore.internal.StructureEdit.getEclipseResource(StructureEdit.java:251) at org.eclipse.wst.common.componentcore.internal.impl.ResourceTreeNode.findMatchingVirtualPathsSet(ResourceTreeNode.java:195) at org.eclipse.wst.common.componentcore.internal.impl.ResourceTreeNode.findModuleResourcesSet(ResourceTreeNode.java:178) at org.eclipse.wst.common.componentcore.internal.impl.ResourceTreeNode.findModuleResources(ResourceTreeNode.java:143) at org.eclipse.wst.common.componentcore.internal.resources.VirtualResource.getProjectRelativePaths(VirtualResource.java:132) at org.eclipse.wst.common.componentcore.internal.resources.VirtualFile.getUnderlyingFiles(VirtualFile.java:104) at org.eclipse.wst.common.componentcore.internal.resources.VirtualFile.getUnderlyingResources(VirtualFile.java:93) at org.eclipse.wst.common.componentcore.internal.resources.VirtualResource.exists(VirtualResource.java:88) at org.eclipse.jst.j2ee.componentcore.J2EEModuleVirtualComponent.getManifestClasspath(J2EEModuleVirtualComponent.java:146) at org.eclipse.jst.j2ee.componentcore.J2EEModuleVirtualComponent.getManifestReferences(J2EEModuleVirtualComponent.java:291) at org.eclipse.jst.j2ee.componentcore.J2EEModuleVirtualComponent.getReferences(J2EEModuleVirtualComponent.java:103) at org.eclipse.jst.j2ee.internal.common.classpath.J2EEComponentClasspathContainer.update(J2EEComponentClasspathContainer.java:200) at org.eclipse.jst.j2ee.internal.common.classpath.J2EEComponentClasspathContainer.install(J2EEComponentClasspathContainer.java:351) at org.eclipse.jst.j2ee.internal.common.classpath.J2EEComponentClasspathInitializer.initialize(J2EEComponentClasspathInitializer.java:29) at org.eclipse.jdt.internal.core.JavaModelManager.initializeContainer(JavaModelManager.java:2642) at org.eclipse.jdt.internal.core.JavaModelManager$11.run(JavaModelManager.java:2548) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:1800) at org.eclipse.jdt.internal.core.JavaModelManager.initializeAllContainers(JavaModelManager.java:2588) at org.eclipse.jdt.internal.core.JavaModelManager.getClasspathContainer(JavaModelManager.java:1808) at org.eclipse.jdt.core.JavaCore.initializeAfterLoad(JavaCore.java:3410) at org.eclipse.jdt.internal.ui.InitializeAfterLoadJob$RealJob.run(InitializeAfterLoadJob.java:35) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:55)
This looks like a completely different issue. Please reopen it against the EMF component.
Satyam, I am going to release the last patch in HEAD and only make changes around the @since tags for the new API to point to 3.5.2 instead of 3.6 (I changed that in the last patch). Please prepare a patch for 3.5.2 and 3.4.x. In both, you set the new API @since tag to 3.5.2. In 3.4.x this is less a problem since there is no more 3.4.x official builds. Attach both patches to this bug report. Daniel, requesting PMC approval for 3.4.x and 3.5.2 backport + new api addition.
Created attachment 152989 [details] Proposed fix + regression tests Patch released for HEAD with regression tests.
Released for 3.6M4. Regression tests added in: org.eclipse.jdt.core.tests.dom.ASTConverterTestAST3_2#test0715 org.eclipse.jdt.core.tests.dom.ASTConverterTestAST3_2#test0716 org.eclipse.jdt.core.tests.dom.ASTConverterTestAST3_2#test0717 org.eclipse.jdt.core.tests.dom.BatchASTCreationTests#testIgnoreMethodBodies1 org.eclipse.jdt.core.tests.dom.BatchASTCreationTests#testIgnoreMethodBodies2 org.eclipse.jdt.core.tests.model.ReconcilerTests#testIgnoreMethodBodies1 org.eclipse.jdt.core.tests.model.ReconcilerTests#testIgnoreMethodBodies2 I keep it open till it is released for 3.5.2.
Walter, could you please tag apt projects for next I-build?
Created attachment 153054 [details] Patch on 3.4 maintenance branch Pulled in the changes on to 3.4 maintenance branch There is one change that is different here. parseClassBodyDeclarations() of parser.java was not doing a diet parse but doing a complete parse. Hence, I made changes in consumeConstructorDeclaration() and consumeMethodDeclaration(). I tried to keep my changes small. Olivier, Please review.
Created attachment 153055 [details] Patch on 3.5 maintenance branch Olivier, The main change on top of your patch is changing the @since version to 3.5.2
In general I'm leaning towards +1 but I'd first like to see another patch that addresses the following issues: I'm not very happy with the new ASTParser.ignoreMethodBodies(): - most other methods in the ASTParser allow me to (re-)set new parameters and run the parser again but once I've called ignoreMethodBodies() I'm locked in. Please replace this method with setIgnoreMethodBodies(boolean). - the Javadoc mentions internal terms like code analysis and generation but it misses to explain what exactly the option does, e.g. what exact implication it has on the bindings. This needs to be cleaned up. Walter, Olivier: I see that we now call ASTParser.ignoreMethodBodies() in BaseProcessorEnv.createASTs(...). My question: could we also do this at the other locations in BaseProcessorEnv where we create a new parser? Olivier, I don't know the code too well. Can you explain quickly we check/use (md.bits & ASTNode.HasLocalType) in Parser? The 3.4 and 3.5 patches cause API tooling errors in my workspace and hence no go for those. Please add exclusion rules where appropriate and update the /org.eclipse.releng/apiexclude/exclude_list.txt accordingly. Some other minor stuff I found: CompilerOptions.ignoreMethodBodies has no Javadoc like all other options have. Copyright date not updated in BaseProcessorEnv I saw comments like "// don't set ignoreMethodBodies" inserted at some places. I'm not very familiar with the code so the comment might be self explaining for JDT Core developers but myself would have liked to have a little explanation why it's OK not to set ignoreMethodBodies. At some places I saw: analyzeCode = false; generateCode = false; I think it would be more readable to only have one local variable because they always have the same value and the comment behind each argument already explains the semantics. The two parse methods in CompilationUnitResolver add additional brackets. In patches for the maintenance branches we should avoid any changes that aren't absolutely required for the fix.
(In reply to comment #36) > I'm not very happy with the new ASTParser.ignoreMethodBodies(): > - most other methods in the ASTParser allow me to (re-)set new parameters and > run the parser again but once I've called ignoreMethodBodies() I'm locked in. > Please replace this method with setIgnoreMethodBodies(boolean). I'll do that in HEAD. > - the Javadoc mentions internal terms like code analysis and generation but it > misses to explain what exactly the option does, e.g. what exact implication it > has on the bindings. This needs to be cleaned up. I'll try to clarify this. > Walter, Olivier: I see that we now call ASTParser.ignoreMethodBodies() in > BaseProcessorEnv.createASTs(...). My question: could we also do this at the > other locations in BaseProcessorEnv where we create a new parser? This was done on purpose as to minimize the changes in APT. I can probably use the new method at other locations as well. > Olivier, I don't know the code too well. Can you explain quickly we check/use > (md.bits & ASTNode.HasLocalType) in Parser? We check that to make sure we preserve all local types. > The 3.4 and 3.5 patches cause API tooling errors in my workspace and hence no > go for those. Please add exclusion rules where appropriate and update the > /org.eclipse.releng/apiexclude/exclude_list.txt accordingly. I update the file once there is a build that shows the new addition. This makes sure that the addition is properly detected. > Some other minor stuff I found: > > CompilerOptions.ignoreMethodBodies has no Javadoc like all other options have. > > Copyright date not updated in BaseProcessorEnv > > I saw comments like "// don't set ignoreMethodBodies" inserted at some places. > I'm not very familiar with the code so the comment might be self explaining for > JDT Core developers but myself would have liked to have a little explanation > why it's OK not to set ignoreMethodBodies. Ok, I'll do that. > At some places I saw: > analyzeCode = false; > generateCode = false; > I think it would be more readable to only have one local variable because they > always have the same value and the comment behind each argument already > explains the semantics. I'll take care of this.
Satyam, please update both patches (3.5.x and 3.4.x) based on Daniel's comment.
>> Walter, Olivier: I see that we now call ASTParser.ignoreMethodBodies() in >> BaseProcessorEnv.createASTs(...). My question: could we also do this at the >> other locations in BaseProcessorEnv where we create a new parser? >This was done on purpose as to minimize the changes in APT. I can probably use >the new method at other locations as well. The question is whether that would buy us an even better performance. Walter, could you take a look at that? Thank!
(In reply to comment #39) > The question is whether that would buy us an even better performance. Walter, > could you take a look at that? Thank! Doing less is always faster :-). Walter, does it make sense to preserve all local types ? I looked at the APT type system and I could not find a way to access a local types from the API. If this is true, we could simplify the code even more.
Created attachment 153180 [details] Complement to fix issues reported in comment 36 I am marking 3.5.x and 3.4.x patches are obsolete as they need to be updated.
(In reply to comment #34) > Pulled in the changes on to 3.4 maintenance branch > There is one change that is different here. > parseClassBodyDeclarations() of parser.java was not doing a diet parse but > doing a complete parse. Hence, I made changes in > consumeConstructorDeclaration() and consumeMethodDeclaration(). I tried to keep > my changes small. Olivier, Please review. Patch looks good. In this case I don't think you need the code in the parse(...) methods. We might actually do the same in all streams, but since this is not a requirement to fix this problem, I would do it later as a tuning. So please simply update 3.4.x and 3.5.x patches based on the last patch for HEAD.
(In reply to comment #42) > In this case I don't think you need the code in the parse(...) methods. We > might actually do the same in all streams, but since this is not a requirement > to fix this problem, I would do it later as a tuning. Forget my comment. Both are required. One in full parse mode and one in diet parse mode. Since 3.5 and HEAD are using only diet parse, the patch is fine.
(In reply to comment #40) > (In reply to comment #39) > Walter, does it make sense to preserve all local types ? I looked at the APT > type system and I could not find a way to access a local types from the API. > If this is true, we could simplify the code even more. Correct, there is no way from the APT API to access local types. I believe it should be fine to discard that information.
Created attachment 153220 [details] Patch on 3.4 maintenance branch Made changes as for Daniel's comments from comment#36. Also removed the checks for local types as they are not required as for Walter's comments.
Created attachment 153221 [details] Patch on 3.5 maintenance branch Made changes as for Daniel's comments from comment#36. Also removed the checks for local types as they are not required as for Walter's comments
The patch for 3.4.x looks good, but the patch for 3.5.x still referenced the old name ignoreMethodBodies(). So +1 for the former, but -1 for the latter. Please attach a new patch for 3.5.x.
Created attachment 153295 [details] Patch on 3.5 maintenance branch Attached the wrong patch file in comment #46
3.4 patch misses to update the bundle version in /org.eclipse.jdt.apt.core/META-INF/MANIFEST.MF to 3.3.102.qualifier. NOTE: Please also check the bundle version of the test bundles (I didn't review the tests). 3.5 patch misses to update the bundle version in /org.eclipse.jdt.apt.core/META-INF/MANIFEST.MF to 3.3.202.qualifier. NOTE: Please also check the bundle version of the test bundles (I didn't review the tests). OK to commit to maintenance streams with above corrections. Minor fixes for HEAD: - Javadoc of ASTParser.setIgnoreMethodBodies(...) needs </p>. - could add a createParser(...) method to BaseProcessorEnv to reduce code duplication
> - Javadoc of ASTParser.setIgnoreMethodBodies(...) needs </p>. and "This settings is not used if ..." should be "setting" (singular).
In HEAD, Javadoc of ASTParser.setIgnoreMethodBodies(...) says: * <p>If a method contains local types, its method body will be retained.</p> => When I try it with the ASTView from HEAD, I don't see any retained method bodies even if they contain a local or an anonymous class.
(In reply to comment #51) > In HEAD, Javadoc of ASTParser.setIgnoreMethodBodies(...) says: > * <p>If a method contains local types, its method body will be retained.</p> > > => When I try it with the ASTView from HEAD, I don't see any retained method > bodies even if they contain a local or an anonymous class. Markus, We want to change this behavior. Saying that, I want to look at your test example to verify if we are missing anything. Just curious, do you want to use this mode? If so, do you need the local types? Thanks, Satyam.
Satyam, why did you remove/touch the PMC approval?
(In reply to comment #52) No, I don't plan to use it right now, I just noticed the inconsistency between API and implementation. Not retaining methods that contain local classes is perfectly OK for me. To see what's going on in the AST, we often use the ASTView: http://www.eclipse.org/jdt/ui/astview/index.php To see the new option in the view menu, you need to check it out from HEAD here: Repository: :pserver:anonymous@dev.eclipse.org:/cvsroot/eclipse Module: jdt-ui-home/plugins/org.eclipse.jdt.astview
(In reply to comment #53) > Satyam, why did you remove/touch the PMC approval? Daniel, I think this happened accidentally -- I wasn't aware of this. I still don't know how this would have happened -- Anyways I will be careful from next time.
Released for 3.5.2. Added same regression tests. Walter, I tagged the apt changes for the next M-build.
Backported to 3.4.x.
Looking at #setIgnoreMethodBodies(boolean) again, I don't see any difference to just using the existing #setFocalPosition(int), whose API says: * The AST will include nodes for all of the compilation unit's * package, import, and top-level type declarations. It will also always contain * nodes for all the body declarations for those top-level types, as well * as body declarations for any member types. Now that local types are not preserved any more, I think you should remove the new API again and just use setFocalPosition(0).
> Now that local types are not preserved any more, I think you should remove the > new API again and just use setFocalPosition(0). Markus, The setFocalPosition(0) gets the method bodies also.
(In reply to comment #58) > Now that local types are not preserved any more, I think you should remove the > new API again and just use setFocalPosition(0). setFocalPosition(..) is not used when calling createASTs(..). It could be used when you want to create a single unit. The APT case is explicitly related to creating multiple units at the same time. Did I miss something ?
(In reply to comment #59) > Markus, The setFocalPosition(0) gets the method bodies also. Usually it doesn't, but Olivier is right about the caveat in creatASTs(..) (In reply to comment #60) > Did I miss something ? No, you're absolutely right, sorry. I forgot about the createASTs(..) case. The Javadoc of setFocalPosition(..) sounded so absolute -- maybe you could add a note to the 3 setters that don't do anything for createASTs(..).
(In reply to comment #61) > No, you're absolutely right, sorry. I forgot about the createASTs(..) case. The > Javadoc of setFocalPosition(..) sounded so absolute -- maybe you could add a > note to the 3 setters that don't do anything for createASTs(..). I opened bug 296708 to improve this.
when will be 3.5.2 officially released? Thanks Srinivas Surapaneni
Generally the x.y.2 releases come out around February. If you look for when 3.4.2 was released, I imagine 3.5.2 will be one year +/1 2 weeks :-)
... or you consult the timeline (linked from the project page of the "eclipse" project): http://www.eclipse.org/projects/timeline/index.php?projectid=eclipse
I no longer see the OOME. Verified for 3.6M4 using build I20091207-1800
