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 168654 - GtkCellRendererText continually re-creates PangoLayouts
GtkCellRendererText continually re-creates PangoLayouts
Status: RESOLVED OBSOLETE
Product: gtk+
Classification: Platform
Component: Widget: GtkTreeView
2.6.x
Other Linux
: Normal normal
: Medium fix
Assigned To: gtktreeview-bugs
gtktreeview-bugs
: 320325 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2005-02-27 13:56 UTC by Ross Burton
Modified: 2018-02-10 03:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
All PangoLayouts measured during GtkFileChooser startup (33.88 KB, text/plain)
2005-11-04 19:23 UTC, Federico Mena Quintero
  Details
Stack traces of pango_layout_check_lines as a descendant of gtk_cell_renderer_text_render (9.22 KB, text/plain)
2005-11-04 19:26 UTC, Federico Mena Quintero
  Details
Patch to HEAD (8.17 KB, patch)
2005-11-06 22:22 UTC, Nickolay V. Shmyrev
none Details | Review
Patch to optimize layout recalculation (2.05 KB, patch)
2005-11-11 09:44 UTC, Nickolay V. Shmyrev
none Details | Review
Updated patch (9.70 KB, patch)
2006-10-24 16:26 UTC, Nickolay V. Shmyrev
none Details | Review
Updated patch (8.48 KB, patch)
2007-06-07 21:51 UTC, Nickolay V. Shmyrev
reviewed Details | Review

Description Ross Burton 2005-02-27 13:56:18 UTC
Creating and laying out a PangoLayout isn't trivial, so would there be some way
of caching the PangoLayouts for cells?

At the moment the PangoLayout is re-created every time the mouse moved over a
cell, so maybe caching the previous PangoLayout and comparing the state would
help in this case.
Comment 1 Kristian Rietveld 2005-06-18 10:26:09 UTC
I remember looking at this a long time ago, but I am not sure what happened with
it. I'll try to get this in 2.8, because it might help a lot with performance.
Comment 2 Matthias Clasen 2005-11-04 17:17:52 UTC
*** Bug 320325 has been marked as a duplicate of this bug. ***
Comment 3 Federico Mena Quintero 2005-11-04 19:20:42 UTC
I took some profiles of this.  I don't have the exact numbers anymore, but as I
remember it, we were running Pango through each cell renderer's text 3 times: 
one during gtk_cell_renderer_get_size, two during render().
Comment 4 Federico Mena Quintero 2005-11-04 19:23:05 UTC
Created attachment 54321 [details]
All PangoLayouts measured during GtkFileChooser startup

This is the output of running the file chooser on my $HOME with a hacked Pango.
 It will print a line of output when it has to run a string through the Pango
engine.

Note how many strings (for the same layouts) are measured twice in sequence;
this is the same cell renderer performing more work than needed.
Comment 5 Federico Mena Quintero 2005-11-04 19:26:40 UTC
Created attachment 54322 [details]
Stack traces of pango_layout_check_lines as a descendant of gtk_cell_renderer_text_render

This is two stack traces that show the times when pango_layout_check_lines()
gets called to do the full work inside gtk_cell_renderer_text_render().

The first time is during get_size(); the second one is during the actual
gtk_paint_layout().

We cause Pango to re-measure the string because we change some layout
properties between two calls (ellipsization or width, if I remember correctly).
 It may be possible to set this properties once at the beginning, and cause
Pango to process the string just once.
Comment 6 Nickolay V. Shmyrev 2005-11-06 22:22:25 UTC
Created attachment 54398 [details] [review]
Patch to HEAD

This patch implements layout caching. Seems that it works, although I haven't
done any measurements.
Comment 7 Nickolay V. Shmyrev 2005-11-06 22:29:15 UTC
Federico, sorry, but I don't understand your point in comments #4 and #5. This
bug is about duplicated creation of layout, not about duplicated measurement of
it's extents. About measurements I can say it seems natural - we need extents to
calculate padding and we need extents to draw layout. 

Between this two calls pango attrs aren't changed at all. So the extents can be
cached, but it should be PangoLayout work.
Comment 8 Federico Mena Quintero 2005-11-10 20:20:48 UTC
Nickolay:  oh, yeah, layouts are created twice for the same cell renderer.  It
would be great to get rid of that as well - I think I'll be able to test your
patch next week or so.

The problem I described is that even within the same layout, we cause it to
measure itself twice.  The cell renderer does something like

  layout = get_layout();
  size = get_size ();
  frob_some_properties_in_the_layout(layout); /* this causes the layout to
invalidate itself, causing a re-measurement in paint() */
  paint (layout);
Comment 9 Nickolay V. Shmyrev 2005-11-11 09:44:08 UTC
Created attachment 54626 [details] [review]
Patch to optimize layout recalculation

I see now, Federico. That is because you are setting ellipsize for that
renderer. I think it can be optimized also with patch attached. It should force
layout to calculate with only one time per cell_renderer_render call. 

Two patches above conflict I think, I'll merge them later.
Comment 10 André Klapper 2006-10-07 16:07:59 UTC
nickolay: have you had a chance to merge your patches?
federico: did you find time to test this?

