Created attachment 120959 [details] [review]
Patch

Second part of Attachment #120520 [details], but with atomic int sets.

This should/will be rewritten to use a TLS.

In case somebody is interested in reimplementing this using TLS before I do it: Bug #558927 shows how to use a TLS

Created attachment 121984 [details] [review]
Using TLS if available

This doesn't yet contain a check for configure.ac

Interesting observation:

With the patch, relatively few creation of objects:

pvanhoof@tinc:~/repos/gnome/glib$ ./test-perf 
0.284632 seconds taken in creating 1048576 instances (GstMiniObject).
0.685635 seconds taken in creating 1048576 instances (GObject).
pvanhoof@tinc:~/repos/gnome/glib$ ./test-perf 
0.270551 seconds taken in creating 1048576 instances (GstMiniObject).
0.684707 seconds taken in creating 1048576 instances (GObject).
pvanhoof@tinc:~/repos/gnome/glib$ ./test-perf 
0.252359 seconds taken in creating 1048576 instances (GstMiniObject).
0.749997 seconds taken in creating 1048576 instances (GObject).
pvanhoof@tinc:~/repos/gnome/glib$ ./test-perf 
0.247179 seconds taken in creating 1048576 instances (GstMiniObject).
0.687177 seconds taken in creating 1048576 instances (GObject).
pvanhoof@tinc:~/repos/gnome/glib$ ./test-perf 
0.252117 seconds taken in creating 1048576 instances (GstMiniObject).
0.684104 seconds taken in creating 1048576 instances (GObject).
pvanhoof@tinc:~/repos/gnome/glib$ ./test-perf 
0.268065 seconds taken in creating 1048576 instances (GstMiniObject).
0.692663 seconds taken in creating 1048576 instances (GObject).
pvanhoof@tinc:~/repos/gnome/glib$ 

Without the patch, relatively few creation of objects:

pvanhoof@tinc:~/repos/gnome/glib$ ./test-perf 
0.254135 seconds taken in creating 1048576 instances (GstMiniObject).
0.734818 seconds taken in creating 1048576 instances (GObject).
pvanhoof@tinc:~/repos/gnome/glib$ ./test-perf 
0.265501 seconds taken in creating 1048576 instances (GstMiniObject).
0.732795 seconds taken in creating 1048576 instances (GObject).
pvanhoof@tinc:~/repos/gnome/glib$ ./test-perf 
0.282394 seconds taken in creating 1048576 instances (GstMiniObject).
0.739451 seconds taken in creating 1048576 instances (GObject).
pvanhoof@tinc:~/repos/gnome/glib$ ./test-perf 
0.254251 seconds taken in creating 1048576 instances (GstMiniObject).
0.769340 seconds taken in creating 1048576 instances (GObject).
pvanhoof@tinc:~/repos/gnome/glib$ ./test-perf 
0.273943 seconds taken in creating 1048576 instances (GstMiniObject).
0.734654 seconds taken in creating 1048576 instances (GObject).
pvanhoof@tinc:~/repos/gnome/glib$ ./test-perf 
0.273254 seconds taken in creating 1048576 instances (GstMiniObject).
0.805001 seconds taken in creating 1048576 instances (GObject).
pvanhoof@tinc:~/repos/gnome/glib$ ./test-perf 
0.263972 seconds taken in creating 1048576 instances (GstMiniObject).
0.738375 seconds taken in creating 1048576 instances (GObject).
pvanhoof@tinc:~/repos/gnome/glib$ ./test-perf 
0.240664 seconds taken in creating 1048576 instances (GstMiniObject).
0.752055 seconds taken in creating 1048576 instances (GObject).
pvanhoof@tinc:~/repos/gnome/glib$ ./test-perf 
0.271343 seconds taken in creating 1048576 instances (GstMiniObject).
0.738096 seconds taken in creating 1048576 instances (GObject).
pvanhoof@tinc:~/repos/gnome/glib$ 

So this looks like a win for TLS and a loose for the gslice based GSList solution.

BUT !!


