Community
Participate
Working Groups
Build: 3.5 M6 The core tools plugin has a button called "count resources" that will walk over the workspace and count all resources in the tree. I recently updated this to also count hidden resources, and I found the following: 1) Start from empty workspace 2) Click count resources -> 1 resource (workspace root) 3) File > New > Plugin Project 4) Enter a name. Click Next. Click Finish (accept all defaults) 5) Count resources -> 4314 resources (mostly in jdt internal folders project) 6) Delete the project 7) Count resources -> 4301 resources -> There are still 4300 resources in my workspace, even though the workspace appears to be empty. The JDT external folders project doesn't seem to be cleaning itself up.
It should be cleaned-up on full save. Can you please confirm/infirm?
Yes, if I shutdown and restart the extra resources are gone.
Thanks for the confirmation. This behavior is by designed. For performance reason, we don't clean-up the external folder when the reference is removed (we would have to search for all classpath entries that may still reference it). No intent to change this for now.
It seems like a potentially large memory leak if the user doesn't shutdown often. One option would be to also perform cleanup on incremental save (snapshot) in JavaModelManager#saving. Snapshot runs roughly every 5 minutes, and since it happens in a background thread the performance cost wouldn't be disruptive for end users.
How much are we leaking? We need numbers before deciding if this is worth fixing this, and compare to the risk of fixing this very sensitive code this late in the release.
>It seems like a potentially large memory leak if the user doesn't shutdown >often. It's not really a leak (at least not growing) because according to Jérome the external folder and its resources are reused when a new Java project is created and hence the problem you see just happens in this very special case where the workspace has no Java project left and the user continues to work without adding new Java projects. I agree with Jérome that it's not worth spending resources for that corner case. By the way I tried to verify what Jérome said but could not find the updated core tools you mention: the latest version on http://www.eclipse.org/eclipse/platform-core/downloads.php is from 2006. Can you add a link to the tool here? Thanks.
>It's not really a leak (at least not growing) because according to J�rome the >external folder and its resources are reused when a new Java project is created >and hence the problem you see just happens in this very special case where the >workspace has no Java project left and the user continues to work without >adding new Java projects. It's a leak in the sense that you have to shutdown the VM to free the memory. Deleting all projects is the easiest way to demonstrate the problem, but the leak occurs every time you add+remove an external library folder. As a simple example I added/removed org.eclipse.swt/bin as an external folder on a generic Java project, and 190KB of memory was leaked in the resource tree (measured via YourKit). It's true that if you keep adding/removing the same folder, it doesn't continue to grow, but each unique external class folder you add/remove will leak more memory. > I agree with J�rome that it's not worth spending resources for that corner > case. If the problem here is just resources, I can provide a patch. I think a simple fix here is trivial. > For performance reason, we don't clean-up the external folder when > the reference is removed I tried instrumenting this, and on my development workspace with 130 plugin projects it took 0ms to perform the cleanup (measured with System.currentTimeMillis()). I also tried adding a few class folders to a project, and then deleted the project (case where real cleanup happens). Again the recorded time was 0ms. This seems like an acceptable performance cost during snapshot, or even on every project deletion.
Created attachment 132825 [details] Potential fix, with performance timing
>It's true that if you keep adding/removing the same folder, it >doesn't continue to grow, but each unique external class folder you add/remove >will leak more memory. Ah, OK. I thought we talk about one global external folder that holds the info. In that case and if the fix is really that trivial we should take it. Jérome? John, do you have a link to the latest version of the core tools?
I generally run with the latest core tools from HEAD. Checkout from CVS, then use the PDE support to installl into the existing workspace. Since this plugin isn't part of the build, pre-built versions on our web site are always out of date.
Jérome, can you take a look at John's latest proposed fix?
John are you saying that adding a link to an external folder takes 190KB? If that is true I would suggest that there is a performance problem in Platform/Resources. Anyway this is too late for M7 to get good test coverage. I see if I can convince myself that this is a good change for RC1. I still think this is sensitive code and I don't want to break products built on top of us with such a change.
> John are you saying that adding a link to an external folder takes 190KB? Yes, in this case it was a folder with many files inside it (all SWT source). The workspace tree state for all those resources takes up memory. This is a large source folder, but I could imagine customer scenarios having even larger ones. > I still think this is sensitive code and I don't want to break products built > on top of us with such a change. Understood. Without knowing the cleanup code in detail, my reasoning was that this code is running today during workspace save, so running during background snapshot didn't seem any more risky. A failure during save on shutdown is potentially much worse than a failure in background snapshot. For what it's worth, I've been running with the attached patch for the past week without any problems.
>my reasoning was that >this code is running today during workspace save, so running during background >snapshot didn't seem any more risky. But do they both have the same lock strategies?
> But do they both have the same lock strategies? Yes, they both lock the workspace during the save. They are even implemented in the same method (SaveManager.save). The difference between them is that they do different amounts of work.
>Yes, they both lock the workspace during the save. Do if we run it as job that job it would block other work which I would not want as a user, right?
> Do if we run it as job that job it would block other work which I would not want as a user, right? Yes, it would block other threads trying to modify the workspace. It needs to write out a consistent state so it can't allow concurrent modifications. From my measurements running with the patch this week, the external folder cleanup is a trivial time compared to the overall snapshot time (System.currentTimeMillis() is too coarse grained and always reports a time of 0 with my patch).
With John's patch, I'm seeing JavaProjectTests failing with the following exception being logged: java.lang.IllegalArgumentException: Attempted to beginRule: P/.org.eclipse.jdt.core.external.folders, does not match outer scope rule: P/JavaProjectTests at org.eclipse.core.runtime.Assert.isLegal(Assert.java:63) at org.eclipse.core.internal.jobs.ThreadJob.illegalPush(ThreadJob.java:122) at org.eclipse.core.internal.jobs.ThreadJob.push(ThreadJob.java:232) at org.eclipse.core.internal.jobs.ImplicitJobs.begin(ImplicitJobs.java:58) at org.eclipse.core.internal.jobs.JobManager.beginRule(JobManager.java:232) at org.eclipse.core.internal.resources.WorkManager.checkIn(WorkManager.java:117) at org.eclipse.core.internal.resources.Workspace.prepareOperation(Workspace.java:1747) at org.eclipse.core.internal.resources.Resource.delete(Resource.java:696) at org.eclipse.core.internal.resources.Resource.delete(Resource.java:682) at org.eclipse.jdt.internal.core.ExternalFoldersManager.cleanUp(ExternalFoldersManager.java:125) at org.eclipse.jdt.internal.core.JavaModelManager.saving(JavaModelManager.java:3983) at org.eclipse.core.internal.resources.SaveManager.executeLifecycle(SaveManager.java:343) at org.eclipse.core.internal.resources.SaveManager$1.run(SaveManager.java:160) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42) at org.eclipse.core.internal.resources.SaveManager.broadcastLifecycle(SaveManager.java:163) at org.eclipse.core.internal.resources.SaveManager.save(SaveManager.java:1012) at org.eclipse.core.internal.resources.SaveManager.save(SaveManager.java:991) at org.eclipse.core.internal.resources.Project.close(Project.java:169) at org.eclipse.jdt.core.tests.model.JavaProjectTests.testGetClasspathOnClosedProject(JavaProjectTests.java:878) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:585) at junit.framework.TestCase.runTest(TestCase.java:164) at junit.framework.TestCase.runBare(TestCase.java:130) at junit.framework.TestResult$1.protect(TestResult.java:106) at junit.framework.TestResult.runProtected(TestResult.java:124) at junit.framework.TestResult.run(TestResult.java:109) at junit.framework.TestCase.run(TestCase.java:120) at junit.framework.TestSuite.runTest(TestSuite.java:230) at org.eclipse.jdt.core.tests.model.SuiteOfTestCases$Suite.runTest(SuiteOfTestCases.java:100) at junit.framework.TestSuite.run(TestSuite.java:225) at org.eclipse.jdt.core.tests.model.SuiteOfTestCases$Suite.superRun(SuiteOfTestCases.java:84) at org.eclipse.jdt.core.tests.model.SuiteOfTestCases$1.protect(SuiteOfTestCases.java:72) at junit.framework.TestResult.runProtected(TestResult.java:124) at org.eclipse.jdt.core.tests.model.SuiteOfTestCases$Suite.run(SuiteOfTestCases.java:81) at org.eclipse.jdt.internal.junit.runner.junit3.JUnit3TestReference.run(JUnit3TestReference.java:130) at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390) at org.eclipse.pde.internal.junit.runtime.RemotePluginTestRunner.main(RemotePluginTestRunner.java:62) at org.eclipse.pde.internal.junit.runtime.CoreTestApplication.run(CoreTestApplication.java:23) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:585) at org.eclipse.equinox.internal.app.EclipseAppContainer.callMethodWithException(EclipseAppContainer.java:574) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:368) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:179) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:585) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:559) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:514) at org.eclipse.equinox.launcher.Main.run(Main.java:1311) at org.eclipse.equinox.launcher.Main.main(Main.java:1287)
Created attachment 134653 [details] Fix v02 This was a bug in my patch. It shouldn't do cleanup on project save (which happens on project close).
All JDT/Core tests pass with John's latest fix. Olivier can you please give a +1 for RC1?
+1. I would factorize the context.getKind() calls.
Fix released for 3.5RC1. Note that there is no regression test since it is not possible to know when a snapshot is performed.
Created attachment 134809 [details] Refactored patch Slightly modified to use a switch statement.
Released refactored patch.
Verified for 3.5RC1 using I20090513-2000
