Created attachment 158411 [details] [review]
Small benchmark notify() Vs notify_by_pspec()

Patch to test notify_by_pspec() speed up over notify()

$ ./performance notify notify-pspec
Running test notify
Notifys per second: 1416769
Running test notify-pspec
NotifyPspecs per second: 1626134

I like the patch, and I think the new idiomatic way of registering properties it introduces should be the preferred one - and it should be included in the GObject documentation as well.

it might probably get more traction if the savings were higher, though on libraries that heavily revolve around property notification (like Clutter) I think we should take all the 10~15% savings we can get.

I think you should mention in the documentation of notify_by_pspec that it is much faster than g_object_notify and also add a similar note to g_object_notify.

Review of attachment 158410 [details] [review]:

::: gobject/gobject.c
@@ +905,3 @@
+ * @pspec: the #GParamSpec of a property installed on the class of @object.
+ * @object: a #GObject
+ * g_object_notify_by_pspec:

documentation ideas:

"""
This function omits the property name lookup, hence it is faster than g_object_notify().

One way to avoid using g_object_notify() from within the class that registered the properties, and using g_object_notify_by_pspec() instead, is to store the GParamSpec used with g_object_class_install_property() inside a static array, e.g.:

|[
  enum {
    PROP_0,
    PROP_FOO,
    PROP_LAST
  };

  static GParamSpec *properties[PROP_LAST];

  static void
  my_object_class_init (MyObjectClass *klass)
  {
    properties[PROP_FOO] = g_param_spec_int ("foo", "Foo", "The foo",
                                             0, 100,
                                             50,
                                             G_PARAM_READWRITE);
    g_object_class_install_property (klass,
                                     PROP_FOO,
                                     properties[PROP_FOO]);
  }
]|

similarly to how signal identifiers are stored.
"""

If we are going to change the recommended way properties are handled, maybe we should go the whole way and put the properties array into the class structure.

That would allow for using the param-spec for g_object_[gs]et* in the future.

Along the lines of¹

  enum {
    PROP_0,
    PROP_FOO,
    PROP_LAST
  };

  struct FooClass {
    GObjectClass parent_class;
    ParamSpec *properties[PROP_LAST];
  };

The actual call g_object_notify_by_pspec() would change a bit, but I guess a new template macro along the lines of FOO_GET_PARAM_SPEC(obj, id) would help.

What do you think Damien? Worth a shot?

¹ For libraries' public headers those would obviously have to be namespaced.

(In reply to comment #5)
> If we are going to change the recommended way properties are handled, maybe we
> should go the whole way and put the properties array into the class structure.

why would you even want to do that? for sub-classes?

that's impressively bad. we can now have private data structures inside GTypeClass structures - making the GParamSpec public would be a massive breach of encapsulation.

it's like putting the signal id inside the Class vtable so that sub-classes can call g_signal_emit() instead of g_signal_emit_by_name().

(In reply to comment #6)
> why would you even want to do that? for sub-classes?

Not for subclasses, but so code using the class can call g_object_get/set_by_pspec() and also save the hash lookup overhead.

The idea of putting the pspec array into the class private data is interesting too. If you still kept the property enum in the header that would allow for g_object_get/set_by_id().

Both would be an improvement over the current string-based API.

(In reply to comment #7)
> (In reply to comment #6)
> > why would you even want to do that? for sub-classes?
> 
> Not for subclasses, but so code using the class can call
> g_object_get/set_by_pspec() and also save the hash lookup overhead.

no, that's the wrong approach, and it would solve the wrong problem.

the issue with g_object_set()/_get() is not the string lookup - though that has its impact, it is dominated by the GValue marshalling and demarshalling; that's why typed accessors like g_object_set_<type>() (where <type> is any fundamental GType) are planned. removing the 10% contribution of the string lookup would still leave the remaining (warning: hyperbole, I don't have up to date profiles) 50% of the GValue marshalling and demarshalling.

internally, we should probably port GParamSpecPool away from a hash table and use interned strings instead; also, per-class storage instead of a global storage. but this is for another bug.

Created attachment 162491 [details] [review]
Introduce g_object_notify_by_pspec() take 2

Updated patch with documentation enhancements suggested by both Christian and Emmanuele.

Review of attachment 162491 [details] [review]:

Looks good to me.

::: gobject/gobject.c
@@ +900,3 @@
   else
     {
+      g_object_notify_by_spec_internal (object, pspec);

Should remove the {} here.

Created attachment 163665 [details] [review]
Introduce g_object_notify_by_pspec() take 3

Updated patch with unnecessary {}s removed

Pushed to master