With the path, large amounts of object creation:
pvanhoof@tinc:~/repos/gnome/glib$ ./test-perf 
1.530326 seconds taken in creating 10000000 instances (GstMiniObject).
7.000411 seconds taken in creating 10000000 instances (GObject).
1.528624 seconds taken in creating 10000000 instances (GstMiniObject).
7.002537 seconds taken in creating 10000000 instances (GObject).
1.534138 seconds taken in creating 10000000 instances (GstMiniObject).
7.014217 seconds taken in creating 10000000 instances (GObject).
pvanhoof@tinc:~/repos/gnome/glib$ 

Without the path, large amounts of object creation:
pvanhoof@tinc:~/repos/gnome/glib$ ./test-perf 
1.542036 seconds taken in creating 10000000 instances (GstMiniObject).
6.741945 seconds taken in creating 10000000 instances (GObject).
1.537469 seconds taken in creating 10000000 instances (GstMiniObject).
6.749734 seconds taken in creating 10000000 instances (GObject).
1.540768 seconds taken in creating 10000000 instances (GstMiniObject).
6.744077 seconds taken in creating 10000000 instances (GObject).
pvanhoof@tinc:~/repos/gnome/glib$ 

*This*, however, looks like a loose for TLS and a win for the gslice based GSList solution.

Using callgrind makes it visible that for large amounts of object creations, the private function ___tls_get_addr starts becoming relatively increasingly expensive.

But this is just a first observation, and these are my first tests with TLS.

One important difference between the two tests is that the first time I didn't do the initial

        perf.test_mini_class (false);
       	perf.test_oclass (false);

To eliminate measuring cache differences for gtype.

This might also explain why in the first sample TLS won, and in the second sample GSList won. Meaning that actually in both cases it's possible that TLS looses.

Interesting results IMO, I didn't know ___tls_get_addr plays such a significant role.


BTW, this was the test:

/* test-perf.vala
 *
 * Copyright (C) 2008 Zeeshan Ali (Khattak) <zeeshanak@gnome.org>
 *
 * Licensed under GPLv2.
 *
 * A simple test app to compare the time it takes to create instances of struct,
 * compact class, class and class deriving from GLib.Object in Vala.
 *
 * Compile using: valac -o test-perf --pkg gstreamer-0.10 test-perf.vala
 *
 */

struct Struct {
    public uint f1;
    public double f2;
}

[Compact]
class CompactClass {
    public uint f1;
    public double f2;

    CompactClass (uint f1, double f2) {
        this.f1 = f1;
        this.f2 = f2;
    }
}

class Class {
    public uint f1;
    public double f2;

    Class (uint f1, double f2) {
        this.f1 = f1;
        this.f2 = f2;
    }
}

class MiniClass : Gst.MiniObject {
    public uint f1;
    public double f2;

    MiniClass (uint f1, double f2) {
        this.f1 = f1;
        this.f2 = f2;
    }
}

class OClass : Object {
    public uint f1;
    public double f2;

    public static OClass instantiate (uint f1, double f2) {
        OClass obj = new OClass ();
        obj.f1 = f1;
        obj.f2 = f2;

        return obj;
    }
}

class Test.Perf {
    static const uint NUM_INSTANCES = 10000000;

    // Create some instances to get rid of any first-time creation time
    Class aclass = new Class (0, 0.0);
    CompactClass acompactclass = new CompactClass (0, 0.0);
    OClass aoclass = OClass.instantiate (0, 0.0);

    Timer timer = new Timer();

    public void test_struct () {
        Struct[] structs = new Struct[NUM_INSTANCES];

        // Reset timer
        this.timer.start ();
        for (int i = 0; i < NUM_INSTANCES; i++) {
            structs[i].f1 = 0;
            structs[i].f2 = 0.0;
        }

        print ("%f seconds taken in creating %u structs.\n",
               timer.elapsed (),
               NUM_INSTANCES);
    }

    public void test_class () {
        Class[] classes = new Class[NUM_INSTANCES];

        // Reset timer
        this.timer.start ();
        for (int i = 0; i < NUM_INSTANCES; i++) {
            classes[i] = new Class (0, 0.0);
        }

        print ("%f seconds taken in creating %u instances.\n",
               timer.elapsed (),
               NUM_INSTANCES);
    }

