Created attachment 242603 [details] [review]
gobject: don't use a global, locked list for constructing GObjects

Private data is a lot cheaper these days, so it's time for GObject
to grow a private and store there that constructed() hasn't run
yet, avoiding linear behavior, serialization and micro allocations
when multiple threads are constructing objects.

Created attachment 242604 [details] [review]
gobject: don't use GDataList for private data

Now that we have a private structure, we can use it for toggle
references, notify queues, closure arrays and weak references.
This way, access to GQuark data for library user is faster, as
there are fewer items to compare against, and fewer reallocations
as the datalist becomes bigger.
Note that this increases the memory consumption, because now
all objects have a slot for these. Memory is cheap these days
(cheaper than CPU time), and we're talking of 32 bytes, so this
should not be a problem in practice.

Created attachment 242605 [details] [review]
gobject: don't use the real g_type_instance_get_private()

We're inside glib, so can make assumptions on the behavior of
GType, and optimize private structure access even more.

Comment on attachment 242603 [details] [review]
gobject: don't use a global, locked list for constructing GObjects

mostly nitpicks...

>+  volatile int in_construction;

glib still prefers "gint"

>+  g_type_class_add_private (class, sizeof(GObjectPrivate));

space after sizeof

>+      priv = G_TYPE_INSTANCE_GET_PRIVATE (object, G_TYPE_OBJECT, GObjectPrivate);

probably should define G_OBJECT_GET_PRIVATE()

>+      /* No need for atomic calls here, this function is called only once for object, and
>+         it's not possible object is used at the same in other code
>+      */
>+      priv->in_construction = 1;

/* align comment asterisks
 * like this
 */

I think the comment is right, but can someone else confirm that not having a memory barrier here after writing it is fine, since we always have a memory barrier before reading it?

Oh, I'd use "TRUE" rather than 1, even though it's a gint.

>+  return g_atomic_int_get (&priv->in_construction) != 0;

likewise TRUE/FALSE throughout


if committed, this patch also fixes bug 661576

I'm not crazy about this patchset.

In particular, the in_construction list is no longer used during normal object construction, so I have no interest in making it fast -- and certainly not at the expense of enlarging the size of every single GObject instance.

In general, I feel that a lot of the patches here enlarge GObject with very little gain....

Review of attachment 242603 [details] [review]:

This is permanently growing every single GObject by some amount, for a minor performance gain during construction.
Are we sure this is worth it ? An alternative might be to temporarily abuse a high bit in the refcount, or fill the 4 byte hole between the ref_count and the qdata:

struct _GObject {
	GTypeInstance              g_type_instance;      /*     0     8 */
	volatile guint             ref_count;            /*     8     4 */

	/* XXX 4 bytes hole, try to pack */

	GData *                    qdata;                /*    16     8 */

	/* size: 24, cachelines: 1, members: 3 */
	/* sum members: 20, holes: 1, sum holes: 4 */
	/* last cacheline: 24 bytes */
};

Review of attachment 242604 [details] [review]:

::: gobject/gobject.c
@@ +205,3 @@
+  volatile int        in_construction;
+};
+

Ugh, now it grows even more.
This is turning GObject into a much more heavy-weight object.
All of these where only allocated when the features are actually used, before.

I think we don't want to do this.  Removing the hacks would have been nice, but not at the cost of increasing the size of every GObject.