Community
Participate
Working Groups
OS: F19 Glib( 2.36) How to reproduce: Run ControlExample and you will see Stack trace in the console: (SWT:2591): GLib-GObject-WARNING **: gclosure.c:697: unable to remove uninstalled invalidation notifier: 0x7fa5df83aae0 (0x7fa615625170) (SWT:2591): GLib-GObject-WARNING **: gclosure.c:697: unable to remove uninstalled invalidation notifier: 0x7fa5df83aae0 (0x7fa615625170) (SWT:2591): GLib-GObject-WARNING **: gclosure.c:697: unable to remove uninstalled invalidation notifier: 0x7fa5df83aae0 (0x7fa615625170) ......
(In reply to comment #0) > OS: F19 Glib( 2.36) > > How to reproduce: > > Run ControlExample and you will see Stack trace in the console: > > (SWT:2591): GLib-GObject-WARNING **: gclosure.c:697: unable to remove > uninstalled invalidation notifier: 0x7fa5df83aae0 (0x7fa615625170) > > (SWT:2591): GLib-GObject-WARNING **: gclosure.c:697: unable to remove > uninstalled invalidation notifier: 0x7fa5df83aae0 (0x7fa615625170) > > (SWT:2591): GLib-GObject-WARNING **: gclosure.c:697: unable to remove > uninstalled invalidation notifier: 0x7fa5df83aae0 (0x7fa615625170) > ...... After a discussion with GTK+ developer here is the extra information I've got: The most probable coz of the problem is that invalidation handler limit is 256. ControlExample uses more than that, thus producing warnings and critical errors on connect as well as on close (Control.hookEvents() is the place to look). From my observation: It starts to exceed the limit when Coolbar is loaded. I have opened a bug in Gnome so GTK+ devs can take a look at that and maybe fix it or suggest some solution. Maybe the limit can be increased, or we need to figure out other solution for this. If anyone interested, the bug I have opened is here: https://bugzilla.gnome.org/show_bug.cgi?id=699953
(In reply to comment #1) > (In reply to comment #0) > > OS: F19 Glib( 2.36) > > > > How to reproduce: > > > > Run ControlExample and you will see Stack trace in the console: > > > > (SWT:2591): GLib-GObject-WARNING **: gclosure.c:697: unable to remove > > uninstalled invalidation notifier: 0x7fa5df83aae0 (0x7fa615625170) > > > > (SWT:2591): GLib-GObject-WARNING **: gclosure.c:697: unable to remove > > uninstalled invalidation notifier: 0x7fa5df83aae0 (0x7fa615625170) > > > > (SWT:2591): GLib-GObject-WARNING **: gclosure.c:697: unable to remove > > uninstalled invalidation notifier: 0x7fa5df83aae0 (0x7fa615625170) > > ...... > > After a discussion with GTK+ developer here is the extra information I've > got: > > The most probable coz of the problem is that invalidation handler limit is > 256. ControlExample uses more than that, thus producing warnings and > critical errors on connect as well as on close (Control.hookEvents() is the > place to look). > > From my observation: It starts to exceed the limit when Coolbar is loaded. > > I have opened a bug in Gnome so GTK+ devs can take a look at that and maybe > fix it or suggest some solution. > > Maybe the limit can be increased, or we need to figure out other solution > for this. > > If anyone interested, the bug I have opened is here: > > https://bugzilla.gnome.org/show_bug.cgi?id=699953 So further investigation lead to conclusion that the problem lies in limited amount of GClosures. As we are reusing closures now, and that the amount of them is limited to 256, every time GClosures exceed amount 256 it starts to bring in the warning: GLib-GObject-CRITICAL **: g_closure_add_invalidate_notifier: assertion `closure->n_inotifiers < CLOSURE_MAX_N_INOTIFIERS' failed every time the closure is used. One of the possible solutions could be to create a new closure every time: For example we could substitute: OS.g_signal_connect_closure_by_id (focusHandle, display.signalIds [POPUP_MENU], 0, display.closures [POPUP_MENU], false); by OS.g_signal_connect_closure_by_id (focusHandle, display.signalIds [POPUP_MENU], 0, OS.g_cclosure_new (display.windowProc2, Widget.POPUP_MENU, 0), false); Thus we won't reuse the same closure and won't be having a warning. I would love to hear expert opinion on this. If you run F19 right now, you would see that this is an urgent issue.
(In reply to comment #2) > (In reply to comment #1) > > (In reply to comment #0) > > > OS: F19 Glib( 2.36) > > > > > > How to reproduce: > > > > > > Run ControlExample and you will see Stack trace in the console: > > > > > > (SWT:2591): GLib-GObject-WARNING **: gclosure.c:697: unable to remove > > > uninstalled invalidation notifier: 0x7fa5df83aae0 (0x7fa615625170) > > > > > > (SWT:2591): GLib-GObject-WARNING **: gclosure.c:697: unable to remove > > > uninstalled invalidation notifier: 0x7fa5df83aae0 (0x7fa615625170) > > > > > > (SWT:2591): GLib-GObject-WARNING **: gclosure.c:697: unable to remove > > > uninstalled invalidation notifier: 0x7fa5df83aae0 (0x7fa615625170) > > > ...... > > > > After a discussion with GTK+ developer here is the extra information I've > > got: > > > > The most probable coz of the problem is that invalidation handler limit is > > 256. ControlExample uses more than that, thus producing warnings and > > critical errors on connect as well as on close (Control.hookEvents() is the > > place to look). > > > > From my observation: It starts to exceed the limit when Coolbar is loaded. > > > > I have opened a bug in Gnome so GTK+ devs can take a look at that and maybe > > fix it or suggest some solution. > > > > Maybe the limit can be increased, or we need to figure out other solution > > for this. > > > > If anyone interested, the bug I have opened is here: > > > > https://bugzilla.gnome.org/show_bug.cgi?id=699953 > > So further investigation lead to conclusion that the problem lies in limited > amount of GClosures. As we are reusing closures now, and that the amount of > them is limited to 256, every time GClosures exceed amount 256 it starts to > bring in the warning: > > GLib-GObject-CRITICAL **: g_closure_add_invalidate_notifier: assertion > `closure->n_inotifiers < CLOSURE_MAX_N_INOTIFIERS' failed > > every time the closure is used. > > One of the possible solutions could be to create a new closure every time: > > > For example we could substitute: > > OS.g_signal_connect_closure_by_id (focusHandle, display.signalIds > [POPUP_MENU], 0, display.closures [POPUP_MENU], false); > > by > > OS.g_signal_connect_closure_by_id (focusHandle, display.signalIds > [POPUP_MENU], 0, OS.g_cclosure_new (display.windowProc2, Widget.POPUP_MENU, > 0), false); > > Thus we won't reuse the same closure and won't be having a warning. > > I would love to hear expert opinion on this. If you run F19 right now, you > would see that this is an urgent issue. One of the possible solutions could be to reuse GClosure until it reaches the maximum limit, and than start to create new closures. I have created a method that does that. The method looks looks like this : ------------------------------------------------- Map closure_map = new HashMap (); // global Integer n_notifier = new Integer(0);// global long /*int*/ getClosure(int closure_id, long windowProcNum, int closure) { Integer closureId = new Integer(closure_id); // check if closure_id is already registered if (closure_map.containsKey(closureId)){ // increment it's value n_notifier = Integer.valueOf(n_notifier.intValue() + 1); } // add closure id and it's current n_notifier value closure_map.put(closureId, n_notifier); if (closure == 0 || n_notifier.intValue() > 255) { // create new closure display.closures[closure_id] = OS.g_cclosure_new (windowProcNum, closure, 0); } return display.closures [closure_id]; } -------------------------------------------------- It can be called liked this: OS.g_signal_connect_closure_by_id (focusHandle, display.signalIds [POPUP_MENU], 0, getClosure(POPUP_MENU, display.windowProc2, Widget.POPUP_MENU), false); If this solutions looks acceptable, I'll go ahead and create a patch, but I need to have committers opinion on it.
Here is a patch based on the solution I have proposed earlier: http://fedorapeople.org/cgit/aspektor/public_git/eclipse.platform.swt.git/commit/?h=gsignal What it does: It counts how many closures left for each signal, and if number of closures reached maximum, it creates new closures. This patch gets read of all gSignal errors in ControlExample when running GTK+ 3.8 in Fedora 19 alpha. Please take a look and let me know what do you think. Thanks!
I am seeing very similar problems with a standalone SWT app (SWT version 4.2.1) running on Ubuntu 13.04 (Raring Ringtail).
Created attachment 234174 [details] Simple test program that shows the problem. I've attached a small test program. My system is running Ubuntu 13.04 (Linux 3.8.0-27-generic #40-Ubuntu SMP Tue Jul 9 00:17:05 UTC 2013 x86_64 x86_64 x86_64 GNU/Linux, version 2.36 of GLib). The test program allows one to interactively create large numbers of widgets. On my system, the error messages appear after 243 widgets are created. My experience with another system suggests that the messages are not harmless. That system crashes shortly after the messages appear. I didn't include a copy of the SWT libraries, so you'll need to provide one to build/run the app.
I've done a bit more investigation on this. I made a similar change to the one listed above to a slightly older version of SWT that is being used by the app I'm working on. The basic change was to replace the references to "display.closures[signalId]" to a method call "display.allocClosure(signalId)". I then tried timing two sets of SWT visual test runs ("quick" and "smoke") under different conditions: new every time: every call to allocClosure(signalId) returns a new closure. cached: closures are kept for 200 calls then a new one is allocated. never released: closures are kept forever (current behavior). This condition generated literally thousands of "GLib-GObject-WARNING" and "GLib-GObject-CRITICAL" messages. Here are the times (three runs each, all times in seconds): new every time, quick: 26.7, 27.2, 26.7 cached, quick: 26.5, 26.8, 26.7 never released: 53.4, 53.3, 53.0 new every time, smoke: 83.7, 84.3, 83.7 cached, smoke: 84.0, 84.5, 83.9 never released: didn't bother. The "never released" times are probably slow because of the thousands and thousands of error messages being printed. These SWT runs have no delays and do minimal graphics, so they create and destroy a LOT of controls. To me, the times strongly suggest that caching closures is not worth the trouble. For reference, here's what the code in Display.java looks like: private static final int MAX_CLOSURES = 200; private long /*int*/ closures[] = new long /*int*/ [Widget.LAST_SIGNAL]; private int closureCount[] = new int[Widget.LAST_SIGNAL]; long /*int*/ allocClosure(int signalId) { if (closures[signalId] > 0 && closureCount[signalId] < MAX_CLOSURES) { closureCount[signalId] += 1; return closures[signalId]; } else { if (closures[signalId] != 0) { OS.g_closure_unref(closures[signalId]); } long /*int*/ closure = createClosure(signalId); OS.g_closure_ref(closure); closures[signalId] = closure; closureCount[signalId] = 1; return closure; } } long /*int*/ createClosure(int signalId) { switch (signalId) { // windowProc2 case Widget.ACTIVATE: return OS.g_cclosure_new (windowProc2, Widget.ACTIVATE, 0); case Widget.ACTIVATE_INVERSE: return OS.g_cclosure_new (windowProc2, Widget.ACTIVATE_INVERSE, 0);
I should add that in the "new every time" case I simply called "createClosure" directly rather than constantly add and remove references. The code for that case looked like: long /*int*/ allocClosure(int signalId) { switch (signalId) { // windowProc2 case Widget.ACTIVATE: return OS.g_cclosure_new (windowProc2, Widget.ACTIVATE, 0); case Widget.ACTIVATE_INVERSE: return OS.g_cclosure_new (windowProc2, Widget.ACTIVATE_INVERSE, 0); case Widget.CHANGED: return OS.g_cclosure_new (windowProc2, Widget.CHANGED, 0); (In reply to comment #7) > I've done a bit more investigation on this. I made a similar change to the > one listed above to a slightly older version of SWT that is being used by > the app I'm working on. The basic change was to replace the references to ...
Hello everyone, I just want to add that this also is a problem with SWT 4.2.2 and SWT 4.3 on Linux 64 bit with glib 2.36. When I run the test suite I see LOTS of those errors: (SWT:11757): GLib-GObject-CRITICAL **: g_closure_add_invalidate_notifier: assertion `closure->n_inotifiers < CLOSURE_MAX_N_INOTIFIERS' failed The test suite passes either way. A timely fix would be highly appreciated! Also if someone knows how to suppress these warnings for now... ;-) Cheers and thanks a lot for your work on SWT! Tobi
I think this bug deserves higest priority as glib versions exposing this problem are more and more common and probably be majority for Luna release.
My personnel investigation also shown that caching is not worth it but I would like to hear Silenio on that.
I have not done any investigation, but I also agree that the caching of closures is not improving performance in any noticeable way. It might have helped 10 years ago when we changed all our g_signal_connect() and g_signal_connect_after() calls to use g_signal_connect_closure() and g_signal_connect_closure_by_id(). But, I do not think we should start leaking closures. Either we need to keep track of the closures we create and dispose them when they are not needed anymore or we should go back and use g_signal_connect/g_signal_connect_after instead. If we use g_signal_connect() instead, we do not have to worry about the live cycle of the closures. On the other hand, performance might be affected since we will start looking up signals by name in same cases (i.e all the ids in Display.signalIds). We should bench this approach and decide.
I guess we could just release the previous closure in the current patch. In Widget.getClosure() just before assigning a new one to display.closure[i]. BTW, we do not need a hash table to keep track of the closure connection count. It could just be a int array where the index is the signalId/closureId (i.e. Widget.BUTTON_PRESS_EVENT) and value is the connection count. Also, are the changes in Display necessary? Why the closures can not be released in the end/shutdown?
Alex, please could you review this patch? http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?h=silenio/bug407077&id=25d9f99bae3ba84644ed6a921ea164452ae5da9d
First test leads to crash. I don't think we can unref the closure when the count is reached as it might still be needed by some existing widget that was created before that. I'll investigate further when time permits.
(In reply to Alexander Kurtakov from comment #15) > First test leads to crash. I don't think we can unref the closure when the > count is reached as it might still be needed by some existing widget that > was created before that. > I'll investigate further when time permits. We should be able to unref it because g_signal_connect_closure() refs it. I will get Fedora 19 up running to be able to test this. https://git.gnome.org/browse/glib/tree/gobject/gsignal.c#n2297
This commit fixes the crash (comment#15) and the warnings. http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?h=silenio/bug407077&id=8ff78dc82c169eaba1d98c37e14a85f324dc0a98 I will cherry-pick to master.
Done.
Works perfect for me. Thanks Silenio.
... > But, I do not think we should start leaking closures. Either we need to keep > track of the closures we create and dispose them when they are not needed > anymore or we should go back and use g_signal_connect/g_signal_connect_after > instead. > > If we use g_signal_connect() instead, we do not have to worry about the live > cycle of the closures. On the other hand, performance might be affected > since we will start looking up signals by name in same cases (i.e all the > ids in Display.signalIds). We should bench this approach and decide. Thanks for working on this! The patch I mentioned earlier uses "g_cclosure_new" to allocate the closure and connects it with "g_signal_connect_closure". I belive that this does not create closures as, as far as I can tell, the closure is created with a reference count of zero. The count is incremented when the closure is connected, and then decremented when the associated GTK object is destroyed. The code that caches closures has to explicitly "ref" the closure when it's added to the cache, and then "unref" the closure when it's removed from the cache. Jim
Thank you! It works well now.
