GNOME Bugzilla – Bug 690450
GObject: avoid resolving property names for all accesses
Last modified: 2018-01-21 22:16:20 UTC
Keep a cache mapping jsids to GParamSpecs, and use that to avoid converting property names from UTF-16 to UTF-8 and hypenating them for each property access.
Created attachment 231831 [details] [review] GObject: avoid resolving property names for all accesses
Review of attachment 231831 [details] [review]: ::: gi/object.c @@ +69,3 @@ + /* A GHashTable mapping jsids to GParamSpec, used to cache + property lookups for GObjects */ + GHashTable *property_cache; Shouldn't we have a cache per *class*, not per instance? The data could be hooked off of qdata on the GType, or it could be in the ObjectInstance of the prototype. @@ +256,3 @@ + gname); + + g_hash_table_insert(priv->property_cache, (gpointer)id, param); So we're caching this even if param == NULL? Is that intentional?
source(In reply to comment #2) > Review of attachment 231831 [details] [review]: > > ::: gi/object.c > @@ +69,3 @@ > + /* A GHashTable mapping jsids to GParamSpec, used to cache > + property lookups for GObjects */ > + GHashTable *property_cache; > > Shouldn't we have a cache per *class*, not per instance? The data could be > hooked off of qdata on the GType, or it could be in the ObjectInstance of the > prototype. No, the cache is explicitly per instance, because it wants to track so-called expando properties too. > @@ +256,3 @@ > + gname); > + > + g_hash_table_insert(priv->property_cache, (gpointer)id, param); > > So we're caching this even if param == NULL? Is that intentional? Yes, because by caching NULL I can recognize a JS-only property without resolving the jsid into a string.
(In reply to comment #3) > source(In reply to comment #2) > > Review of attachment 231831 [details] [review] [details]: > > > > ::: gi/object.c > > @@ +69,3 @@ > > + /* A GHashTable mapping jsids to GParamSpec, used to cache > > + property lookups for GObjects */ > > + GHashTable *property_cache; > > > > Shouldn't we have a cache per *class*, not per instance? The data could be > > hooked off of qdata on the GType, or it could be in the ObjectInstance of the > > prototype. > > No, the cache is explicitly per instance, because it wants to track so-called > expando properties too. Why does object.c need to know about expandos?
Because object_instance_get_prop/object_instance_set_prop get called for *all* properties that are installed on each GObject instance, every time. The alternative to that is to resolve GObject properties and define them with a custom getter and setter. The downside of this solution is that there is no closure data for JSPropertyOps, so you still need to resolve the jsid into a name, fetch the param spec, etc. We might be able to do something clever with TinyIds and maybe GPropertyInfo too, but we still need to resolve properties on objects that have no introspection support (eg. GDrive, GFile...)
(In reply to comment #5) > Because object_instance_get_prop/object_instance_set_prop get called for *all* > properties that are installed on each GObject instance, every time. True. But still though, I think the positive lookup cache should be stored as qdata on the GType. If I create 50 Clutter.Actor instances, there shouldn't be 50 hash tables mapping "width" -> ClutterActor's width paramspec. It's not fully clear to me that we need a negative lookup cache; what if we just skip out of resolve here if the property name starts with '_'?
source(In reply to comment #6) > (In reply to comment #5) > > Because object_instance_get_prop/object_instance_set_prop get called for *all* > > properties that are installed on each GObject instance, every time. > > It's not fully clear to me that we need a negative lookup cache; what if we > just skip out of resolve here if the property name starts with '_'? To do that, you need to call gjs_get_string_id(). At that point, you already lost. The optimization is not calling gjs_get_string_id() unless there is a cache miss, which should not happen after the first property use. Sure thing, we can have a two-level cache, with a positive only cache in the prototype and a positive/negative one in the instance, and that would save quite a bit of memory. I'm less convinced it would help performance in the code path that is most sensible to this optimization, ie Tweener animation of Clutter actors.
(In reply to comment #7) > source(In reply to comment #6) > > (In reply to comment #5) > > > Because object_instance_get_prop/object_instance_set_prop get called for *all* > > > properties that are installed on each GObject instance, every time. > > > > It's not fully clear to me that we need a negative lookup cache; what if we > > just skip out of resolve here if the property name starts with '_'? > > To do that, you need to call gjs_get_string_id(). At that point, you already > lost. > The optimization is not calling gjs_get_string_id() unless there is a cache > miss, which should not happen after the first property use. Hmm. We should be able to fast path the jsid -> char * better. From what I can tell at a quick look at jsapi.h, a jsid may only hold interned JSStrings. What if we had a per-GType hash table that mapped JSString* -> GParamSpec?
Nevermind, you're already taking advantage of that. But if we just want to check whether the first character is '_', we could call JS_GetInternedStringChars()[0] == (jschar)'_' and avoid the malloc, right?
Created attachment 231850 [details] [review] GObject: avoid resolving property names for all accesses Keep a cache mapping jsids to GParamSpecs, and use that to avoid converting property names from UTF-16 to UTF-8 and hypenating them for each property access. Ok, this implements the two level cache, without looking at the names of expando properties.
Review of attachment 231850 [details] [review]: ::: gi/object.c @@ +65,3 @@ and may be finalized at will. */ gboolean uses_toggle_ref; Hmm...this patch is on top of http://bugzilla-attachments.gnome.org/attachment.cgi?id=220596 ? I suppose we should do a more final yes/no on that patch. I'll comment on that bug. @@ +186,2 @@ + /* First check for the id in the cache */ + param = g_hash_table_lookup(proto_priv->property_cache, (gpointer)id); Can we cast to (JSString*) here? It's the same code, but I think it's clearer what's going on. @@ +207,2 @@ + if (G_LIKELY (param != NULL)) + g_hash_table_insert(proto_priv->property_cache, (gpointer)id, param); Here we take ownership of param. @@ +208,3 @@ + g_hash_table_insert(proto_priv->property_cache, (gpointer)id, param); + else if (priv != NULL) + g_hash_table_add(priv->property_cache, (gpointer)id); And here we leak the reference to the caller. @@ +209,3 @@ + else if (priv != NULL) + g_hash_table_add(priv->property_cache, (gpointer)id); + g_free(gname); Also need g_type_class_unref(klass) @@ +1809,3 @@ priv->gtype = gtype; priv->klass = g_type_class_ref (gtype); + priv->property_cache = g_hash_table_new (g_direct_hash, g_direct_equal); I keep thinking this GHashTable which is just shadowing the JS object properties is horribly ugly. There has to be a way for us to say in the resolve op "if the JS object already has this property, return it". Can we call something like JS_AlreadyHasOwnPropertyById()?
(In reply to comment #11) > Review of attachment 231850 [details] [review]: > > @@ +207,2 @@ > + if (G_LIKELY (param != NULL)) > + g_hash_table_insert(proto_priv->property_cache, (gpointer)id, param); > > Here we take ownership of param. > > @@ +208,3 @@ > + g_hash_table_insert(proto_priv->property_cache, (gpointer)id, param); > + else if (priv != NULL) > + g_hash_table_add(priv->property_cache, (gpointer)id); > > And here we leak the reference to the caller. param comes from object_class_find_property, which is (transfer none), so I don't see the leak. And anyway, if we take the second branch, param is NULL. > @@ +209,3 @@ > + else if (priv != NULL) > + g_hash_table_add(priv->property_cache, (gpointer)id); > + g_free(gname); > > Also need g_type_class_unref(klass) > > @@ +1809,3 @@ > priv->gtype = gtype; > priv->klass = g_type_class_ref (gtype); > + priv->property_cache = g_hash_table_new (g_direct_hash, g_direct_equal); > > I keep thinking this GHashTable which is just shadowing the JS object > properties is horribly ugly. There has to be a way for us to say in the > resolve op "if the JS object already has this property, return it". Can we > call something like JS_AlreadyHasOwnPropertyById()? Uhm... Perhaps. The problem is that AlreadyHasOwnPropertyById returns true for all properties that were set at least once, including GObject properties. We can hack around that and observe that the stored value of GObject properties is undefined, so if we find anything else, that's surely a JS only property.
(In reply to comment #12) > > param comes from object_class_find_property, which is (transfer none), so I > don't see the leak. Ah right, sorry, you are correct. > Uhm... Perhaps. > The problem is that AlreadyHasOwnPropertyById returns true for all properties > that were set at least once, including GObject properties. > We can hack around that and observe that the stored value of GObject properties > is undefined, so if we find anything else, that's surely a JS only property. Hum. Is it really undefined? It looks to me like we're not overriding the value to be set. So when you do something like label.text = "Hello!" we call g_object_set and *also* store a "text" property on the wrapper object. Wasteful. It looks to me like in the _set_prop hook we can override the value to be set. What if we stored JSVAL_VOID instead? Would that cause JS_AlreadyHasOwnProperty() to return false?
Created attachment 232177 [details] [review] GObject: avoid resolving property names for all accesses Keep a cache mapping jsids to GParamSpecs, and use that to avoid converting property names from UTF-16 to UTF-8 and hypenating them for each property access.
Review of attachment 232177 [details] [review]: Still need to figure out about the toggle ref, or rebase this patch. Otherwise good to go. ::: gi/object.c @@ +65,3 @@ and may be finalized at will. */ gboolean uses_toggle_ref; So this is still based on the toggle ref patch...if they're not actually tied together, I'd be happy landing this first. Ideally Owen would respond on that bug though. @@ +317,3 @@ + GObject property, because we don't let the value through + for those. + */ The comment here seems odd - this is when we're setting a property. It should be harmless to avoid attempting to set properties to undefined though.
Created attachment 232286 [details] [review] GObject: avoid resolving property names for all accesses Keep a cache mapping jsids to GParamSpecs, and use that to avoid converting property names from UTF-16 to UTF-8 and hypenating them for each property access. Indeed not only the comment was wrong, that check caused all GObject property sets to be ignored, causing all sort of weirdness. Unfortunately, *value_p for set_property is not the stored value, is the new value, so we can't use that to check if it is an expando property or not.
Created attachment 232287 [details] [review] GObject: avoid resolving property names for all accesses Keep a cache mapping jsids to GParamSpecs, and use that to avoid converting property names from UTF-16 to UTF-8 and hypenating them for each property access. Much worse! *value_p after get_prop is called becomes the new stored value, so the hack works neither on set nor on get. We need something better...
Created attachment 232302 [details] [review] object: define GObject properties at resolve time instead of hooking get and set Resolve and define GObject properties like methods, instead of modifying the result of getProperty and setProperty. This allows to have two different code paths for GObject properties and expando properties, and to take better advantage of the interpreter optimizations for already defined properties. It is also a prerequisite for using introspection information to access properties.
Ah, already found a problem with this patch: now GObject properties shadow methods with the same name completely, so the old hack of doing Soup.Session.prototype.add_feature no longer works. I see three alternatives here: 1) make methods take priority over GObject properties This is still breaking ABI, because it changes the meaning of session.add_feature = ...; but it is a lot less likely to break existing code for this particular case, and that's really the only example of problematic API I know of 2) live with the API breakage and fix gnome-shell code to do session.add_feature = feature; and at the same time remember that this was an awful idea on the Soup side and should be never repeated again 3) pay attention to the start object and only resolve as property when starting of from a GObject instance This is probably the most API compatible, but also very hacky and likely to bug the programmer in unexpected ways. In particular, once a property is resolved, it is there forever, and prototype chains are shared, so "session.add_feature" would change meaning depending on the presence or not of a "Soup.Session.prototype.add_feature" access before it (setting "session.add_feature" would not be affected, because the method is not an accessor/JSPROP_SHARED property, so it is shadowed when setting on the instance)
Created attachment 232306 [details] [review] object: only resolve GObject properties when starting from instances For backward compatibility, we're required not to resolve GObject properties when starting from prototypes, so if they conflict with a method of the same name. This is option 3. Except that I was wrong: with this patch, session.add_feature = ... does not set the GObject property, it introduces a new 'add_feature' expando property. Which means there is not much in favor of this choice...
Created attachment 232307 [details] [review] GObject: resolve method names before GObject property names For backward compatibility with Soup.Session usage. And this is option 1. As you can see, it kills most of the benefits of the GObject property cache, because it calls gjs_get_string_id() at the beginning, but object_instance_new_resolve() is not a hot call in theory. On the up side, this allows to replace Soup.Session.prototype.add_feature.call() with a simpler session.add_feature().
Which patches are supposed to be reviewed, and which are obsolete? Also, can you explain the last eight hours so I don't have to relive your experiences?
The first two AND (the third OR the forth). My experiences: With the first one, no problem, but the performance win might be less than advertised because for expando properties we always get a cache miss and a full jsid_to_string. With the second one, GObject properties start shadowing methods with the same name. Tests in gjs pass, but if you start gnome-shell it fails trying to call Soup.Session.prototype.add_feature() (which is now the add-feature property, not a method). With the third one or the forth one, tests in gjs pass and the shell starts successfully. Additionally, I'm running the shell with the forth patch now and I'm seeing the issue. As far as performance is concerned, it might be that the third one is slightly better (you can avoid a jsid_to_string because you reuse the cache built during object construction). On the other hand, the third one is definitely hackier and might introduce subtle bugs.
I'm not comfortable enough in this as a quick optimization designed to speed up conversion from UTF-16 to UTF-8.
The second one has a lot of appeal to me - the backcompat break with libsoup is an unfortunate corner case, but since gnome-shell can be fixed, I think it's worth the cost. Particularly if it's possible to change gnome-shell in such a way that it works in both cases.
Are we sure that g_utf16_to_utf8 is showing up on profiles? We probably have a lot of low-hanging fruit first.
(In reply to comment #26) > Are we sure that g_utf16_to_utf8 is showing up on profiles? Well I don't have any current data myself, but we're talking about property reads/writes which are heavily manipulated by tweener, they're definitely in the animation hot path. Avoiding calls to malloc()/free() at 60fps is quite desirable.
Created attachment 233945 [details] [review] object: restore compatibility with GObject.Class and Lang.Class Lang.Class defines a number of properties on object prototypes, which could clash with GObject properties with the same name, whose JS counterpart is not configurable. In that case, we can rely on the JS prototype chain to resolve the property, which also allows us to skip the check for custom properties. At the same time, save some memory by avoiding multiple definitions for the same GObject property.
I found also https://bugs.freedesktop.org/show_bug.cgi?id=61292, which means that moving GObject properties to prototypes, one way or the other, causes problems. I'm postponing it for now, we'll think of if next cycle.
Ok, so I just found that currently we're defining all functions with NULL, NULL operations, that is, with class operations. In other words, every time you call a method on a GObject, you're going through to GObject property path, even if we already know there is no property with that name. This is very wrong, but the moment I attempted fixing this I found that the JS engine prefers a defined property on the prototype, rather than calling the getter on the instance with nothing. So, we have these two cases: - act_user_is_loaded() vs ActUser::is-loaded gnome-shell uses .is_loaded as if it was the GObject property - soup_session_add_feature() vs SoupSession::add-feature gnome-shell knows of this bug, and calls Soup.Session.prototype.add_feature as a method (I did not check other apps, but hopefully these are the only problematic cases) This is buggy in all possible ways (we're calling gjs_get_string_id() and g_object_class_find_property() at every method call!), so we need to bite the bullet and risk breaking compatibility here. If we go with the (minor) API break, we would install GObject props on prototype chains, possibly using g-i info. We can choose, either props override methods, or the opposite. Probably it makes sense to have methods override badly named properties. Alternatively, we can install GObject props on the prototype chain, letting methods win the conflict but marking the prototype with a conflicted jsid. Then, at resolve time on the instance we notice the conflict and define a new property shadowing the one on the prototype and marshalling to the GObject property. This way, both instance.name and instance.__proto__.name.call(instance, ...) do the compatible thing. The downside of this alternative is the added complexity of implementation, and possibly worse performance.
There is an additional problem here: even if we define a property on the prototype chain, we're still getting a resolve on all derived classes, which causes a jsid conversion, for methods and GObject properties. The only way out of this is to define methods and GObject properties eagerly, but I'm not sure we want that, especially if we introduce more caching around methods.
Review of attachment 232306 [details] [review]: This one definitely won't apply anymore since it uses the *objp mechanism which doesn't exist. Since the fourth patch is an alternative, let's remove this one from the review queue.
On evaluation of the whole patch stack, I think it's better to close this bug. With SpiderMonkey 60, the getProperty and setProperty class operations are going away, and the alternatives are: - define properties eagerly - define properties lazily in resolve - use a proxy The second option we already do for boxed objects, and there's now API that gets rid of the objection that you can't pass closure data to property getter and setter operations. So I think we should do that instead.