Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Memory leak fixes #1112

Merged
merged 5 commits into from
Jul 23, 2020
Merged

Memory leak fixes #1112

merged 5 commits into from
Jul 23, 2020

Conversation

tbzatek
Copy link
Contributor

@tbzatek tbzatek commented Jul 16, 2020

A bunch of tiny fixes found by valgrind.

Please test if this fixes #1085

@tbzatek
panel-applet-frame: Fix leaking bg_pattern
eefa766 
Also acquire it only when needed.

This fixes a leak for me when switching workspaces back and forth.
@tbzatek
panel-frame: Fix leaking background-color
0a495f1
@tbzatek
panel-profile: Fix leaking list elements
55cd256 
The mate_gsettings_strv_to_gslist() call will dup all the strings
and removing duplicates should free them.
@tbzatek
panel-util: Fix leaking root GFile
7485c0e
@tbzatek
panel-toplevel: Refactor panel_toplevel_update_hide_buttons_size()
e155e84 
No real leak here except of the unnecessary gtk_css_provider_new() call,
refactored for readability and to conform to the code style.
rbuj
rbuj approved these changes Jul 16, 2020
View reviewed changes
Copy link
Contributor

@rbuj rbuj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test: Use mate-panel with custom bakground color, autohide buttons. Run the commands below in MATE Terminal.

$ CFLAGS="-fsanitize=leak -fno-omit-frame-pointer -g -ggdb3 -O0" LDFLAGS="-fsanitize=leak -ggdb3" ./autogen.sh --prefix=/usr && make && sudo make install
$ mate-panel --replace

Master

=================================================================
==85992==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 2560 byte(s) in 4 object(s) allocated from:
    #0 0x7fe6bc778e76 in realloc (/lib64/liblsan.so.0+0x10e76)
    #1 0x7fe6baee1830 in FcPatternObjectInsertElt /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fcpat.c:536

Direct leak of 1920 byte(s) in 15 object(s) allocated from:
    #0 0x7fe6bc778af1 in malloc (/lib64/liblsan.so.0+0x10af1)
    #1 0x7fe6bbbc0f1b in INT_cairo_pattern_create_for_surface /usr/src/debug/cairo-1.16.0-8.fc32.x86_64/src/cairo-pattern.c:741
    #2 0x7fe6bbbc0f1b in INT_cairo_pattern_create_for_surface /usr/src/debug/cairo-1.16.0-8.fc32.x86_64/src/cairo-pattern.c:726

Direct leak of 1 byte(s) in 1 object(s) allocated from:
    #0 0x7fe6bc778af1 in malloc (/lib64/liblsan.so.0+0x10af1)
    #1 0x7fe6bb7f7978 in g_malloc (/lib64/libglib-2.0.so.0+0x58978)

Indirect leak of 12672 byte(s) in 12 object(s) allocated from:
    #0 0x7fe6bc778842  (/lib64/liblsan.so.0+0x10842)
    #1 0x7fe6bacb4c29 in create_bits ../pixman/pixman-bits-image.c:1269
    #2 0x7fe6bacb4c29 in _pixman_bits_image_init ../pixman/pixman-bits-image.c:1292

Indirect leak of 8480 byte(s) in 265 object(s) allocated from:
    #0 0x7fe6bc778af1 in malloc (/lib64/liblsan.so.0+0x10af1)
    #1 0x7fe6baeccd6f in FcConfigValues /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fccfg.c:1569

Indirect leak of 5096 byte(s) in 13 object(s) allocated from:
    #0 0x7fe6bc778af1 in malloc (/lib64/liblsan.so.0+0x10af1)
    #1 0x7fe6bbba7878 in _cairo_image_surface_create_for_pixman_image /usr/src/debug/cairo-1.16.0-8.fc32.x86_64/src/cairo-image-surface.c:185

Indirect leak of 4154 byte(s) in 309 object(s) allocated from:
    #0 0x7fe6bc778af1 in malloc (/lib64/liblsan.so.0+0x10af1)
    #1 0x7fe6bb39434e in __GI___strdup /usr/src/debug/glibc-2.31-17-gab029a2801/string/strdup.c:42

Indirect leak of 3432 byte(s) in 13 object(s) allocated from:
    #0 0x7fe6bc778af1 in malloc (/lib64/liblsan.so.0+0x10af1)
    #1 0x7fe6baceb6bf in _pixman_image_allocate ../pixman/pixman-image.c:184

Indirect leak of 2400 byte(s) in 75 object(s) allocated from:
    #0 0x7fe6bc778842  (/lib64/liblsan.so.0+0x10842)
    #1 0x7fe6baee1ffe in FcValueListCreate /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fcpat.c:136
    #2 0x7fe6baee1ffe in FcPatternObjectAddWithBinding /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fcpat.c:707

