GNOME Bugzilla – Bug 730886
Avoid false change notifications with GSettings bindings
Last modified: 2017-09-26 02:53:23 UTC
Created attachment 277383 [details] [review] proposed glib patch False change notifications issued by GSettings bindings can cause UI freezes in particular cases. This had been spot by evolution, but can sometimes be triggered by gnome-shell as well. I did not notice any other applications, but that can also because Evolution uses GSettings bindings heavily. The root cause of these false notifications is dconf, when it notifies of the root path change (the path is "/"), while there did not change anything. I do not think it's possible to have each user of GSettings bindings check for a property change, especially when the binding is only a bridge between a GtkWidget descendant property and a GSettings key value. g_object_set() also doesn't help much with change notifications, but that's another issue. The attached is a simple glib patch to remember and test values before passing them to the GObject property or to the GSettings key within the GSettings binding, simply avoiding false notifications when the value did not change.
GSettings already does explicit loop-breaking for the direct case of a notify received when changing the property -- this is the problem caused by g_object_set() automatically notifying. As for other cases, it's the usual behaviour for property setters to check for this and not emit the notify if no change occurred. I'd argue that this patch is more or less covering up bugs in those classes that don't do this... Is there a fundamental reason that this can't be fixed there, or is it just a case of being too much work to fix all the cases?
(In reply to comment #1) > Is there a fundamental reason that this can't be fixed there, or is it just a > case of being too much work to fix all the cases? I'd not call that "too much work to fix", it's rather about "too hard to identify all the places where it strikes, especially when the problem is caused by a used library, when the library consumer has absolutely no influence on. Also note that having the change checker on one central place makes more sense than having it all around the code, duplicated. Some change listeners are also dependent only to trigger some rebuilds of UI, thus they are not real setters, but they connect directly to GSettings, which does false change notifications, the same as the GObject. Writing the "value-really-changed" code for such listeners involves certain burden.
The patch causes extra overhead - you make us store a (possibly large) gvariant for each binding. Instead, the check should be done where you already have the old and the new value available in the same place - that is the setter of the property.
We both seem to agree here -- so WONTFIXed, sorry :/
(In reply to comment #3) > The patch causes extra overhead - you make us store a (possibly large) gvariant > for each binding. Instead, the check should be done where you already have the > old and the new value available in the same place - that is the setter of the > property. That doesn't work. I just caught a backtrace where the setter is not involved at all. My setter properly checks the change and avoids the notification, but the GSettingsBinding calls g_object_set_property(), which does the notification "for me", and thus my signal connector on "notify::property-name" signal, which relies on correct notifications and has no ability to check for changes, because it's another objects which updates yet another objects when certain property of the source object changes. I do not know whether it's understood from the above, but the chain is this: a) false change notification from dconf, which causes b) false change notification from GSettings, which causes c) false change notification from GSettingsBinding, which causes d) false change notification from GObject's set_property(), which causes e) call of "notify::property-name" callback, which relies on true notifications, because it runs flood of expensive operations when the property changes. The backtrace I talk about, with debuginfo for eds and evo only, from 3.8.5 of both:
+ Trace 233660
Thread 1 (Thread 0x7fd475052a40 (LWP 13349))
But this is really just a bug in GObject that you're talking about now -- the fact that _set() sends notify unconditionally. That's something that is fixed by GProperty, fwiw..
(In reply to comment #5) I think you're just going to have to handle these things case-by-case. In the example you mentioned, EMailDisplay should stash the last-used image loading policy internally and check it against EMailFormatter's current policy at the beginning of its "notify" handler. If there is no actual change, have the handler return without doing anything.
(In reply to comment #7) > I think you're just going to have to handle these things case-by-case. That's the problem, anything I do will be a workaround for these GObject/GSettingsBindings/dconf false notifications. This case is even simple, but there can be tens to hundreds of such cases in the code.
I agree that a fix is needed here -- but it is to GObject, not GSettings.
I've opened bug 731200 to deal with this issue separately. No reason we can't do a fix this cycle.