any news on this? tia
Comment 11 Matthias Clasen 2006-10-08 14:03:27 UTC
We have tests/testcellrenderertext now, that can be used as a basic test of the correctness of this patch.
Comment 12 Nickolay V. Shmyrev 2006-10-24 16:26:44 UTC
Created attachment 75314 [details] [review]
Updated patch

This patch is built from two previous patches and should apply to HEAD cleanly.
Comment 13 Kristian Rietveld 2006-11-02 15:22:50 UTC
(Sorry for the late response, but I will try to give it a review next week).
Comment 14 Kristian Rietveld 2006-11-21 19:13:17 UTC
The first thing I noticed is that the patch clears the cached layout in the _set_property() method.  Of course this patch will/should still help in the case _get_size() and _render() are called consecutively without attribute changes; I think there *might* be some more potential.  For example a tree view with a bunch of rows which all render text with the same attributes.  In this case only the string changes and I think it should possible to always cache the layout and just change the string in this case.

Of course I have no idea if this is really viable, we probably need some measurements for this first.  But if it is, things would be a little different from this patch; ie. we would always keep a layout around and have _set_property() also update the layout.


Other than that I am wondering what happened with this chunk:

@@ -1437,12 +1413,6 @@ get_layout (GtkCellRendererText *celltex
     {
       pango_layout_set_width (layout, priv->wrap_width * PANGO_SCALE);
       pango_layout_set_wrap (layout, priv->wrap_mode);
-
-      if (pango_layout_get_line_count (layout) == 1)
-	{
-	  pango_layout_set_width (layout, -1);
-	  pango_layout_set_wrap (layout, PANGO_WRAP_CHAR);
-	}
     }

Why do we need to delete the if statement here (I see no place in the patch where this comes back).

Another thing I am wondering about is:

@@ -1662,12 +1692,6 @@ gtk_cell_renderer_text_render (GtkCellRe
       cairo_destroy (cr);
     }
 
-  if (priv->ellipsize_set && priv->ellipsize != PANGO_ELLIPSIZE_NONE)
-    pango_layout_set_width (layout, 
-			    (cell_area->width - x_offset - 2 * cell->xpad) * PANGO_SCALE);
-  else if (priv->wrap_width == -1)
-    pango_layout_set_width (layout, -1);

the if branch has been moved up in this function, but the else if clause has been deleted and does not reappear.  Why?  (I am really wondering about this, since most of this is part of the ellipsization support which is pretty fragile ...).

Comment 15 Nickolay V. Shmyrev 2006-11-22 00:05:21 UTC
Thanks for looking on this Kris.

About additional optimizations, I agree they are possible, but probably it's better to make little step first.

About first chunk:

-      if (pango_layout_get_line_count (layout) == 1)
-       {
-         pango_layout_set_width (layout, -1);
-         pango_layout_set_wrap (layout, PANGO_WRAP_CHAR);
-       }

actually I don't see the reason we should set width to -1 here. Why really we return back to WRAP_CHAR? Does it give something to us? Probably I misunderstand something here. The might be another reason I did it, but it was lost during last year :)

About second chunk:

-  else if (priv->wrap_width == -1)
-    pango_layout_set_width (layout, -1);

With the patch we set width to -1 earlier in get_layout. I think there is no need to set width twice. Moreover it will cause layout extents recalculation (see the original Federico's problem).
Comment 16 Kristian Rietveld 2006-11-28 15:50:39 UTC
(In reply to comment #15)
> About additional optimizations, I agree they are possible, but probably it's
> better to make little step first.

That might be a good idea.
 
> About first chunk:
> 
> -      if (pango_layout_get_line_count (layout) == 1)
> -       {
> -         pango_layout_set_width (layout, -1);
> -         pango_layout_set_wrap (layout, PANGO_WRAP_CHAR);
> -       }
> 
> actually I don't see the reason we should set width to -1 here. Why really we
> return back to WRAP_CHAR? Does it give something to us? Probably I
> misunderstand something here. The might be another reason I did it, but it was
> lost during last year :)

I don't know either, it looks like this was introduced when Matthias committed his word wrapping patch last year.  I'll ask him if he remembers why this was needed.

> About second chunk:
> 
> -  else if (priv->wrap_width == -1)
> -    pango_layout_set_width (layout, -1);
> 
> With the patch we set width to -1 earlier in get_layout. I think there is no
> need to set width twice. Moreover it will cause layout extents recalculation
> (see the original Federico's problem).

Ah yes, you're right :)

So, let's ask Matthias why that first chunk is needed; I will try testcellrenderertext with the patch applied tonight.  After that I think this is probably fine to commit on HEAD.


Comment 17 Matthias Clasen 2007-05-22 17:04:10 UTC
Sorry, I don't remember why exactly it was needed. It may be that alignment of single-line cells went bad otherwise. It may not be needed anymore. Now that we have testcellrenderertest, it should be possible to verify that we can remove it without regression.
Comment 18 Behdad Esfahbod 2007-06-04 21:01:54 UTC
Note that setting the width of layout to -1 if it's already set to that, doesn't invalidate the layout.  Same for most other properties: setting them to the current value doesn't invalidate the layout.  The exception of course is things like setting text, markup, or attributes.
Comment 19 Behdad Esfahbod 2007-06-05 21:35:53 UTC
(In reply to comment #15)
> Thanks for looking on this Kris.
> 
> About additional optimizations, I agree they are possible, but probably it's
> better to make little step first.
> 
> About first chunk:
> 
> -      if (pango_layout_get_line_count (layout) == 1)
> -       {
> -         pango_layout_set_width (layout, -1);
> -         pango_layout_set_wrap (layout, PANGO_WRAP_CHAR);
> -       }
> 
> actually I don't see the reason we should set width to -1 here. Why really we
> return back to WRAP_CHAR? Does it give something to us? Probably I
> misunderstand something here. The might be another reason I did it, but it was
> lost during last year :)

When width is -1, wrap mode doesn't even matter.  So, I'll leave that code as is.  It's either serving some purpose (hence was added) or is harmless.

> About second chunk:
> 
> -  else if (priv->wrap_width == -1)
> -    pango_layout_set_width (layout, -1);
> 
> With the patch we set width to -1 earlier in get_layout. I think there is no
> need to set width twice. Moreover it will cause layout extents recalculation
> (see the original Federico's problem).

Again, this doesn't have any overhead.

Can the patch go in now?
Comment 20 Kristian Rietveld 2007-06-06 08:01:06 UTC
(In reply to comment #19)
> > -      if (pango_layout_get_line_count (layout) == 1)
> > -       {
> > -         pango_layout_set_width (layout, -1);
> > -         pango_layout_set_wrap (layout, PANGO_WRAP_CHAR);
> > -       }
> > 

Just for the record, this code is not there anymore on both trunk and gtk-2-10.


I would say we get this patch in trunk ASAP since we have devel releases and all going now.
Comment 21 Nickolay V. Shmyrev 2007-06-07 21:51:51 UTC
Created attachment 89572 [details] [review]
Updated patch

Patch updated to recent trunk
Comment 22 Matthias Clasen 2007-06-10 04:30:15 UTC
-      if (priv->ellipsize || priv->width_chars > 0)
+      if (priv->ellipsize_set || priv->width_chars > 0)

This doesn't look right. Shouldn't it be

if ((priv->ellipsize_set && priv->ellipsize != PANGO_ELLIPSIZE_NONE) ||
    priv->width_chars > 0)

?

As Kris already pointed out, it seems a little odd that the cached
layout is thrown away whenever any property is set.

Also, I repeat my question from comment 17, has anybody run testcellrenderertext to verify that this patch does not have any unintended side-effects ?
As Kris points out, ellipsization support is pretty fragile...
Comment 23 Nickolay V. Shmyrev 2007-06-10 15:20:18 UTC
About ellipsize_set I completely agree, it should be (priv->ellipsize_set && priv->ellipsize != PANGO_ELLIPSIZE_NONE) just to be similar. It wasn't that before, that's why it's updated.

About drop on property change most properties change geometry so layout should be recalculated anyhow and probably there will be no real improvement. The only set which is not important is foreground/background/editable. Everything else potentially changes layout.

I've tried testcellrenderertext, it doesnt check ellipsize at all. As far as I can see widget before patch and after it looks the same. But I dont know how to check it better.

Comment 24 Behdad Esfahbod 2007-06-11 02:34:39 UTC
Even if the layout needs to be invalidated, there's not much point in freeing one layout and creating a new one.  Just use the old one...
Comment 25 Nickolay V. Shmyrev 2007-06-11 04:21:44 UTC
> Even if the layout needs to be invalidated, there's not much point in freeing
one layout and creating a new one.  Just use the old one...

If we are going to reproduce old behavior we have to reset layout when property is set. For example if layout has underline for one cell we have to remove underline if we are trying to render another cell. How can it be implemented? Will it be faster just to reset layout instead?
Comment 26 Behdad Esfahbod 2007-06-11 07:24:06 UTC
(In reply to comment #25)
> > Even if the layout needs to be invalidated, there's not much point in freeing
> one layout and creating a new one.  Just use the old one...
> 
> If we are going to reproduce old behavior we have to reset layout when property
> is set. For example if layout has underline for one cell we have to remove
> underline if we are trying to render another cell. How can it be implemented?
> Will it be faster just to reset layout instead?

I wasn't suggesting to use the same layout for multiple cells, no.  I thought it's more about per-cell properties.  Anyway, resetting is a matter of pango_layout_set_text() pango_layout_set_attributes().  Other properties (width, ellipsized, ...) we are setting already anyway, right?
Comment 27 Matthias Clasen 2018-02-10 03:32:48 UTC
We're moving to gitlab! As part of this move, we are closing bugs that haven't seen activity in more than 5 years. If this issue is still imporant to you and
still relevant with GTK+ 3.22 or master, please consider creating a gitlab issue
for it.