Indirect leak of 1536 byte(s) in 48 object(s) allocated from:
    #0 0x7fe6bc778842  (/lib64/liblsan.so.0+0x10842)
    #1 0x7fe6baee128f in FcValueListCreate /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fcpat.c:136
    #2 0x7fe6baee128f in FcValueListDuplicate /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fcpat.c:230

Indirect leak of 384 byte(s) in 12 object(s) allocated from:
    #0 0x7fe6bc778842  (/lib64/liblsan.so.0+0x10842)
    #1 0x7fe6baee112f in FcValueListCreate /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fcpat.c:136
    #2 0x7fe6baee112f in FcValueListPrepend /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fcpat.c:180

Indirect leak of 96 byte(s) in 2 object(s) allocated from:
    #0 0x7fe6bc778af1 in malloc (/lib64/liblsan.so.0+0x10af1)
    #1 0x7fe6baedb25d in IA__FcLangSetCreate /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fclang.c:476
    #2 0x7fe6baedb25d in IA__FcLangSetCopy /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fclang.c:504

Indirect leak of 64 byte(s) in 2 object(s) allocated from:
    #0 0x7fe6bc778842  (/lib64/liblsan.so.0+0x10842)
    #1 0x7fe6baee11af in FcValueListCreate /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fcpat.c:136
    #2 0x7fe6baee11af in FcValueListAppend /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fcpat.c:200

SUMMARY: LeakSanitizer: 42795 byte(s) leaked in 771 allocation(s).

PR

=================================================================
==89920==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 2560 byte(s) in 4 object(s) allocated from:
    #0 0x7fd6f07c8e76 in realloc (/lib64/liblsan.so.0+0x10e76)
    #1 0x7fd6eef31830 in FcPatternObjectInsertElt /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fcpat.c:536

Direct leak of 1 byte(s) in 1 object(s) allocated from:
    #0 0x7fd6f07c8af1 in malloc (/lib64/liblsan.so.0+0x10af1)
    #1 0x7fd6ef847978 in g_malloc (/lib64/libglib-2.0.so.0+0x58978)

Indirect leak of 8480 byte(s) in 265 object(s) allocated from:
    #0 0x7fd6f07c8af1 in malloc (/lib64/liblsan.so.0+0x10af1)
    #1 0x7fd6eef1cd6f in FcConfigValues /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fccfg.c:1569

Indirect leak of 4154 byte(s) in 309 object(s) allocated from:
    #0 0x7fd6f07c8af1 in malloc (/lib64/liblsan.so.0+0x10af1)
    #1 0x7fd6ef3e434e in __GI___strdup /usr/src/debug/glibc-2.31-17-gab029a2801/string/strdup.c:42

Indirect leak of 2400 byte(s) in 75 object(s) allocated from:
    #0 0x7fd6f07c8842  (/lib64/liblsan.so.0+0x10842)
    #1 0x7fd6eef31ffe in FcValueListCreate /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fcpat.c:136
    #2 0x7fd6eef31ffe in FcPatternObjectAddWithBinding /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fcpat.c:707

Indirect leak of 1536 byte(s) in 48 object(s) allocated from:
    #0 0x7fd6f07c8842  (/lib64/liblsan.so.0+0x10842)
    #1 0x7fd6eef3128f in FcValueListCreate /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fcpat.c:136
    #2 0x7fd6eef3128f in FcValueListDuplicate /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fcpat.c:230

Indirect leak of 384 byte(s) in 12 object(s) allocated from:
    #0 0x7fd6f07c8842  (/lib64/liblsan.so.0+0x10842)
    #1 0x7fd6eef3112f in FcValueListCreate /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fcpat.c:136
    #2 0x7fd6eef3112f in FcValueListPrepend /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fcpat.c:180

Indirect leak of 96 byte(s) in 2 object(s) allocated from:
    #0 0x7fd6f07c8af1 in malloc (/lib64/liblsan.so.0+0x10af1)
    #1 0x7fd6eef2b25d in IA__FcLangSetCreate /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fclang.c:476
    #2 0x7fd6eef2b25d in IA__FcLangSetCopy /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fclang.c:504

Indirect leak of 64 byte(s) in 2 object(s) allocated from:
    #0 0x7fd6f07c8842  (/lib64/liblsan.so.0+0x10842)
    #1 0x7fd6eef311af in FcValueListCreate /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fcpat.c:136
    #2 0x7fd6eef311af in FcValueListAppend /usr/src/debug/fontconfig-2.13.92-9.fc32.x86_64/src/fcpat.c:200

SUMMARY: LeakSanitizer: 19675 byte(s) leaked in 718 allocation(s).

@rbuj rbuj requested a review from a team July 16, 2020 20:45
lukefromdc
lukefromdc approved these changes Jul 17, 2020
View reviewed changes
Copy link
Member

