After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 788643 - Some memory/performance improvements
Some memory/performance improvements
Status: RESOLVED FIXED
Product: pango
Classification: Platform
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: pango-maint
pango-maint
Depends on:
Blocks:
 
 
Reported: 2017-10-07 17:15 UTC by Timm Bäder
Modified: 2017-10-27 21:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
testiter: Don't leak PangoFontDescriptions (755 bytes, patch)
2017-10-07 17:15 UTC, Timm Bäder
accepted-commit_now Details | Review
[PATCH 2/4] layout: Move PangoLayouIter struct to private header (5.31 KB, patch)
2017-10-07 17:15 UTC, Timm Bäder
accepted-commit_now Details | Review
layout/renderer: Don't heap-allocate short lived layout iterators (7.39 KB, patch)
2017-10-07 17:15 UTC, Timm Bäder
accepted-commit_now Details | Review
PangoLayoutIter: Allocate an array of Extents instead of a linked list (8.61 KB, patch)
2017-10-07 17:16 UTC, Timm Bäder
accepted-commit_now Details | Review
PangoLayout: Optimize pango_layout_get_baseline (1.48 KB, patch)
2017-10-14 06:51 UTC, Timm Bäder
none Details | Review

Description Timm Bäder 2017-10-07 17:15:01 UTC
Created attachment 361099 [details] [review]
testiter: Don't leak PangoFontDescriptions

When drawing lots of text (or textnodes rather) in gtk+, these things show up in profiles, especially the use of heap-allocated PangoLayoutIter instances that get free'd at the end of the function anyway. Attached are a few patches, as well as one that fixes a memory leak in tests/testiter.
Comment 1 Timm Bäder 2017-10-07 17:15:24 UTC
Created attachment 361100 [details] [review]
[PATCH 2/4] layout: Move PangoLayouIter struct to private header
Comment 2 Timm Bäder 2017-10-07 17:15:48 UTC
Created attachment 361101 [details] [review]
layout/renderer: Don't heap-allocate short lived layout iterators
Comment 3 Timm Bäder 2017-10-07 17:16:21 UTC
Created attachment 361102 [details] [review]
PangoLayoutIter: Allocate an array of Extents instead of  a linked list
Comment 4 Matthias Clasen 2017-10-09 18:06:08 UTC
Review of attachment 361099 [details] [review]:

sure
Comment 5 Matthias Clasen 2017-10-09 18:12:52 UTC
Review of attachment 361100 [details] [review]:

looks fine to me
Comment 6 Matthias Clasen 2017-10-09 18:14:21 UTC
Review of attachment 361101 [details] [review]:

ok, if this shows up on profiles...
Comment 7 Matthias Clasen 2017-10-09 18:18:51 UTC
Review of attachment 361102 [details] [review]:

ok, I guess
Comment 8 Behdad Esfahbod 2017-10-11 10:26:26 UTC
lgtm.  I'm curious, how did you arrive on these?  Where they showing on profiles?  What else was showing?  Thanks.
Comment 9 Timm Bäder 2017-10-11 13:30:58 UTC
This particular example was the imagelist example from https://github.com/baedert/listbox-c after clicking on "scroll". It's just one GtkLabel per row and just ~10 rows, but they get snapshot every frame.

We spend around 3.1% in pango_renderer_draw_layout in this case: https://i.imgur.com/a6HSSJM.png after it's at around 2.8%: https://i.imgur.com/GHjHHKK.png 

One other thing that would be cool to have is a stack allocated PangoGlyphString so we can avoid the g_slice_alloc in pango_glyph_string_copy in gsk_text_node_new: https://i.imgur.com/oi5EmNn.png but that would require public API and you'd have to maintain ABI so maybe ~2.8% is good enough for now :)
Comment 10 Timm Bäder 2017-10-14 06:51:24 UTC
Created attachment 361569 [details] [review]
PangoLayout: Optimize pango_layout_get_baseline

This shows up in profiles when e.g. scrolling in the listbox sample from gtk4-demo.

This patch reduces the time taken in pango_layout_get_baseline inside get_layout_location from gtklabel.c from ~0.64% to ~29%.
Comment 11 Timm Bäder 2017-10-14 07:15:46 UTC
Actually, I guess that patch removes a pango_layout_check_lines call from _get_baseline.
Comment 12 Timm Bäder 2017-10-14 13:12:04 UTC
Ignore me, pango_layout_get_extents_internal calls it as well.