GNOME Bugzilla – Bug 349695
some leak and ref counting fixes
Last modified: 2021-05-25 12:45:43 UTC
% export G_DEBUG=gc-friendly % export G_SLICE=always-malloc % valgrind --tool=memcheck --leak-check=full --show-reachable=yes ./util/.libs/gnome-menu-spec-test ==8636== LEAK SUMMARY: ==8636== definitely lost: 0 bytes in 0 blocks. ==8636== possibly lost: 0 bytes in 0 blocks. ==8636== still reachable: 111,058 bytes in 3,268 blocks. ==8636== suppressed: 0 bytes in 0 blocks. This is a problem for long running processes like gnome-screensaver. gnome-screensaver uses gnome-menus to find screensaver themes. We worked around this for 2.14 by never unreffed the GMenuTree. Now in HEAD g-s we try to unref the GMenuTree when it isn't needed.
Created attachment 70085 [details] [review] patch This patch does a few things: * Makes reference counting atomic (useful for future thread-safeness) * Destroys the caches when not in use * Reduces duplicate code in entry-directories.c * Simplifies the ref counting of CachedDir * Uses G_DIR_SEPARATOR just for fun. * Fixes a bug in the gmenu_tree_entry_get_launch_in_terminal g_return value Open questions: * Need to make sure that we haven't reintroduced the bug solved by: 2005-08-18 Mark McLoughlin <mark@skynet.ie> Obfuscate this code some more. Basic issue is that if an EntryDirectory has a subdir which is also an EntryDirectory and the subdir gets deleted, then the CachedDir for the subdir gets freed leaving us with a dangling reference in the EntryDirectory. * Do we waste memory by no longer using a bit-field for ref_count? * Can we drop char *basename from DesktopEntry and just use g_path_get_basename(entry->path) to save memory? Does this cause more fragmentation?
Created attachment 70087 [details] valgrind plot (HEAD) Here is a valgrind massif plot of gnome-screensaver using gnome-menus from HEAD. The screensaver is locked three times and then exitted cleanly.
Created attachment 70088 [details] valgrind plot (HEAD+patch) Same scenario but with gnome-menus patched.
Looks promising. Can we get this reviewed please?
It'd be better to make separate patches for the various things. This will help with reviewing. Can you make one patch for atomic reference count (and other ref counting things), one patch for "Destroys the caches when not in use" and one patch for "Reduces duplicate code in entry-directories.c"?
Is this still valid?
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new enhancement request ticket at https://gitlab.gnome.org/GNOME/gnome-menus/-/issues/ Thank you for your understanding and your help.