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 627083 - :first-child, :last-child trigger theme transitions
:first-child, :last-child trigger theme transitions
Status: RESOLVED OBSOLETE
Product: gnome-shell
Classification: Core
Component: st
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
triaged
Depends on:
Blocks:
 
 
Reported: 2010-08-16 20:26 UTC by Owen Taylor
Modified: 2021-07-05 14:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[StWidget] Always start transitions from the last painted node (3.01 KB, patch)
2010-08-21 20:27 UTC, drago01
rejected Details | Review
Notice style transitions that don't change how StThemeNode paints (12.67 KB, patch)
2010-08-26 18:13 UTC, Owen Taylor
none Details | Review
Notice style transitions that don't change how StThemeNode paints (12.94 KB, patch)
2010-08-26 18:25 UTC, Owen Taylor
committed Details | Review
Queue style computation to a repaint function (23.00 KB, patch)
2010-08-26 22:01 UTC, Owen Taylor
none Details | Review
Queue style computation to a repaint function (23.00 KB, patch)
2010-08-26 22:48 UTC, Owen Taylor
needs-work Details | Review

Description Owen Taylor 2010-08-16 20:26:56 UTC
It seems that if we add a bunch of children to a container, each of them is transiently the last child of the container, and this triggers a transition from the style with :last-child to the style with :last-child.

I don't have an exact sequence of events. From a quick look, I think the theme node is first established when adding the child because  st_widget_style_changed() calls st_widget_recompute_style(). Then when widget is first size requested, st_widget_ensure_style() calls st_widget_recompute_style() which triggers the transition.

So, how to fix?

 A) We could have a rule of "no transitions from invisible states" - we hold a reference to the last theme node we *painted* (if any) and transitions always start from that theme node. In theory this might lead to inconsistent transitions, depending on how fast rendering is going, but given the reversing behavior we have I can't think of any realistic examples for things like hovers. It does mean that you can't use CSS transitions to produce an "opening" transition - like a fade-in of an object that didn't previously exist.

 B) We could try to make sure that widgets never have st_widget_recompute_style() called except during the allocate and paint phases, so the first time a widget got its theme node assigned would be when it was first allocated. This works out to about the same as A) but creates less temporary StThemeNodes

 C) We could add the explicit state machinery for transitions discussed in https://bugzilla.gnome.org/show_bug.cgi?id=619025, so only specific style transitions would ever trigger transitions.

A) seems like the easy thing to try, but maybe there is a better approach than any of these?
Comment 1 Dan Winship 2010-08-19 15:41:10 UTC
we currently only have one rule using :last-child... maybe we should be a little bit smarter about when we emit style-changed (only do it if the set of applicable CSS rules changes?)
Comment 2 drago01 2010-08-21 20:27:08 UTC
Created attachment 168478 [details] [review]
[StWidget] Always start transitions from the last painted node

In order to avoid transitions from "invisible states" hold a reference to the last painted node and always start transitions from this node.

-------

This is basically an implementation of A) (assuming I understood it correctly).
The good news is it result into a noticeable speedup, the bad news is that it isn't enough to fix it.

The 12x slowdown I was seeing goes down to 2x with this patch applied, which isstill a noticeable hit.
Comment 3 drago01 2010-08-21 20:34:41 UTC
(In reply to comment #2)
> Created an attachment (id=168478) [details] [review]
> [StWidget] Always start transitions from the last painted node
> 
> In order to avoid transitions from "invisible states" hold a reference to the
> last painted node and always start transitions from this node.
> 
> -------
> 
> This is basically an implementation of A) (assuming I understood it correctly).
> The good news is it result into a noticeable speedup, the bad news is that it
> isn't enough to fix it.
> 
> The 12x slowdown I was seeing goes down to 2x with this patch applied, which
> isstill a noticeable hit.

I tested with the patch from bug 627085 applied but it does not have any effect on the framerate.
Comment 4 Owen Taylor 2010-08-25 16:13:17 UTC
I investigated this some together with Adel, and it turns out that the problem is much harder than I thought. The problem is that transitions aren't just triggered by the immediate class/pseudo-classe/etc of a child changing, but also by the class/pseudo-class/etc of any parent changing.

In this case, the remaining transitions are being triggered by the "cover pane" in overview.js - as that is raised or lowered we add and remove :first-child properties.

I think the immediate short-term fix is probably to add a :track-first-last-child property to StContainer that has to be turned on when we want to use this functionality. (And to be careful when we turn it on.)