@lukefromdc lukefromdc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builds fine, though I did not have time to check a build from scratch for warnings against a new build from master by comparing text. Runs fine, didn't get anything appearing to relate to these changes in xsession-errors. I DO get a warning about reregistering indicators but since they may not have been killed with the previous panel instance (some of their code is from the applications creating them) that does not surprise me. System, color, and image backgrounds all worked same as before, at least with my UbuntuStudio_Legacy theme.

Codewise I see we now limit some of the cairo work to the custom BG cases of image and color with alpha cases, thus avoiding that code entirely with system and solid color backgrounds by reducing the scope of those entire blocks. Tests with my theme (hide buttons borderless) and adwaita-dark(hide buttons get a border) showed the same appearance of panel hide buttons so your removal in panel_toplevel_update_hide_buttons_size of the code to create a new cssprovider and call gtk_css_provider_get_named without having created a new provider seems to work fine though I don't know enough about cssproviders to vouch for that change. Still, that worked same as master in both the themes I tested. Also, I see this catches several cases of something that needed to be freed not being freed.

In testing mate-panel (w wncklet, clock, fish, and tray built in-process) after these changes we do of course still have other memory leaks (as your tests show). There are a LOT of them, and they will simply have to be found and corked up one at a time with more PR's like this one. I found evidence of two of them though not their locations in the code:

Window previews (thumbnails shown on hover) appear to be leaked,as RAM use goes up when one is shown and that RAM does not appear to be released until another thumbnail is shown instead. First few times opening the calender will jump RAM use about 6MB, though about 1MB (w my theme etc) is released when it is closed.

As I said, those two are NOT issues for this PR, which closes other leaks. I was not able to run your tests (time limited) but this didn't create any new problems on this end with Debian Unstable.

@lukefromdc
Copy link
Member

Note that memory use still rises with time of use, though as I said there are a LOT of other memory leaks in the panel. Still, this is a start

@lukefromdc lukefromdc requested a review from a team July 17, 2020 07:00
@lukefromdc
Copy link
Member

Called core team for another review if anyone gets the time. Should I merge this if nobody does?

@tbzatek
Copy link
Contributor Author

tbzatek commented Jul 17, 2020

Thanks for testing, this is impressive. I'll keep opening separate pull requests for future fixes (if any).

After nearly 24h idle I don't see any memory consumption increase, mate-panel stays at 40MB RSS with out-of-process applets sitting around 35 MB each. wnck, netspeed, notification-area and clock applets as processes. Previously at this setup I got several GB resident memory size after couple of days of regular use.

That said there might be other leaks in features I don't regularly use, let's see if I can reproduce some more.

Also many of the indirect leaks are singletons sitting there for the rest of the application lifecycle, such as the font or icon cache, theming, etc.

@raveit65
Copy link
Member

The leaks are dramatic when compile in-process, see bug report.

@tbzatek tbzatek mentioned this pull request Jul 22, 2020
Merged
@lukefromdc
Copy link
Member

Is this ready to merge or is this expected to be part of another PR later?

@tbzatek
Copy link
Contributor Author

tbzatek commented Jul 23, 2020

Is this ready to merge or is this expected to be part of another PR later?

You can merge this one. I tend to open separate pull requests for easier reviewing and to get fixes in in batches. If a single big pull request works for you better, I can rebase #1113 for example.

raveit65
raveit65 approved these changes Jul 23, 2020
View reviewed changes
Copy link
Member

@raveit65 raveit65 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Runs fine when applets-build-in process.

@raveit65 raveit65 merged commit bb299f9 into mate-desktop:master Jul 23, 2020
@raveit65
Copy link
Member

And cherry-picked to stable 1.24 branch.
Thanks for this support by you and RedHat :)

@raveit65
Copy link
Member

By the way,
greatest memory consumption was reported here https://bugzilla.redhat.com/show_bug.cgi?id=1830885
Build-in-process and reporter use wine and amule notification-area applet and did run his box several days.
This is something which i can't /don't want to test.
I switched back to build applets out-of-process to close this rhbz report.
But this blocks wayland support.

@tbzatek
Copy link
Contributor Author

tbzatek commented Jul 23, 2020

And cherry-picked to stable 1.24 branch.
Thanks for this support by you and RedHat :)

Thanks! For the record, this is my private initiative and I can't afford investing much time in this, just using my single (company) GitHub account and debugging on-the-go while doing my day job (spoiler: https://github.com/storaged-project/udisks/).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raveit65 raveit65 raveit65 approved these changes

@rbuj rbuj rbuj approved these changes

@lukefromdc lukefromdc lukefromdc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

high memory usage - mate-panel
4 participants
@tbzatek @lukefromdc @raveit65 @rbuj