Community
Participate
Working Groups
Eclipse IDE loads the same images (toolbar icons) over and over again because it decreases the reference count to 0 before applying the same images again to the toolbar. The idea is to either keep references to a fixed set of most used icons or simply automatically determine those by a LRU cache. The cache must be limited to avoid too many OS handles. By using SoftReferences it can also be avoided to accidentally cache too many big images which may end up in Out of Memory Error. In a first test a little cache of 300 cached images was enough to never trigger reload during typical actions in jdt ui.
Cool idea. +1
the e4-tooling has something like that already (and some DI magic)
(In reply to Thomas Schindl from comment #2) > the e4-tooling has something like that already (and some DI magic) can you point to a classname please? do you know if that solution is somehow better/worse then the suggested change?
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/186086 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=dd760156c66832fac12586d4c7a38c80c1657afc
Jörg, please add to N&N how the cache size can be configured in isv document.
New Gerrit change created: https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/187216
This change causes memory leaks, visible via 8 new JavaLeakTest failures on all platforms, here for example https://download.eclipse.org/eclipse/downloads/drops4/I20211101-1800/testresults/html/org.eclipse.jdt.ui.tests_ep422I-unit-cen64-gtk3-java11_linux.gtk.x86_64_11.html This is caused by keeping DerivedImageDescriptor instances in the cache. Looking on the DerivedImageDescriptor, it should never assume it can be cached because it keeps the original descriptor reference in the field. Same applies for DeferredImageDescriptor. While the code in AccessibleArrowImage can be clearly improved to be a static class and not keep the reference to the editor (via this$0 chain), there is now a possibility that other 3rd party code will now leak memory as well, and this is not acceptable. Beside this, LazyResourceManager should have a license header. I will push a patch in a moment.
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/187221
(In reply to Andrey Loskutov from comment #7) > This is caused by keeping DerivedImageDescriptor instances in the cache. > Looking on the DerivedImageDescriptor, it should never assume it can be > cached because it keeps the original descriptor reference in the field. Same > applies for DeferredImageDescriptor. I am not sure that the issue is understood. A leak would mean that endless memory could be consumed. Instead the only a finite amount of Images i cached. They are not leaked but intentional cached - together with their original ImageDescriptor. What's the problem with that? What am i missing? How to run that JavaLeakTest? i get "java.lang.RuntimeException: JVM settings for --add-modules, --add-opens, and --illegal-access are probably missing."
(In reply to Jörg Kubitz from comment #9) > (In reply to Andrey Loskutov from comment #7) > > This is caused by keeping DerivedImageDescriptor instances in the cache. > > Looking on the DerivedImageDescriptor, it should never assume it can be > > cached because it keeps the original descriptor reference in the field. Same > > applies for DeferredImageDescriptor. > > I am not sure that the issue is understood. A leak would mean that endless > memory could be consumed. Instead the only a finite amount of Images i > cached. The descriptor instances keep references to original descriptor / supplier objects. These can keep references to *anything* in 3rd party code (as happened for the particular JDT editor test). Imagine, you have an editor that uses N GB memory and that you expect that after closing this editor the memory is freed up by GC. With the change that will be not the case anymore, because the editor data may be kept forever via the reference chain starting from the descriptor. If you haven't noticed yet: you see this chain in the test fail message: org.eclipse.jdt.internal.ui.javaeditor.CompilationUnitEditor$AdaptedSourceViewer#this$0 -> org.eclipse.jdt.internal.ui.javaeditor.CompilationUnitEditor@1921a985 org.eclipse.jdt.internal.ui.javaeditor.breadcrumb.EditorBreadcrumb#fTextViewer -> org.eclipse.jdt.internal.ui.javaeditor.CompilationUnitEditor$AdaptedSourceViewer@397a8ad2 org.eclipse.jdt.internal.ui.javaeditor.breadcrumb.EditorBreadcrumb$$Lambda$1274/0x0000000100e64c40#arg$1 -> org.eclipse.jdt.internal.ui.javaeditor.JavaEditorBreadcrumb@d7ab200 java.lang.Object[0] -> org.eclipse.jdt.internal.ui.javaeditor.breadcrumb.EditorBreadcrumb$$Lambda$1274/0x0000000100e64c40@ org.eclipse.core.runtime.ListenerList#listeners -> [Ljava.lang.Object;@112f5b41 org.eclipse.jdt.internal.ui.javaeditor.breadcrumb.BreadcrumbViewer#fMenuListeners -> [org.eclipse.jdt.internal.ui.javaeditor.breadcrumb.EditorBreadcrumb$$Lambda$1274/0x0000000100e64c40 org.eclipse.jdt.internal.ui.javaeditor.breadcrumb.BreadcrumbItem#fParent -> org.eclipse.jdt.internal.ui.javaeditor.JavaEditorBreadcrumb$ProblemBreadcrumbViewer@6c003280 org.eclipse.jdt.internal.ui.javaeditor.breadcrumb.BreadcrumbItemDropDown#fParent -> BreadcrumbItem {*Disposed*} org.eclipse.jdt.internal.ui.javaeditor.breadcrumb.BreadcrumbItemDropDown$AccessibleArrowImage#this$0 -> org.eclipse.jdt.internal.ui.javaeditor.breadcrumb.BreadcrumbItemDropDown@67e54d16 org.eclipse.jface.resource.DerivedImageDescriptor#original -> org.eclipse.jdt.internal.ui.javaeditor.breadcrumb.BreadcrumbItemDropDown$AccessibleArrowImage@5eae3 java.util.HashMap$Node#key -> org.eclipse.jface.resource.DerivedImageDescriptor@5eae3eef java.util.HashMap$Node[65] -> org.eclipse.jface.resource.DerivedImageDescriptor@5eae3eef=org.eclipse.jface.resource.DeviceResourc java.util.HashMap#table -> [Ljava.util.HashMap$Node;@6039d0e org.eclipse.jface.resource.LazyResourceManager#unreferenced -> {URLImageDescriptor(platform:/plugin/org.eclipse.ui/icons/full/etool16/print_edit.png)=org.eclipse. org.eclipse.jface.resource.LocalResourceManager#parentRegistry -> org.eclipse.jface.resource.LazyResourceManager@2c17c0b1 org.eclipse.ui.internal.ide.IDEWorkbenchPlugin#resourceManager -> > How to run that JavaLeakTest? i get > "java.lang.RuntimeException: JVM settings for --add-modules, --add-opens, > and --illegal-access are probably missing." Me too, I assume you have to run the test.xml manually.
(In reply to Andrey Loskutov from comment #10) > happened for the particular JDT editor test). Imagine, you have an editor > that uses N GB memory and that you expect that after closing this editor the > memory is freed up by GC. With the change that will be not the case anymore, > because the editor data may be kept forever via the reference chain starting > from the descriptor. understood. So "original.shouldBeCached()" should be fine since other cached instances do not have that problem.
This broke javadoc generation as per https://download.eclipse.org/eclipse/downloads/drops4/I20211101-1800/compilelogs/platform.doc.isv.javadoc.txt . It is generated via https://git.eclipse.org/c/platform/eclipse.platform.common.git/tree/bundles/org.eclipse.platform.doc.isv . To unbreak the build pde annotations should be added somewhere in https://git.eclipse.org/c/platform/eclipse.platform.common.git/tree/bundles/org.eclipse.platform.doc.isv/platformOptions.txt
(In reply to Alexander Kurtakov from comment #12) > should be added somewhere Do you know where? i don't understand that file. Please help.
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.common/+/187222
(In reply to Eclipse Genie from comment #14) > New Gerrit change created: > https://git.eclipse.org/r/c/platform/eclipse.platform.common/+/187222 ^^ is enough to fix the build locally. But please check tomorrow that there are no javadoc errors.
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.common/+/187222 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.common.git/commit/?id=7bdfd8b96ed2af6070eea1f2995bd9a404aeb321
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/187221 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=02d5ed14011b10b3959a04eb56188fcdb088a50c
Thanks Andrey, Alexander for cleaning up!
There are no related fails in I20211103-0020: https://download.eclipse.org/eclipse/downloads/drops4/I20211103-0020/testResults.php Anything else missing here?
Gerrit change https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/187216 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=6fe5985509f6d6e051e5ea837017c5020716cba8