But long term, I think it's basically really bad if a change in a class or even in an irrelevant property in the parent class triggers a transition.

Some thoughts about long term fixes:

 * Transitions only care about the properties that StThemeNodePaint cares about. So, I was wrong when I thought that it was was an impossible task to compare properties - custom properties via st_theme_node_get_length() or whatever don't matter. And further more, if we've already computed and cached a properties we almost certainly want to do this for the new style as well. So, we can actually do a comparison of computed properties at this level without worrying about custom properties.

[ There's another optimization we can do there - in the case where they compare equal and we don't need to transition, we can fill in the cached textures in the new style by referencing the cached textures in the old style. This significantly cuts down on the expense of recomputing the entire tree of child styles when a parent object gains or loses a class. Though it's still not entirely cheap. ]

I think if we did that, then an explicit state machine becomes largely unneeded unless it was needed to better control the visual effects.

 * We need to be better about lazily computing theme nodes. Right now, it appears that we often recompute the theme node not when we need the theme node but when a property that affects the theme node changes. The call to gtk_widget_recompute_style() in st_widget_style_changed() needs to go away. However, this is more complex than it sounds because that call is what emits the ::style-changed signal. And without emitting ::style-changed *before relayout* custom properties won't work properly. Maybe we need to recompute styles in an appropriate meta_later_add() that runs after event handling and before allocation.  (This is independent of this issue, really, but would also reduce the expense of recomputing all the child theme nodes when a class changes on the parent. It's bad once, it's worse twice.)
Comment 5 Owen Taylor 2010-08-26 18:13:51 UTC
Created attachment 168827 [details] [review]
Notice style transitions that don't change how StThemeNode paints

Add st_theme_node_paint_equal() and use that to do two things:

 1) Avoid animating transitions where nothing changes.
 2) Copy cached painting state from the old theme node to the new
    theme node.
Comment 6 Owen Taylor 2010-08-26 18:25:01 UTC
Created attachment 168828 [details] [review]
Notice style transitions that don't change how StThemeNode paints

Here's a version that actually works. (Screwed the last version up when
adding comments and cleaning it up a bit.)
Comment 7 drago01 2010-08-26 19:07:57 UTC
Review of attachment 168478 [details] [review]:

Owen's patch achieves pretty much the same + more so that one isn't really needed.
Comment 8 Owen Taylor 2010-08-26 22:01:51 UTC
Created attachment 168847 [details] [review]
Queue style computation to a repaint function

Instead of immediately recomputing styles when a style property is changed,
keep a list of widgets whose styles need recomputation, and run through that
list in a repaint function that runs before size allocation.
Comment 9 Owen Taylor 2010-08-26 22:48:27 UTC
Created attachment 168852 [details] [review]
Queue style computation to a repaint function

Here's a work-in-progress implementation of the idea of making style recomputation
more lazy. Some numbers:

With only the previous patch
------------------------------------------------------------
overviewFpsFirst 4.99932509111, 5.45300869755, 5.45323172145
overviewFpsSubsequent 26.2474080685, 26.2473096508, 24.0041287101
overviewLatencyFirst 314748, 292292, 290858
overviewLatencySubsequent 169606, 168105, 166518
------------------------------------------------------------

With both patches
------------------------------------------------------------
overviewFpsFirst 7.05716302047, 7.0591806409, 6.6659556314
overviewFpsSubsequent 25.7058395099, 23.9976962212, 26.2484907118
overviewLatencyFirst 185281, 217672, 181830
overviewLatencySubsequent 115826, 111589, 113564
------------------------------------------------------------

(In these tests I had 5 full-screen windows open, and I have a lot of deleted recent
documents, which is why the overvewFpsFirst numbers are so bad - the first time I
go into the overview, it is continually adding files to recently used and then
deleting them again.)

I think these numbers show that the idea is definitely promising -
there's a pretty big decrease in latency - in the time to set up
actors. Howver, there are some issues with it too - the basic problem
is that ::style-changed can be emitted at arbitrary times - if
anything needs a style before the repaint function runs, then the
first call to get_theme_node() causes a ::style-changed signal to be
omitted.

So, for example, if we get the sequence:

 - Click on the date to open the panel clock
 - Move the mouse
 - Run repaint functions
 - Allocate the stage

Then the "pick" that is done when handling the mouse motions, can trigger allocation,
and that can trigger a style-set handler. And if the style set handler does something
that queues a size request then you get the dreaded

 "The actor `StLabel` is currently inside an allocation cycle; calling
  clutter_actor_queue_relayout is not recommended"

