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 690450 - GObject: avoid resolving property names for all accesses
GObject: avoid resolving property names for all accesses
Status: RESOLVED WONTFIX
Product: gjs
Classification: Bindings
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gjs-maint
gjs-maint
Depends on:
Blocks:
 
 
Reported: 2012-12-18 20:28 UTC by Giovanni Campagna
Modified: 2018-01-21 22:16 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
GObject: avoid resolving property names for all accesses (8.24 KB, patch)
2012-12-18 20:28 UTC, Giovanni Campagna
reviewed Details | Review
GObject: avoid resolving property names for all accesses (15.71 KB, patch)
2012-12-19 00:43 UTC, Giovanni Campagna
reviewed Details | Review
GObject: avoid resolving property names for all accesses (15.72 KB, patch)
2012-12-24 01:24 UTC, Giovanni Campagna
accepted-commit_now Details | Review
GObject: avoid resolving property names for all accesses (15.51 KB, patch)
2012-12-27 16:24 UTC, Giovanni Campagna
none Details | Review
GObject: avoid resolving property names for all accesses (16.08 KB, patch)
2012-12-27 17:48 UTC, Giovanni Campagna
none Details | Review
object: define GObject properties at resolve time instead of hooking get and set (8.06 KB, patch)
2012-12-27 21:02 UTC, Giovanni Campagna
none Details | Review
object: only resolve GObject properties when starting from instances (3.99 KB, patch)
2012-12-27 21:40 UTC, Giovanni Campagna
rejected Details | Review
GObject: resolve method names before GObject property names (3.48 KB, patch)
2012-12-27 21:49 UTC, Giovanni Campagna
none Details | Review
object: restore compatibility with GObject.Class and Lang.Class (3.25 KB, patch)
2013-01-20 14:29 UTC, Giovanni Campagna
none Details | Review

Description Giovanni Campagna 2012-12-18 20:28:43 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.
Comment 1 Giovanni Campagna 2012-12-18 20:28:47 UTC
Created attachment 231831 [details] [review]
GObject: avoid resolving property names for all accesses
Comment 2 Colin Walters 2012-12-18 21:25:29 UTC
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?
Comment 3 Giovanni Campagna 2012-12-18 21:37:04 UTC
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.
Comment 4 Colin Walters 2012-12-18 22:22:25 UTC
(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?
Comment 5 Giovanni Campagna 2012-12-18 22:28:19 UTC
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...)
Comment 6 Colin Walters 2012-12-18 22:45:11 UTC
(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 '_'?
Comment 7 Giovanni Campagna 2012-12-18 22:54:58 UTC
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.
Comment 8 Colin Walters 2012-12-18 23:14:38 UTC
(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?
Comment 9 Colin Walters 2012-12-18 23:18:48 UTC
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?
Comment 10 Giovanni Campagna 2012-12-19 00:43:41 UTC
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.
Comment 11 Colin Walters 2012-12-19 11:13:33 UTC
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()?
Comment 12 Giovanni Campagna 2012-12-22 18:24:51 UTC
(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.
Comment 13 Colin Walters 2012-12-22 20:05:44 UTC
(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?
Comment 14 Giovanni Campagna 2012-12-24 01:24:42 UTC
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.
Comment 15 Colin Walters 2012-12-24 03:29:45 UTC
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.
Comment 16 Giovanni Campagna 2012-12-27 16:24:38 UTC
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.
Comment 17 Giovanni Campagna 2012-12-27 17:48:37 UTC
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...
Comment 18 Giovanni Campagna 2012-12-27 21:02:08 UTC
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.
Comment 19 Giovanni Campagna 2012-12-27 21:23:07 UTC
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)
Comment 20 Giovanni Campagna 2012-12-27 21:40:06 UTC
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...
Comment 21 Giovanni Campagna 2012-12-27 21:49:46 UTC
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().
Comment 22 Jasper St. Pierre (not reading bugmail) 2012-12-28 01:09:45 UTC
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?
Comment 23 Giovanni Campagna 2012-12-28 01:28:11 UTC
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.
Comment 24 Jasper St. Pierre (not reading bugmail) 2012-12-28 01:33:28 UTC
I'm not comfortable enough in this as a quick optimization designed to speed up conversion from UTF-16 to UTF-8.
Comment 25 Colin Walters 2012-12-29 22:57:38 UTC
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.
Comment 26 Jasper St. Pierre (not reading bugmail) 2012-12-29 23:01:10 UTC
Are we sure that g_utf16_to_utf8 is showing up on profiles?

We probably have a lot of low-hanging fruit first.
Comment 27 Colin Walters 2012-12-29 23:56:07 UTC
(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.
Comment 28 Giovanni Campagna 2013-01-20 14:29:58 UTC
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.
Comment 29 Giovanni Campagna 2013-02-22 16:59:59 UTC
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.
Comment 30 Giovanni Campagna 2013-05-10 17:01:32 UTC
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.
Comment 31 Giovanni Campagna 2013-05-10 20:52:19 UTC
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.
Comment 32 Philip Chimento 2018-01-21 19:09:17 UTC
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.
Comment 33 Philip Chimento 2018-01-21 22:16:20 UTC
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.