Community
Participate
Working Groups
After bug 569752 we can track non disposed SWT Resources, but default reporting is going to console and not to the IDE error log. We should add default implementation of NonDisposedReporter to the IDEApplication, so the possible leaks are reported to Eclipse log (if "-Dorg.eclipse.swt.graphics.Resource.reportNonDisposed=stacks" system property is set), and also set this property for SDK product, so that it is reported by default for plain SDK consumers (mostly developers). I plan to provide patches.
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/174289
(In reply to Eclipse Genie from comment #1) > New Gerrit change created: > https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/174289 This produces something like this: org.eclipse.ui.ide Error Tue Jan 05 18:15:40 CET 2021 Not properly disposed SWT resource: Path {140737233996832} org.eclipse.swt.graphics.Resource$NonDisposedException at org.eclipse.swt.graphics.Resource.<init>(Resource.java:117) at org.eclipse.swt.graphics.Path.<init>(Path.java:85) at org.eclipse.jdt.internal.ui.javaeditor.breadcrumb.BreadcrumbViewer.doUpdateItem(BreadcrumbViewer.java:352) at org.eclipse.jface.viewers.StructuredViewer$UpdateItemSafeRunnable.run(StructuredViewer.java:427) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:45) at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:174) at org.eclipse.jface.viewers.StructuredViewer.updateItem(StructuredViewer.java:2111) at org.eclipse.jface.viewers.StructuredViewer.internalUpdate(StructuredViewer.java:2094) at org.eclipse.jface.viewers.StructuredViewer.update(StructuredViewer.java:2035) at org.eclipse.jface.viewers.StructuredViewer.update(StructuredViewer.java:1979) at org.eclipse.jface.viewers.StructuredViewer.handleLabelProviderChanged(StructuredViewer.java:1158) at org.eclipse.jdt.internal.ui.javaeditor.JavaEditorBreadcrumb$ProblemBreadcrumbViewer.handleLabelProviderChanged(JavaEditorBreadcrumb.java:185) at org.eclipse.jface.viewers.ContentViewer$1.labelProviderChanged(ContentViewer.java:95) at org.eclipse.jface.viewers.BaseLabelProvider$1.run(BaseLabelProvider.java:75) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:45) at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:174) at org.eclipse.jface.viewers.BaseLabelProvider.fireLabelProviderChanged(BaseLabelProvider.java:72) at org.eclipse.ui.internal.decorators.DecoratorManager$1.run(DecoratorManager.java:347) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:45) at org.eclipse.ui.internal.decorators.DecoratorManager.fireListener(DecoratorManager.java:344) at org.eclipse.ui.internal.decorators.DecorationScheduler$3.runInUIThread(DecorationScheduler.java:525) at org.eclipse.ui.progress.UIJob.lambda$0(UIJob.java:95) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:40) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:185) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:5043) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:4549) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1157) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:338) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1046) at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:155) at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:644) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:338) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:551) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:156) at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:152) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:203) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:134) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:401) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:255) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:566) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:653) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:590) at org.eclipse.equinox.launcher.Main.run(Main.java:1461) at org.eclipse.equinox.launcher.Main.main(Main.java:1434)
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.releng.aggregator/+/174292
(In reply to Eclipse Genie from comment #3) > New Gerrit change created: > https://git.eclipse.org/r/c/platform/eclipse.platform.releng.aggregator/+/ > 174292 @Platform UI: I'm not sure about adding the new system property to the SDK product (patch above), mostly because of possible slowdown. Every creation of new SWT resource (and that happens constantly, like creating SWT Regions) would trigger a new exception object to be created (and that records the stack and this costs time). I personally don't feel any difference on my workstation, but I have a 16 core Xeon with 256 GB RAM, so it is obviously not the mainstream config. Should we merge after M1 and see how it goes? If nobody complains, we can simply keep it enabled for SDK? Anyone has any objections?
(In reply to Andrey Loskutov from comment #4) > > Should we merge after M1 and see how it goes? If nobody complains, we can > simply keep it enabled for SDK? Anyone has any objections? Can't we automatically activate it based on the product? The people that we reach by this are the ones that install a new product from the milestone build. I think most committers upgrade from i-builds which would not activate the property? Would be nice to have a preference for it as well.
(In reply to Wim Jongman from comment #5) > (In reply to Andrey Loskutov from comment #4) > > > > > Should we merge after M1 and see how it goes? If nobody complains, we can > > simply keep it enabled for SDK? Anyone has any objections? > > > Can't we automatically activate it based on the product? That would be the patch https://git.eclipse.org/r/c/platform/eclipse.platform.releng.aggregator/+/174292
(In reply to Andrey Loskutov from comment #6) > (In reply to Wim Jongman from comment #5) > > (In reply to Andrey Loskutov from comment #4) > > > > > > > > Should we merge after M1 and see how it goes? If nobody complains, we can > > > simply keep it enabled for SDK? Anyone has any objections? > > > > > > Can't we automatically activate it based on the product? > > That would be the patch > https://git.eclipse.org/r/c/platform/eclipse.platform.releng.aggregator/+/ > 174292 Yes, I saw it. But this would only be available after the installation of a new product. I wonder how many people will install milestone SDK's I suggest to check the product ID at runtime and automatically activate the property when people are running the SDK.
(In reply to Wim Jongman from comment #7) > Yes, I saw it. But this would only be available after the installation of a > new product. I wonder how many people will install milestone SDK's I do it every day, it takes few minutes after SDK download :) > I suggest to check the product ID at runtime and automatically activate the > property when people are running the SDK. No, that would mean this would *always* be enabled for SDK, unconditionally. I don't want to force everyone into this without giving possibility to escape. We have no rush to have this, and everyone who is interested can do it immediately by enabling the property manually.
(In reply to Andrey Loskutov from comment #8) > (In reply to Wim Jongman from comment #7) > > Yes, I saw it. But this would only be available after the installation of a > > new product. I wonder how many people will install milestone SDK's > > I do it every day, it takes few minutes after SDK download :) > > > I suggest to check the product ID at runtime and automatically activate the > > property when people are running the SDK. > > No, that would mean this would *always* be enabled for SDK, unconditionally. > I don't want to force everyone into this without giving possibility to > escape. We have no rush to have this, and everyone who is interested can do > it immediately by enabling the property manually. Ok, that makes sense.
Created attachment 285202 [details] Perf measurement snippet Let's talk some numbers, shall we? On my Win10 machine, snippet reports 350000 exceptions created per sec, 1920 bytes of memory per exception. I would (educated guess) estimate that Eclipse uses fewer than 1000 resources at a time and creates fewer than 1000 resources per sec. This gives: (1920 * 1000) ~= 2mb extra memory (1000/350000) ~= 0.3% slowdown. My conclusion is that in case of Eclipse, you can default-enable tracking if you like.
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/174289 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=99cbe6f11ddf9fc4edad7b1d03480703992583c5
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.releng.aggregator/+/174292 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.releng.aggregator.git/commit/?id=123bce73c50c6605a34f672a69ee9054f966c3e2
Now everything is merged and I've found already two issues - see bug 570212 and bug 570211.
Please add to noteworthy (how to enable it...)
I understand that it's already added here: Bug 569752 comment 10
Run our product build with that change, amazing how many leaks one can find! Found two in Xtext too: https://github.com/eclipse/xtext-eclipse/issues/1621 & https://github.com/eclipse/xtext-eclipse/issues/1622.
(In reply to Andrey Loskutov from comment #16) > Run our product build with that change, amazing how many leaks one can find! > Found two in Xtext too: https://github.com/eclipse/xtext-eclipse/issues/1621 > & https://github.com/eclipse/xtext-eclipse/issues/1622. LOL. Same here. This is very useful. I noticed that it also reports Font leaks. I thought Fonts don't have to be disposed of any more?
(In reply to Wim Jongman from comment #17) > (In reply to Andrey Loskutov from comment #16) > > Run our product build with that change, amazing how many leaks one can find! > > Found two in Xtext too: https://github.com/eclipse/xtext-eclipse/issues/1621 > > & https://github.com/eclipse/xtext-eclipse/issues/1622. > > LOL. Same here. This is very useful. > > I noticed that it also reports Font leaks. I thought Fonts don't have to be > disposed of any more? You mean Colors. Fonts are still system resources.
Some Fonts are excluded from tracking in Bug 569752, specifically the Fonts that SWT don't "own". For illustration, consider the following possible scenario: 1) SWT creates a new Font. This one will be tracked and reported. 2) SWT sets the Font to a Canvas. 3) SWT receives Paint event and wants to construct a GC. 4) GC requests current font from OS and constructs an SWT Font wrapper. This wrapper does not "own" the font, so it will be excluded from tracking. In this scenario, Font from (1) is tracked and reported, while font from (4) is not. To my knowledge, all Fonts reported with current SWT are actual leaks. If in doubt, give me the allocation stack and we can figure if I overlooked something.
And yes, Fonts are OS resources and need to be disposed properly.
(In reply to Alexandr Miloslavskiy from comment #20) This is a brilliant tool, Alexandr. Thanks. > And yes, Fonts are OS resources and need to be disposed properly. I was confused with Colors. https://www.eclipse.org/eclipse/news/4.17/platform_isv.php#swt-device-free-constructors
Wim, there's a bit of a battle in Bug 569752, it would help if you praise the new feature a bit there :) Thanks for liking my thing :)
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/175056
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.releng.aggregator/+/175059
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.releng.aggregator/+/175059 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.releng.aggregator.git/commit/?id=a40efe0fb30771ed6e3fe05c2561db9b55cd098a
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/175056 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=0d0065e1b4a3431ebe42788fa24d47272fe386b9
Should we add a note to https://git.eclipse.org/r/plugins/gitiles/platform/eclipse.platform.common/+/refs/heads/master/bundles/org.eclipse.platform.doc.isv/guide/jface_resources.htm about this new feature?