    public void test_compact_class () {
        CompactClass[] classes = new CompactClass[NUM_INSTANCES];

        // Reset timer
        this.timer.start ();
        for (int i = 0; i < NUM_INSTANCES; i++) {
            classes[i] = new CompactClass (0, 0.0);
        }

        print ("%f seconds taken in creating %u instances (compact).\n",
               timer.elapsed (),
               NUM_INSTANCES);
    }

    public void test_mini_class (bool print_it) {
        MiniClass[] classes = new MiniClass[NUM_INSTANCES];

        // Reset timer
        this.timer.start ();
        for (int i = 0; i < NUM_INSTANCES; i++) {
            classes[i] = new MiniClass (0, 0.0);
        }

	if (print_it)
	        print ("%f seconds taken in creating %u instances (GstMiniObject).\n",
        	       timer.elapsed (),
        	       NUM_INSTANCES);
    }

    public void test_oclass (bool print_it) {
        OClass[] classes = new OClass[NUM_INSTANCES];

        // Reset timer
        this.timer.start ();
        for (int i = 0; i < NUM_INSTANCES; i++) {
            classes[i] = OClass.instantiate (0, 0.0);
        }

	if (print_it)
	        print ("%f seconds taken in creating %u instances (GObject).\n",
        	       timer.elapsed (),
        	       NUM_INSTANCES);
    }

    public static int main (string[] args) {
        Test.Perf perf = new Test.Perf ();
	int i;

        perf.test_mini_class (false);
       	perf.test_oclass (false);
	
	for (i = 0; i < 3; i++) {
	        perf.test_mini_class (true);
        	perf.test_oclass (true);
	}

        return 0;
    }
}

Any comments on this from Tim (who proposed to me to use TLS)?

Hmm, according to Company it's better to leave the bug unconfirmed

Created attachment 138348 [details] [review]
Patch reworked and reapplied on git master

All requested measurements are in the comments of this bug.

Test program as attached for Bug #557100 can be used for testing this too:
http://bugzilla.gnome.org/show_bug.cgi?id=557100#c16

Created attachment 138351 [details] [review]
Using diff -up

For your viewing pleasures

I don't understand the TLS use here. What protect against the constructor  code constructing another object, thus overwriting the tls variable?

Well, the construction_objects list could be in TLS to avoid cacheline bouncing and the lock, but it can't be a boolean.

Created attachment 145056 [details] [review]
Only add object to list new objects when it has a custom constructor

This works around the need to take a custom mutex twice and add the
object to a GSList of objects that are currently in construction for the
common case. Only when the constructor is overwritten do we use the
previous behavior and allow things like singleton objects.

The only slightly incompatible change is that previously, it was ok to
call g_object_set() on construct-only properties while the object was
initialized. This will now fail. If that behavior is needed, setting a
custom constructor that just chains up will reenable this functionality.

Comment on attachment 138351 [details] [review]
Using diff -up

Yeah, the patch is wrong. You'll get into trouble when an init function creates a new object.

