Bugzilla – Full Text Bug Listing
| Summary: | [Edit] memory leak in compare with each other | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Manfred Klug <manklu> | ||||||||||
| Component: | Compare | Assignee: | Tomasz Zarna <tomasz.zarna> | ||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||
| Severity: | major | ||||||||||||
| Priority: | P3 | CC: | daniel_megert, malgorzata.tomczyk, remy.suen, stephan.herrmann | ||||||||||
| Version: | 3.7 | ||||||||||||
| Target Milestone: | 3.8 M3 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Windows XP | ||||||||||||
| See Also: | https://bugs.eclipse.org/bugs/show_bug.cgi?id=366012 | ||||||||||||
| Whiteboard: | |||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 361417 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Manfred Klug
Created attachment 200770 [details]
Test Project
If I open 19 files after step 3, the memory is freed. Here is the stacktrace mentioned in comment 0. org.eclipse.swt.SWTException: Failed to execute runnable (java.lang.OutOfMemoryError) at org.eclipse.swt.SWT.error(SWT.java:4283) at org.eclipse.swt.SWT.error(SWT.java:4198) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:138) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4140) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3757) at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:2696) at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2660) at org.eclipse.ui.internal.Workbench.access$4(Workbench.java:2494) at org.eclipse.ui.internal.Workbench$7.run(Workbench.java:674) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:667) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149) at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:123) 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:344) 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:48) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:37) at java.lang.reflect.Method.invoke(Method.java:600) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:622) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:577) at org.eclipse.equinox.launcher.Main.run(Main.java:1410) at org.eclipse.equinox.launcher.Main.main(Main.java:1386) Caused by: java.lang.OutOfMemoryError at java.lang.StringBuffer.ensureCapacityImpl(StringBuffer.java:335) at java.lang.StringBuffer.append(StringBuffer.java:111) at org.eclipse.core.internal.filebuffers.ResourceTextFileBuffer.setDocumentContent(ResourceTextFileBuffer.java:570) at org.eclipse.core.internal.filebuffers.ResourceTextFileBuffer.initializeFileBufferContent(ResourceTextFileBuffer.java:286) at org.eclipse.core.internal.filebuffers.ResourceFileBuffer.create(ResourceFileBuffer.java:245) at org.eclipse.core.internal.filebuffers.TextFileBufferManager.connect(TextFileBufferManager.java:112) at org.eclipse.ui.editors.text.TextFileDocumentProvider.createFileInfo(TextFileDocumentProvider.java:559) at org.eclipse.ui.editors.text.TextFileDocumentProvider.connect(TextFileDocumentProvider.java:478) at org.eclipse.compare.SharedDocumentAdapter.connect(SharedDocumentAdapter.java:48) at org.eclipse.team.internal.ui.synchronize.EditableSharedDocumentAdapter.connect(EditableSharedDocumentAdapter.java:90) at org.eclipse.compare.contentmergeviewer.TextMergeViewer$ContributorInfo.connect(TextMergeViewer.java:857) at org.eclipse.compare.contentmergeviewer.TextMergeViewer$ContributorInfo.connectToSharedDocument(TextMergeViewer.java:839) at org.eclipse.compare.contentmergeviewer.TextMergeViewer$ContributorInfo.createDocument(TextMergeViewer.java:805) at org.eclipse.compare.contentmergeviewer.TextMergeViewer$ContributorInfo.internalSetDocument(TextMergeViewer.java:682) at org.eclipse.compare.contentmergeviewer.TextMergeViewer$ContributorInfo.setDocument(TextMergeViewer.java:646) at org.eclipse.compare.contentmergeviewer.TextMergeViewer.updateContent(TextMergeViewer.java:2831) at org.eclipse.compare.contentmergeviewer.ContentMergeViewer.internalRefresh(ContentMergeViewer.java:743) at org.eclipse.compare.contentmergeviewer.ContentMergeViewer.inputChanged(ContentMergeViewer.java:643) at org.eclipse.jface.viewers.ContentViewer.setInput(ContentViewer.java:280) at org.eclipse.compare.CompareViewerSwitchingPane.setInput(CompareViewerSwitchingPane.java:277) at org.eclipse.compare.internal.CompareContentViewerSwitchingPane.setInput(CompareContentViewerSwitchingPane.java:158) at org.eclipse.compare.CompareEditorInput.internalSetContentPaneInput(CompareEditorInput.java:845) at org.eclipse.compare.CompareEditorInput.access$8(CompareEditorInput.java:843) at org.eclipse.compare.CompareEditorInput$11.run(CompareEditorInput.java:779) at org.eclipse.swt.custom.BusyIndicator.showWhile(BusyIndicator.java:70) at org.eclipse.compare.CompareEditorInput.feed1(CompareEditorInput.java:773) at org.eclipse.compare.CompareEditorInput.feedInput(CompareEditorInput.java:751) at org.eclipse.compare.CompareEditorInput.createContents(CompareEditorInput.java:555) at org.eclipse.compare.internal.CompareEditor.createCompareControl(CompareEditor.java:462) at org.eclipse.compare.internal.CompareEditor.access$6(CompareEditor.java:422) at org.eclipse.compare.internal.CompareEditor$3.run(CompareEditor.java:378) at org.eclipse.ui.internal.UILockListener.doPendingWork(UILockListener.java:164) at org.eclipse.ui.internal.UISynchronizer$3.run(UISynchronizer.java:158) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:135) ... 23 more An instance of org.eclipse.ui.internal.EditorHistory seems to be a leak problem here. It occupies 382 862 904 (86,58%) bytes. I believe this would not be an issue if bug 231555 was fixed, preventing oversized CompareEditorInputs to stay in memory. This of course doesn't change the fact that CompareEditorInput is the culprit, since it's far from being a lightweight object (not following contract mentioned on bug 228004, comment 1). (In reply to comment #2) > If I open 19 files after step 3, the memory is freed. This makes perfect sense, if we assume the compare editor input is removed from a FIFO list of recently opened files. Another thing I spotted: CompareEditorInputs are added to the EditorHistory list (increasing its size) even though they represent the same pair of files. Is that because the editor input doesn't override Object.equals(Object)? This could prevent the OOME, as the input would have been added to history only once. Am I right Remy? My guess regarding overriding Object.equals(Object) was correct, it is used to remove duplicated entries from the history list. However, there is a problem with implementing that method for Compare Editor with Saveables. When being added to the history list, compare result is not yet computed, so getActiveSaveables returns an empty array. This makes the code below, my first attempt to implement #equals(Object), useless:
public boolean equals(Object obj) {
if (this == obj)
return true;
if (!(obj instanceof ISaveablesSource))
return false;
Saveable[] s1 = this.getSaveables();
Saveable[] s2 = ((ISaveablesSource) obj).getSaveables();
if (s1.length != s2.length)
return false;
for (int i = 0; i < s1.length; i++)
if (!s1[i].equals(s2[i]))
return false;
return true;
}
Created attachment 201254 [details]
An excerpt from dominator tree
Dominator tree from a heap dump analysis for this scenario shows that the real problem is caused by huge Strings kept by Documents from SaveablesCompareEditorInput. Shouldn't these Strings/TextStores/Documents be released on editor's disposal?
Created attachment 201255 [details]
mylyn/context/zip
(In reply to comment #7) > Shouldn't these Strings/TextStores/Documents be released on editor's disposal? Dani what's your opinion on it? (In reply to comment #7) > Created attachment 201254 [details] > An excerpt from dominator tree > > Dominator tree from a heap dump analysis for this scenario shows that the real > problem is caused by huge Strings kept by Documents from > SaveablesCompareEditorInput. Shouldn't these Strings/TextStores/Documents be > released on editor's disposal? Those are objects like any others, which means they are there until no longer referenced. If someone decides to hold on to them, then it is their job to release the reference (or fault if not done ;-). Didn't look into it in more detail but maybe bug 288596 is related? (In reply to comment #11) > Didn't look into it in more detail but maybe bug 288596 is related? It does look similar, but it turned out to be a dupe of bug 297950. Thanks anyway. Created attachment 201400 [details] Fix v01 A patch that is closer to fixing the actual culprit: * Removes the reference to a Document in LocalResourceSaveableComparison. The saveable seems to be doing fine by getting the Document with getAdapter(IDocument.class)[1] when needed. * Cleans selection provider for Compare Editor site on the editor's disposal. The provider kept reference to compare editor's viewers[2][3]. Even though the patch fixes scenario from comment 0, I'm still not convinced if it's good enough. Seeing Compare Editor sites in a heap dump after their editors have been closed makes me think I should go a step further. [1] org.eclipse.team.internal.ui.synchronize.LocalResourceSaveableComparison.getAdapter(Class) [2] org.eclipse.compare.internal.CompareEditorSelectionProvider.fViewers [3] org.eclipse.compare.internal.CompareEditorSelectionProvider.fViewerInFocus (In reply to comment #13) > Seeing Compare Editor sites in a heap dump after their > editors have been closed makes me think I should go a step further. I'm just now looking at a heap dump of a point in time when all compare editors were closed, yet compare editors caused exceptions to be logged. For some of the objects from org.eclipse.compare.* I see paths to GC root that end with ... - fViewer org.eclipse.compare.CompareEditorInput$10 @ 0x74ed75e8 - [6147], [6214] org.eclipse.swt.widgets.Widget[12288] @ 0x7db24be8 - widgetTable org.eclipse.swt.widgets.Display @ 0x609b9138 Native Stack where CompareEditorInput$10 seems to be the anonymous subclass of CompareStructureViewerSwitchingPane. Also direct instances of CompareStructureViewerSwitchingPane are still accessible via the Display along similar paths. Here's a wild guess: is this because CompareEditorInput.handleDispose() nulls out its fields without calling dispose() before?? Most of these fields are actually widgets that are referenced by the Display unless disposed, no? PS: Sorry, no reproducable steps, the exceptions occur after some hours of normal work, involving switching compare editors, editing in compare editors, closing compare editors (e.g., by reverting the changes they show) etc. Using the same heap dump as in comment 14 (no compare editors open) I can see 30 instances of JavaReconciler running in their respective AbstractReconciler.BackgroundThread. At least 5 of these are connected to a JavaMergeViewer (subclass of TextMergeViewer). I can see that TextMergeViewer can install a reconciler, but I couldn't find the path where this reconciler is uninstalled/canceled again. How is termination of reconcilers supposed to happen? > I can see that TextMergeViewer can install a reconciler, but I couldn't
> find the path where this reconciler is uninstalled/canceled again.
> How is termination of reconcilers supposed to happen?
The disposal of the viewer's StyledText widget triggers an event which un-configures the viewer and uninstalls the reconciler. This seems to work fine in the scenarios I just tested.
another xref: (In reply to comment #14) > Here's a wild guess: is this because CompareEditorInput.handleDispose() > nulls out its fields without calling dispose() before?? Most of these > fields are actually widgets that are referenced by the Display unless > disposed, no? This is what I currently suspect to be the root cause for all the nasty effects like in bug 320523 and its dups. (In reply to comment #14) > Here's a wild guess: is this because CompareEditorInput.handleDispose() > nulls out its fields without calling dispose() before?? Most of these > fields are actually widgets that are referenced by the Display unless > disposed, no? Thanks for the input Stephan, but I don't this is related to this particular leak. Why don't you open a separate bug (if it's not already there) for the piece of code that nulls those fields instead of disposing them. btw, have you tried fix the code and see if that helps? (In reply to comment #18) > Thanks for the input Stephan, but I don't this is related to this particular > leak. Why don't you open a separate bug (if it's not already there) for the > piece of code that nulls those fields instead of disposing them. btw, have you > tried fix the code and see if that helps? I just tried to further narrow down and found this - in 3.7.0 I could reproduce that dangling instance of JavaMergeViewer by closing a dirty compare editor (without saving). - in I20110830-1241 I seem to be unable to reproduce Looking at the reference chain in 3.7.0 I see that ModelCompareEditorInput.fRightDirtyViewer is the reference that keeps the JavaMergeViewer alive. This suggests that the bug I'm seeing in 3.7.0 may have been fixed as a side effect of bug 348787, where the mentioned field was changed to a boolean. Can someone with more background knowledge about bug 348787 confirm that it may have fixed a leak? Anyway, I'll upgrade to a build >= I20110823-0800 and hope that my issues might be fixed. (In reply to comment #19) > Can someone with more background knowledge about bug 348787 confirm that > it may have fixed a leak? Gosia, could you comment on this? (In reply to comment #13) > Created attachment 201400 [details] > Fix v01 I will review the patch and release it at the beginning of M3. (In reply to comment #19) > Looking at the reference chain in 3.7.0 I see that > ModelCompareEditorInput.fRightDirtyViewer is the reference that keeps > the JavaMergeViewer alive. This suggests that the bug I'm seeing in 3.7.0 > may have been fixed as a side effect of bug 348787, where the mentioned > field was changed to a boolean. > > Can someone with more background knowledge about bug 348787 confirm that > it may have fixed a leak? If really reference ModelCompareEditorInput.fRightDirtyViewer was causing the memory leak then as you see removing the reference in bug 348787 should as a side effect fix the memory leak. Of course provided that ModelCompareEditorInput.fRightDirtyViewer was really the cause of the problem. The patch is not good, messing with the IDocument reference resulted in the CompareEditor not being removed from SaveablesList.modelMap after closing the editor. I'm going to look for a better fix. The simplest solution seems to be the best one in this case ie. nulling document reference in org.eclipse.team.internal.ui.synchronize.LocalResourceSaveableComparison.dispose() repairs the leak. In fact, this is what Dani suggested in comment 10. So far, I haven't noticed any side effects. So, I patched my Eclipse and if it runs smoothly for the next few days I'll release the fix. Fixed with e4281ecfd04ac79545ba778419a1cfa257818d4c. Available in builds >= N20111012-2000. |