Community
Participate
Working Groups
To reproduce, run snippet (with VM arguments "-Xmx128M -Xms128M -XX:+AlwaysPreTouch", adjust path to point to any icon image) and observe growing memory consumption: public static void main (String [] args) { String icon = "/home/sandreev/git/eclipse/anyedit/anyedittools/AnyEditTools/icons/invertCase.gif"; Display display = new Display (); Image image = new Image(display, icon); final Shell shell = new Shell (display); shell.setLayout(new GridLayout()); final CTabFolder folder = new CTabFolder(shell, SWT.BORDER); folder.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, false)); folder.setSimple(false); folder.setUnselectedImageVisible(false); folder.setUnselectedCloseVisible(false); List<CTabItem> items = new ArrayList<>(); shell.setSize(300, 300); shell.open(); AtomicBoolean c = new AtomicBoolean(true); Thread t = new Thread(new Runnable() { @Override public void run() { while (c.get()) { if (!display.isDisposed()) { display.asyncExec(new Runnable() { public void run() { if (!display.isDisposed()) { for (CTabItem i : items) { i.dispose(); } items.clear(); for (int i = 0; i < 8; i++) { CTabItem item = new CTabItem(folder, SWT.CLOSE); item.setText("Item "+i); item.setImage(image); Text text = new Text(folder, SWT.MULTI | SWT.V_SCROLL | SWT.H_SCROLL); text.setText("Text for item "+i); item.setControl(text); items.add(item); } } }; }); try { Thread.sleep(1000); } catch (InterruptedException e) { } } } } }); t.start(); while (!shell.isDisposed ()) { if (!display.readAndDispatch ()) display.sleep (); } c.set(false); image.dispose(); display.dispose (); } valgrind reports the leak as e.g.: ==18191== 148,992 (23,808 direct, 125,184 indirect) bytes in 62 blocks are definitely lost in loss record 17 of 14,214 ==18191== at 0x4C2B017: malloc (vg_replace_malloc.c:380) ==18191== by 0x50E685B2: _cairo_image_surface_create_for_pixman_image (cairo-image-surface.c:184) ==18191== by 0x50E68998: _cairo_image_surface_create_with_pixman_format (cairo-image-surface.c:353) ==18191== by 0x7C452F61: Java_org_eclipse_swt_internal_cairo_Cairo_cairo_1image_1surface_1create (in /home/sandreev/git/eclipse/eclipse.platform.swt.binaries/bundles/org.eclipse.swt.gtk.linux.x86_64/libswt-cairo-gtk-4944r26.so) ==18191== by 0x18FD5E8F: ??? ==18191== by 0x11CDFBDB: ??? ==18191== by 0x11400F72: ??? ==18191== by 0x11400CBF: ??? ==18191== by 0x11400F72: ??? ==18191== by 0x11C8A48B: ??? ==18191== by 0x11400FB7: ??? ==18191== by 0x11400F72: ??? ==18191== by 0x11C0AF83: ??? ==18191== by 0x113F7BC8: ??? ==18191== by 0x66FE052: JavaCalls::call_helper(JavaValue*, methodHandle const&, JavaCallArguments*, Thread*) (in /usr/lib/jvm/java-11-openjdk-11.0.11.0.9-1.el7_9.x86_64/lib/server/libjvm.so) ==18191== by 0x676910C: jni_invoke_static(JNIEnv_*, JavaValue*, _jobject*, JNICallType, _jmethodID*, JNI_ArgumentPusher*, Thread*) [clone .isra.165] (in /usr/lib/jvm/java-11-openjdk-11.0.11.0.9-1.el7_9.x86_64/lib/server/libjvm.so) ==18191== by 0x676CF92: jni_CallStaticVoidMethod (in /usr/lib/jvm/java-11-openjdk-11.0.11.0.9-1.el7_9.x86_64/lib/server/libjvm.so) ==18191== by 0x52723DE: JavaMain (in /usr/lib/jvm/java-11-openjdk-11.0.11.0.9-1.el7_9.x86_64/lib/jli/libjli.so) ==18191== by 0x5058E24: start_thread (pthread_create.c:308) ==18191== by 0x577A34C: clone (clone.S:113) I don't find a matching destroy for some of the cairo surface created in: ImageList.convertSurface()
The reproduction snippet is mostly this: http://www.java2s.com/Tutorial/Java/0280__SWT/AddImagetoCTabFolder.htm If I run this snippet, already I see a memory leak reported by valgrind/jemalloc.
There is a call in ImageList.set(), to "Cairo.cairo_surface_reference(surface)" for the result of "ImageList.convertSurface(Image)". If I remove this call, neither jemalloc nor valgrind report the memory leak. I think I found the destroy call I thought was missing, its in "Image.getImageDataAtCurrentZoom()". The documentation of cairo_surface_reference() is found e.g. here: https://www.cairographics.org/manual/cairo-cairo-surface-t.html#cairo-surface-reference > Increases the reference count on surface by one. > This prevents surface from being destroyed until > a matching call to cairo_surface_destroy() is made. > > Use cairo_surface_get_reference_count() to get the number > of references to a cairo_surface_t. Paul, unfortunately I'm not familiar with cairo. The call is coming from bug 569691, with this commit: https://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=b84f0060d2d31f1cd93acbbd184953522870c7d2 See in particular: https://git.eclipse.org/c/platform/eclipse.platform.swt.git/diff/bundles/org.eclipse.swt/Eclipse%20SWT/gtk/org/eclipse/swt/internal/ImageList.java?id=b84f0060d2d31f1cd93acbbd184953522870c7d2 Does the reference call mean the surface must be destroyed twice to actually be freed? What is the exact purpose of the reference call?
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/180735
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/180783
(In reply to Simeon Andreev from comment #2) > There is a call in ImageList.set(), to > "Cairo.cairo_surface_reference(surface)" for the result of > "ImageList.convertSurface(Image)". If I remove this call, neither jemalloc > nor valgrind report the memory leak. > > I think I found the destroy call I thought was missing, its in > "Image.getImageDataAtCurrentZoom()". > > The documentation of cairo_surface_reference() is found e.g. here: > https://www.cairographics.org/manual/cairo-cairo-surface-t.html#cairo- > surface-reference > > > Increases the reference count on surface by one. > > This prevents surface from being destroyed until > > a matching call to cairo_surface_destroy() is made. > > > > Use cairo_surface_get_reference_count() to get the number > > of references to a cairo_surface_t. > > Paul, unfortunately I'm not familiar with cairo. The call is coming from bug > 569691, with this commit: > https://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=b84f0060d2d31f1cd93acbbd184953522870c7d2 > > See in particular: > https://git.eclipse.org/c/platform/eclipse.platform.swt.git/diff/bundles/org. > eclipse.swt/Eclipse%20SWT/gtk/org/eclipse/swt/internal/ImageList. > java?id=b84f0060d2d31f1cd93acbbd184953522870c7d2 > > Does the reference call mean the surface must be destroyed twice to actually > be freed? What is the exact purpose of the reference call? Here is the purpose of ImageList.convertSurface(Image) In eclipse we create a blank image using gdk_window_create_similar_surface() this creates surface of type XLIB but for our image handling we need a surface of type IMAGE. The above method converts any surface of type other than IMAGE to type IMAGE. We can replace gdk_window_create_similar_surface() with cairo_image_surface_create(). But I noticed regressions on Ubuntu. There are couple problems I noticed, is surface gets created with 0,0 dimensions and scaling for different scale factors doesn't work properly. These are not found on RHEL based distributions. Hope this helps
Also ImageList.convertSurface should not leak. In Image object we hold image surface in field name "surface". Earlier we used pixbuf for gtk < 2.18 any remanats of pixbuf usage needs to eliminated. the purpose of this method is already explained in comment 5. When we call convertSurface(Image), we already have a cairo surface in the Image object. That needs to be converted to proper format and assigned to image.surface field. I went through the convertSurface method and I did not find a destroy call for the old contents of Image.surface. Adding that should resolve leak in convertSurface.
Hi all, any movement here? Best regards, Simeon
I understand that my concerns for removing code in 'TableItem.setImage()' were supported by others and therefore I'm awaiting for this issue to be resolved before I could review the patch.
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/185455
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/185455 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=6296a3acfc6be3c9f6810d088c69b46a12b422e7
Simeon, please don't forget ot update target/resolution status next time. Please verify the leak with our build (as soon as we've managed to have SDK with the fix for bug 576093).
