GNOME Bugzilla – Bug 597260
[performance] g_object_new checks 3 times that the GType is a GObject type
Last modified: 2010-01-11 17:38:31 UTC
Currently when calling g_object_new(), the code will check 3 times that the provided GType is of the correct type (i.e. a G_TYPE_OBJECT subtype). g_object_new() CHECK_TYPE call g_object_new_valist g_object_new_valist() CHECK_TYPE call g_object_newv g_object_newv() CHECK_TYPE The proposed patch does the following: (1) Do no check for the type in g_object_new. It will be done later on, g_object_new being just a thin wrapper over g_object_new_valist (2) Split g_object_newv into g_object_newv_checked (which doesn't do the type check and is a private function) and g_object_newv (which is public, does the type check and then calls the _checked variant) (3) Leave the type check in g_object_new_valist, but call the new g_object_newv_checked variant which doesn't need to do the check
Created attachment 144699 [details] [review] gobject.c: Reduce number of times we check for valid GType. Fixes #597260 * Don't check that the provided GType is GObject sub-type in g_object_new, just call g_object_new_valist directly. * Split g_object_newv into two methods: * g_object_newv_checked which is the same code as the previous g_object_newv, expect that it is now private and doesn't check for the GType. * g_object_newv, which does the type check and calls the new _checked variant. * Modify g_object_new_valist so that it now calls the _checked variant.
ping ?
Care to cite some numbers about the performance win? I think that performance gain has to compete against the ugliness of the patch when deciding if it is a good idea to apply it. And I think we agree that the patch makes the code more ugly. FWIW, G_TYPE_IS_OBJECT is O(1) - a function call, some shifts and some reads, so it shouldn't be that bad. Also, I'm not sure if it's a good idea to omit the check in g_object_new(). Usually the checks are there in every public API call, because the return_if_fail macros print the function name and it might be confusing to developers if passing the wrong type to g_object_new() would complain about passing the wrong type to g_object_new_valist().
(In reply to comment #3) > Care to cite some numbers about the performance win? Not really. citing numbers are interesting for complex modifications where it's not obvious that there is a gain, here it's plain simple that there will be a gain. As for the performance gain not being high enough, why would that matter ? I'm pretty sure if people start looking at it (like some of us have), we'd see plenty of optimisations to be done like this. Pile up enough... and you're starting to get interesting speedups. > I think that performance gain has to compete against the ugliness of the patch > when deciding if it is a good idea to apply it. And I think we agree that the > patch makes the code more ugly. > FWIW, G_TYPE_IS_OBJECT is O(1) - a function call, some shifts and some reads, > so it shouldn't be that bad. > > Also, I'm not sure if it's a good idea to omit the check in g_object_new(). > Usually the checks are there in every public API call, because the > return_if_fail macros print the function name and it might be confusing to > developers if passing the wrong type to g_object_new() would complain about > passing the wrong type to g_object_new_valist(). I thought about that... but the patch would have been even uglier. The only thing this patch does is put into one single location a g_object_newv that doesn't require checking.
Created attachment 150626 [details] [review] Improve g_object_new() performance Apply the following performance improvements while retaining identical object type checking behavior: - g_object_new() avoids useless varargs processing for simple calls - g_object_new() checks object type once instead of 3 times - g_object_new_valist() checks object type once instead of 2 times - g_object_newv_internal() avoids calling g_free(NULL)
Edward Hervey's patch (attachment 144699 [details] [review]) does indeed provide a nice performance gain, as is demonstrated by the test program glib/tests/gobject/performance. With Edward's patch, the 'simple-construction' test runs 4.3% faster on my machine. I noticed the same problem Edward reported while examining another performance problem with g_object_new(): It calls g_object_new_valist() (and therefore va_start/end()) even when there is no valist -- which will be the case for the majority of calls, I think. My proposed patch (attachment 150626 [details] [review]) lets g_object_new() skip that pointless varargs processing for simple calls where no varargs properties are supplied. It also makes g_object_new() and _valist() check the object type only once, like Edward's. But my version does NOT move the check out of g_object_new() so any type warnings will still refer to "g_object_new" -- no change to the API-level behavior here. Performance result: With my patch, the 'simple-construction' test runs 19.9% faster on my machine. I am surprised that the improvement is so dramatic, and I invite others to replicate the test -- I suspect that CPU-cache locality issues are in play here. Test: $ glib/tests/gobject/performance simple-construction Tester: Kamal Mostafa <kamal@whence.com> Date: Thu, 31 Dec 2009 14:58:41 -0800 Machine: Intel Q9550 quad-core @ 2.83GHz / Ubuntu 9.10 amd64 Compile: -g -O2 (standard build) gobject.c Number of constructed version objects per second: comparison --------------------- ----------------------- -------------- 2.23.2 unmodified 10167085 baseline Edward Hervey patch 10614228 4.3% faster https://bugzilla.gnome.org/attachment.cgi?id=144699 Kamal Mostafa patch 12192442 19.9% faster https://bugzilla.gnome.org/attachment.cgi?id=150626
That second patch is indeed cleaner. I've given up trusting the performance test though, it's just too unreliable. I use callgrind and then compare the number of instruction calls in the various methods to have a much more accurate estimation of the speedup.
It'd have been nice if you had made the 3 separate fixes separate commits (and ideally had tested them separately). I think the 3-liner ignoring the valist creation is a nice optimization, and it doesn't make the code a lot more complex either. So if that is a large part of the optimization I'd be happy to commit it. I don't think not calling g_free() is a worthy optimization (in particular because it uglifies the code).
(In reply to comment #8) > I think the 3-liner ignoring the valist creation is a nice optimization, and it > doesn't make the code a lot more complex either. So if that is a large part of > the optimization I'd be happy to commit it. I tested it, and yes just skipping the valist processing delivers a 15% performance increase for the simple-construction test on my machine. Since I we're all in agreement that it is a good optimization -- but it doesn't resolve *this* bug report -- I have opened a separate bug 605883 (g_object_new() processes varargs even when there are none). I propose that we go ahead and commit that one, and then continue our discussion on the original issue here.
Created attachment 150667 [details] [review] [PATCH] g_object_newv() skips pointless g_free(NULL)
Created attachment 150668 [details] [review] [PATCH] g_object_new() checks object type just once Use new routine g_object_newv_internal() for these performance improvements: - g_object_new() checks object type once instead of 3 times - g_object_new_valist() checks object type once instead of 2 times Fixes https://bugzilla.gnome.org/show_bug.cgi?id=597260
(In reply to comment #8) > It'd have been nice if you had made the 3 separate fixes separate commits (and > ideally had tested them separately). As requested, the fixes have been split into three patches. Tested separately, here were the performance results of the three fixes versus the baseline code: just skip valist processing 15% faster (now bug 605883) just check type only once 10% faster (attachment 150668 [details] [review]) just skip pointless g_free() 4% faster (attachment 150667 [details] [review]) (Note that the second test "check type only once" is effectively the method proposed by Edward in comment #1, but would print the wrong function name in warnings). > I don't think not calling g_free() is a worthy optimization (in particular > because it uglifies the code). Beauty is in the eye of the beholder I guess. In my opinion, a "pretty" routine should call g_free() the same number of times that it called g_new(). But in the most common calling case, the current g_object_newv() doesn't call g_new() yet it always calls g_free(NULL) once.
Review of attachment 150668 [details] [review]: NOTE: This patch fits on top of the patch attachment 150664 [details] [review] for bug 605883.
Review of attachment 150667 [details] [review]: I agree with Benjamin that this is not an improvement. It adds a pointless extra branch to the code.
Thanks Benjamin for committing the "skip valist processing" patch. The "check type only once" patch for this bug (attachment 150668 [details] [review]) is still awaiting review -- it would resolve this bug report.
I think this patch uglifies the code too much without noticable real-world gains. If you really need the performance you can compile with defining G_DISABLE_CHECKS and get the performance boost without ugly code. So I'm gonna close this.