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.

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

lvfntman: fix harfbuzz related memory leak on quit #534

Merged

Conversation

benoit-pierre
Copy link
Contributor

@benoit-pierre benoit-pierre commented Oct 11, 2023

This change is Reviewable

Comment on lines -2527 to +2539
if ( SVGGlyphsCollector_svg_funcs == NULL ) {
setup_SVGGlyphsCollector_svg_funcs();
}
setup_SVGGlyphsCollector_svg_funcs();
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for moving the check ==NULL inside the function (so, always calling this function that does its work only once) - rather that doing the check here and really calling this function once ?
(This is called for each text segment in SVGs.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's cleaner that way IMHO. I'm not concerned about performance (but I've made setup_SVGGlyphsCollector_svg_funcs static so the compiler can do its thing). (Note that I do benchmark regularly to avoid regressions).

Copy link
Contributor

Choose a reason for hiding this comment

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

It's cleaner that way IMHO

I don't feel the same: one has to go jump at the function to see it's a no-op on next calls.
And I don't trust compilers to intuite the logic I thought about but was lazy to write :)

@poire-z
Copy link
Contributor

poire-z commented Oct 11, 2023

"memory leak on quit" sounds like the best memory leaks :)
Why chase them ? Just to get them out of the way to catch the other ones? Or some other reason?

@benoit-pierre benoit-pierre force-pushed the pr/fix_harfbuzz_related_leak_on_quit branch from a2057ef to 75fcce7 Compare October 11, 2023 23:43
@NiLuJe
Copy link
Member

NiLuJe commented Oct 11, 2023

It makes Valgrind reports tedious to parse, making finding meaningful leaks that much more annoying ;).

@benoit-pierre
Copy link
Contributor Author

benoit-pierre commented Oct 11, 2023

"memory leak on quit" sounds like the best memory leaks :) Why chase them ? Just to get them out of the way to catch the other ones? Or some other reason?

Well, technically, leaks on dlclose.

It makes Valgrind reports tedious to parse, making finding meaningful leaks that much more annoying ;).

This.

I'm down to the expected 32 bytes (freetype memory object) in a lot of cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants