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.

*** Bug 320325 has been marked as a duplicate of this bug. ***

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().

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.

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.

Created attachment 54398 [details] [review]
Patch to HEAD

This patch implements layout caching. Seems that it works, although I haven't
done any measurements.

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.

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);

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.

nickolay: have you had a chance to merge your patches?
federico: did you find time to test this?

any news on this? tia

We have tests/testcellrenderertext now, that can be used as a basic test of the correctness of this patch.

Created attachment 75314 [details] [review]
Updated patch

This patch is built from two previous patches and should apply to HEAD cleanly.

(Sorry for the late response, but I will try to give it a review next week).

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 ...).

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).

(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.

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.

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.

(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?

(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.

Created attachment 89572 [details] [review]
Updated patch

Patch updated to recent trunk

-      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...

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.

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...

> 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?

(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?

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.