warning. I don't have a lot of ideas yet about how to handle this. What you really
want to do for the above is to have Clutter ensure the styles for a section of the
actor tree before size-negotiating that section of the actor tree, but clutter
doesn't really know any of this is going on.
Comment 10 drago01 2010-08-28 11:24:27 UTC
Review of attachment 168828 [details] [review]:

This patch restores the performance to the previous level here. (i.e from ~2.xx fps to ~40.xx fps).

The code looks fine to me, just minor nitpicks.

::: src/st/st-border-image.c
@@ +114,3 @@
+          image->border_bottom == other->border_bottom &&
+          image->border_left == other->border_left &&
+          strcmp (image->filename, other->filename) == 0);

Any reason why you don't just use g_strcmp0 here ? (to be consistent with the rest of the code).

::: src/st/st-theme-node-drawing.c
@@ +1201,3 @@
+
+/**
+ * st_theme_node_copy_paint_caches:

Hmm ... this name is kind of odd. It isn't really about copying the cache but the cached state.

What about something like st_theme_node_copy_cached_paint_state ?
Comment 11 drago01 2010-08-29 14:49:34 UTC
Comment on attachment 168847 [details] [review]
Queue style computation to a repaint function

This one seems to be attached twice, remove the first copy to avoid confusion.
Comment 12 Owen Taylor 2010-08-30 17:41:49 UTC
Comment on attachment 168828 [details] [review]
Notice style transitions that don't change how StThemeNode paints

Attachment 168828 [details] pushed as 5e7c25e - Notice style transitions that don't change how StThemeNode paints
Comment 13 Owen Taylor 2010-08-30 17:42:33 UTC
(In reply to comment #10)
> Review of attachment 168828 [details] [review]:
> 
> This patch restores the performance to the previous level here. (i.e from ~2.xx
> fps to ~40.xx fps).
> 
> The code looks fine to me, just minor nitpicks.
> 
> ::: src/st/st-border-image.c
> @@ +114,3 @@
> +          image->border_bottom == other->border_bottom &&
> +          image->border_left == other->border_left &&
> +          strcmp (image->filename, other->filename) == 0);
> 
> Any reason why you don't just use g_strcmp0 here ? (to be consistent with the
> rest of the code).

I don't really think of "g_strcmp0" as "better strcmp" but rather as "compare strings, and also compare NULLs". Since image->filename can't be NULL, doesn't seem appropriate here.

> ::: src/st/st-theme-node-drawing.c
> @@ +1201,3 @@
> +
> +/**
> + * st_theme_node_copy_paint_caches:
> 
> Hmm ... this name is kind of odd. It isn't really about copying the cache but
> the cached state.
> 
> What about something like st_theme_node_copy_cached_paint_state ?

But it's so loong :-). Since I'm always campaigning for descriptive names, and if they're long, deal, I can't really object. Changed.
Comment 14 Dan Winship 2010-10-22 15:21:34 UTC
(In reply to comment #9)
> I don't have a lot of ideas yet about how to handle this. What you really
> want to do for the above is to have Clutter ensure the styles for a section of
> the
> actor tree before size-negotiating that section of the actor tree, but clutter
> doesn't really know any of this is going on.

Maybe the stage could emit a signal right before it relayouts, and we could recompute all dirty styles at that point?
Comment 15 Jasper St. Pierre (not reading bugmail) 2012-03-15 21:00:24 UTC
Review of attachment 168852 [details] [review]:

While a good cleanup (and one I'd like to see rebased, eventually), the patch hasn't applied for quite a while.
Comment 16 drago01 2012-03-15 21:03:24 UTC
Review of attachment 168852 [details] [review]:

"and one I'd like to see rebased, eventually" ... well that does not sound like a rejection ;)
Comment 17 drago01 2013-08-31 21:49:42 UTC
So what about this? Owen do you still plan to work on this? Does it still make sense?
Comment 18 GNOME Infrastructure Team 2021-07-05 14:47:43 UTC
GNOME is going to shut down bugzilla.gnome.org in favor of  gitlab.gnome.org.
As part of that, we are mass-closing older open tickets in bugzilla.gnome.org
which have not seen updates for a longer time (resources are unfortunately
quite limited so not every ticket can get handled).

If you can still reproduce the situation described in this ticket in a recent
and supported software version, then please follow
  https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines
and create a new ticket at
  https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/

Thank you for your understanding and your help.