GNOME Bugzilla – Bug 135683
Cache glyphstring extents
Last modified: 2006-12-04 23:47:10 UTC
Here is an old patch I found on my disk. It caches the extents of a PangoGlyphString. I remember I found it to be an improvement in cases where the same layout is rendered many times.
Created attachment 24892 [details] [review] The patch
Note: I do intend to come up with a benchmark, but not until after 2.4 is out.
Created attachment 29028 [details] The benchmark Here is the benchmark. It consist of scrolling a textview containing the LGPL up and down two times. With patch I get ~19 seconds, without patch I get ~21 seconds.
Comment on attachment 24892 [details] [review] The patch I don't the extents can be cached on the glyphstring itself - modifying a glyphstring and getting the extents again is perfectly legitimate, and there is no way in the PangoGlyphString API to know about changes. I think we could do caching in PangoLayout - caching the extents per LayoutLine might be enough, or maybe we would want to cache the extents of each individual run.
Created attachment 38017 [details] [review] Cache extents in PangoLayoutLine This patch caches extents in PangoLayoutLine. It doesn't really help in the scrolling a text view case sadly, as PangoLayoutLines are created there on demand. It is used when redrawing constant labels such as GtkLabel's however. I'm investigating more about how to speed large runs, such as the benchmark in this bug. Further pointers appreciated. :)
Created attachment 38046 [details] [review] Cache PangoLayoutLine and PangoItem extents Attached patch caches in PangoGlyphItem and PangoLayoutLine.
Looks like good news. Owen, any advice on where to go from here?
Run my little benchmark before and after plugging this into Pango. The benchmark is here: http://primates.ximian.com/~federico/docs/file-chooser-profile/code/pango-layout-profile.c If I remember correctly, adding up the glyph extents was taking around 4% or 5% of the profile.
My original patch was intended to speed up rendering, not layouting, and it did achieve a ~10% better framerate on the scrolling text benchmark I attached, Layouting is _irrelevant_ to this bug - it is not the bottleneck here at all. However, my patch was rejected by Owen (and I am not going to argue about whether that was right or not). Ross's patch does caching on PangoGlyphItem and PangoLayoutLine as Owen suggested. As Ross notes, this does not have any effect on my original benchmark and I don't know any other benchmark or usecase that it would have an effect on. I would certainly not expect it to affect a layout benchmark where the extents are only used once. If anything it would slow it down a little. So, in summary: - the "bug" here is "scrolling is slow" - my fix for the bug was rejected - the suggestion for an alternative fix does not work because it doesn't fix the bug. So since it isn't really a bug in the doesn't-work sense, probably the right course of action is to resolve WONTFIX or NOTABUG and then possibly, if someone can think of something that doing caching on PangoLayoutLine would actually help, maybe open another bug for that. (Maybe PangoLayoutLines could be cached in the TextView or something).
So, what is taking up the time in the benchmark? Has anyone run Sysprof on this? Maybe you'll need to make N_ITERS larger than 2 to have it run for, say, 30 seconds. We can probably reassign this bug to GTK+ and re-title it as "Speed up GtkTextView scrolling" until we determine the real cause of the slowness.
Making this bug public (why was it marked private!?).
Created attachment 68629 [details] [review] patch Not as effective as Soeren's initial patch, but I committed this that uses a new function pango_glyph_string_get_width instead of pango_glyph_string_extents in the common case of no underline/strikethrough. get_width in turn doesn't have to call pango_font_get_glyph_extents at all and just sums geometry.width over all the glyphs in the glyph string.
Ross, you cannot change the PangoGlyphItem struct, as it's public and allocated by the user directly.
Created attachment 68630 [details] [review] more uses 2006-07-08 Behdad Esfahbod <behdad@gnome.org> Part of Bug 135683 – Cache glyphstring extents * pango/pango-layout.c (pango_layout_line_index_to_x), (pango_layout_line_x_to_index), (pango_layout_line_get_x_ranges): Use pango_glyph_string_get_width in most places in PangoLayout.
The patch contains a silly error. Fixed that, and also removed unused font parameter from pango_glyph_string_get_width(). 2006-07-08 Behdad Esfahbod <behdad@gnome.org> * pango/glyphstring.c (pango_glyph_string_get_width): * pango/pango-glyph.h: * pango/pango-layout.c (pango_layout_line_index_to_x), (pango_layout_line_x_to_index), (pango_layout_line_get_x_ranges): * pango/pango-renderer.c (pango_renderer_draw_layout_line): Fix silly error in implementation of pango_glyph_string_get_width(), and also remove unused font parameter from its signature.
Created attachment 77695 [details] [review] cache line extents Patch I committed. Caches line extents, but only if the user doesn't have a pointer to the line or any of its runs. If she does, caching is disabled. 2006-12-04 Behdad Esfahbod <behdad@gnome.org> Bug 135683 – Cache glyphstring extents * pango/pango-layout.c (pango_layout_get_lines), (pango_layout_get_line), (pango_layout_line_leaked), (pango_layout_line_get_extents), (pango_layout_line_new), (pango_layout_iter_get_run), (pango_layout_iter_get_line): Cache line extents. Line extents are cached only if the user doesn't have a pointer to the line or any of its runs. Functions that give away such pointers mark the line as "leak"ed. * pango/pango-layout.c (pango_layout_index_to_line_and_extents), (pango_layout_xy_to_index), (pango_layout_index_to_pos): Use _pango_layout_iter_get_line() which is like pango_layout_iter_get_line() but doesn't leak the line. * pango/pango-layout-private.h: Add pango_layout_iter_get_line() duplicate _pango_layout_iter_get_line_readonly() that doesn't leak the line. * pango/pango-renderer.c (pango_renderer_draw_layout): Use _pango_layout_iter_get_line_readonly().