(In reply to comment #11)
> Created an attachment (id=145056) [details]

> The only slightly incompatible change is that previously, it was ok to
> call g_object_set() on construct-only properties while the object was
> initialized.

Come to think about it, intiialization time is the *only* time where construct-only properties make sense to be writable, so you're effectively rendering them useles. I.e. you're simply breaking object_in_construction_list() with your patch, and thus the construct-only property checks.

> This will now fail. If that behavior is needed, setting a
> custom constructor that just chains up will reenable this functionality.

Regardless of the issues mentioned above, changing a class method function pointer only to have a different pointer value, in order to switch behaviour elsewhere in the object system is simply not what I consider a clean API.

(In reply to comment #13)
> Come to think about it, intiialization time is the *only* time where
> construct-only properties make sense to be writable, so you're effectively
> rendering them useles.
>
The only time when it makes sense to set construction time properties is when passing them to g_object_new(). This still works perfectly fine, as the code calls object_set_property() directly and bypasses this check.

What does not work anymore is calling g_object_set() or g_object_set_property() while the object is still in construction, i.e. from a custom constructor or from an instance init function.
I don't think this should be supported or should have been supported at any time ever, because it allows calling CONSTRUCT_ONLY properties twice and I'm very sure noone expects this, so a lot of code will likely leak memory or worse when this happens. It's just the only thing that used to accidentally work and does not anymore now.

Also, the comment about a workaround was meant for people that have a really messed up glib and absolutely want to work around this unsupported broken behavior. For them, I though I'll leave a trace in the commit message.

FWIW, I'm running this current desktop session with this patch and it works perfectly fine so far, so it doesn't break anywhere obvious.

(In reply to comment #14)
> The only time when it makes sense to set construction time properties is when
> passing them to g_object_new(). This still works perfectly fine, as the code
> calls object_set_property() directly and bypasses this check.

Nope that's not the only time. Calling g_object_set() in object_init is a perfectly valid way to override a construct-only property of a base class. E.g. derive MyPopup from GtkWindow and in my_popup_init, do g_object_set (self, "type", GTK_WINDOW_POPUP, NULL);

Actually, it's not, unless you also overwrite the constructor.
Otherwise, g_object_constructor() will revert it to its original value later.

Exactly, anything using plain g_object_constructor will always set the construct properties (all of them, not just the ones specified in g_object_new) after g_type_create_instance () has returned, and thus all init functions have run. So, even if g_object_set() didn't warn before it certainly did not behave in any way that would be expected. 

If you have a custom constructor you can insert code after g_object_constructor and there use g_object_set() in a sane fashion. The patch will make that case keep working though.

in master now

Turns out some code in the wild does set construct properties in the init funciton:

static void
shell_embedded_window_init (ShellEmbeddedWindow *window)
{
  window->priv = G_TYPE_INSTANCE_GET_PRIVATE (window, SHELL_TYPE_EMBEDDED_WINDOW,
                                              ShellEmbeddedWindowPrivate);

  /* Setting the resize mode to immediate means that calling queue_resize()
   * on a widget within the window will immmediately call check_resize()
   * to be called, instead of having it queued to an idle. From our perspective,
   * this is ideal since we just are going to queue a resize to Clutter's
   * idle resize anyways.
   */
  g_object_set (G_OBJECT (window),
                "resize-mode", GTK_RESIZE_IMMEDIATE,
                "type", GTK_WINDOW_POPUP,
                NULL);
}

Where "type" is:
  g_object_class_install_property (gobject_class,
                                   PROP_TYPE,
                                   g_param_spec_enum ("type",
						      P_("Window Type"),
						      P_("The type of the window"),
						      GTK_TYPE_WINDOW_TYPE,
						      GTK_WINDOW_TOPLEVEL,
						      GTK_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY));

gtk_window_get_window_type() returns GTK_WINDOW_TOPLEVEL after g_object_new returns even without the patch, so in this case the warning is actually beneficial.

So this is an application bug that should be fixed there, right?
And are our changes a problem there?

Yeah, its a gnome-shell problem, just wanted to bring it up that apps seems to do this.

Fwiw, xfce4-panel does it too : http://git.xfce.org/xfce/xfce4-panel/tree/libxfce4panel/xfce-panel-window.c#n306

I don't know if this is related to this commit or to another change, but I'm under the impression that none of the properties get set in that g_object_set call, is that possible that the call to g_object_set in _init aborts early because of the attempt at setting a construct-only property which would explain why the other properties aren't set?

(for a bit more context about my issues, people complained that with the new glib, their xfce panel got window decorations. While investigating, I came upon a startup warning about setting a construct-only property which drove me to the code I linked above. When I patch out the attempt to set the type property, the decoration are correctly hidden so this gave me the impression that the behaviour change from this patch was more that what is described in the log, but maybe it's caused by something totally different)

You are indeed correct that the code breaks out early and doesn't set further properties in g_object_set.
I'll cook up a patch that works around this issue - after pondering about the best way to do it.

Created attachment 150650 [details] [review]
Continue instead of break if property is CONSTRUCT_ONLY

This works around broken code that sets construct-only properties in the
object's init function.

I'm not sure if this patch should be applied (as it's rather kludgy) or if we can claim that the apps that trigger it (like XFCE here) are buggy and it's their fault?

Any opinions?

YFI: The Xfce panel seemed the be the only place where Xfce used g_object_set in the init function. I've already fixed this and release a new version of the panel.

(In reply to comment #26)
> I'm not sure if this patch should be applied (as it's rather kludgy) or if we
> can claim that the apps that trigger it (like XFCE here) are buggy and it's
> their fault?
> 
> Any opinions?

g_object_set of construct properties inside object_init functions is part of our working API. I.e. it is supposed to work (e.g. for specialisation of derived classes), so a new GObject version must not break it.

No, it's not. It does not work and has never worked.

(In reply to comment #29)
> No, it's not.

Well, you might have broken it of course...

> It does not work and has never worked.

The very least to sound meaningful here would be for you to provide reasoning for your claims.

We discussed this very same thing 15 comments further up already, which might explain my somewhat brusque response. See comment 17 for Alex' discussion of this very topic.

(In reply to comment #31)
> We discussed this very same thing 15 comments further up already, which might
> explain my somewhat brusque response. See comment 17 for Alex' discussion of
> this very topic.

What Alex wrote there is quite different, what he says is that after type instance creation, your construct property might be overwritten, which I think is right. Now depending on your construct property implementation, not allowing g_object_set from within object_init may or may not work have a different effect in your application (e.g. a setter implementation that has side effects or only stores the first unset->set transition value will be sensitive to this change).
It is however a break with existing API, that was my point in comment #28.

FWIW, from an OO standpoint allowing g_object_set from within object_init was meant to allow things like:
  ChildClass::ChildClass (int a) : ParentClass (a + 1) {}
The "resetting" of construct properties at the end of g_object_new is a major annoyance for that scenario though.

Yeah, enough of a annoyance to make it really really painful to exploit, and thus very unlikely in the wild. Which is pointed out further by the only two things hitting this so far did so by mistake and were bugs only now exposed.

Of course the xcfe issue affects things other than the construct property in question though, so I'm not sure the best way to handle that. The original approach is really way to costly for something that de-facto has proabaly only been used by mistake ever in the lifetime of gobject, so i don't think going back to that is right, so the alternatives is to do a hack like benjamins patch, or to allow this very slight break in API.

Fwiw, the XFCE developers have already removed the problematic construct from their code:
http://bugzilla.xfce.org/show_bug.cgi?id=6110

We use this in gEDA to set the GtkWindow "type" property. With recent GLib versions, we get debugging spew.

We previously had something like:

  g_object_set (G_OBJECT (window),
                "type", GTK_WINDOW_POPUP,
                NULL);

in the instance_init function.

What is the correct way to to this now. I couldn't find anything in devhelp.

Note that this code did not do anything at all, so you can in theory just delete it. (See comments 14-17 if you care about details.)

Should you still want to set this property correctly when constructing, you want to pass it as an argument to g_object_new().

I take your point that the existing code never worked, but I would like to know how to fix it!

And no, unfortunately I can't pass it to g_obejct_new()

The class needs to override the property on its parent class - and it would be pretty rubbish if our API interface specified that the caller must always set that property for us.

What is the correct way to achieve this _within_ the class?

There is no proper way to change default values for classes currently. Bug 587256 has a similar problem.
Various ugly hacks can be done to achieve this, but I''m not gonna suggest any of them here.
If you want to override properties, the supported thing that you should do is provide a my_object_new() function that calls g_object_new (MY_TYPE_OBJECT, "first-property-to-override", new-value, ..., NULL);

Yuck, that is horrible - all the callers just exploit g_object_new() directly. I'm not going to pretend it causes extra grief for language-bindings - as we don't have any, but obviously there is a defficiency here.

Thanks for the suggestion though.

Personally (and our code reflects this) I'd have assumed we could change construct only properties up until the point the object was constructed. Is there any plan to fix this behaviour in the future?

The docs aren't particulary forthcoming here either though... so if the plan is not to fix this, it would be good to document it more clearly. It is a shame there are alrady things like GTK in the wild using these properties. People would probably bother less with them if their limitations were better documented.

My goal is to have a way to override default values using something like g_object_class_override_property_default_value (object_class, property_name, new_default_value); in the class_init function.

But I haven't had a good idea on how to implement this in a non-ugly way that doesn't break somewhere yet, so I didn't bother trying.
I definitely do want it though.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/166.