GNOME Bugzilla – Bug 627083
:first-child, :last-child trigger theme transitions
Last modified: 2021-07-05 14:47:43 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?
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?)
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.
(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.
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.)
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.
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.)
Review of attachment 168478 [details] [review]: Owen's patch achieves pretty much the same + more so that one isn't really needed.
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.
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.
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 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 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
(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.
(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?
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.
Review of attachment 168852 [details] [review]: "and one I'd like to see rebased, eventually" ... well that does not sound like a rejection ;)
So what about this? Owen do you still plan to work on this? Does it still make sense?
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.