From cc4ffe47427c8af135d93107e09dcaae95538183 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 26 Jan 2021 13:38:18 +0100 Subject: [PATCH 1/3] gsignal: use stack allocate temporary buffer in g_signal_new_valist() g_signal_new_valist() is called by g_signal_new(), which is probably the most common way to create a signal. Also, in almost all cases is the number of signal parameters small. Let's optimize for that by using a stack allocated buffer if we have few parameters. --- gobject/gsignal.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/gobject/gsignal.c b/gobject/gsignal.c index 4e50a2be2..937e106f8 100644 --- a/gobject/gsignal.c +++ b/gobject/gsignal.c @@ -1952,30 +1952,38 @@ g_signal_new_valist (const gchar *signal_name, GSignalFlags signal_flags, GClosure *class_closure, GSignalAccumulator accumulator, - gpointer accu_data, + gpointer accu_data, GSignalCMarshaller c_marshaller, GType return_type, guint n_params, va_list args) { + /* Somewhat arbitrarily reserve 200 bytes. That should cover the majority + * of cases where n_params is small and still be small enough for what we + * want to put on the stack. */ + GType param_types_stack[200 / sizeof (GType)]; + GType *param_types_heap = NULL; GType *param_types; guint i; guint signal_id; + param_types = param_types_stack; if (n_params > 0) { - param_types = g_new (GType, n_params); + if (G_UNLIKELY (n_params > G_N_ELEMENTS (param_types_stack))) + { + param_types_heap = g_new (GType, n_params); + param_types = param_types_heap; + } for (i = 0; i < n_params; i++) - param_types[i] = va_arg (args, GType); + param_types[i] = va_arg (args, GType); } - else - param_types = NULL; signal_id = g_signal_newv (signal_name, itype, signal_flags, - class_closure, accumulator, accu_data, c_marshaller, - return_type, n_params, param_types); - g_free (param_types); + class_closure, accumulator, accu_data, c_marshaller, + return_type, n_params, param_types); + g_free (param_types_heap); return signal_id; } From 7777f3bdbe7101b78ef94b262998e63f137abeb8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 26 Jan 2021 14:09:02 +0100 Subject: [PATCH 2/3] gsignal: let g_clear_signal_handler() evaluate argument only once Preferably macros behave function-like to minimize surprises. That means for example that they evaluate all arguments exactly once. Rework g_clear_signal_handler() to assign the macro parameters to auto variables so they are accessed exactly once. Also, drop the static assert for the size of (*handler_id_ptr). As we now assign to a "gulong *" pointer, the compiler already checks the types. In fact, the check is now stricter than before. Previously it would have allowed a pointer to a "signed long". This is a change in behavior of the macro and the stricter compile check could cause a build failure with broken code. Also, clear the handler id first, before calling g_signal_handler_disconnect(). Disconnecting a signal invokes the destroy notify, which can have side effects. It just feels cleaner to first reset the *_handler_id_ptr, before those side effects can happen. Of course, in practice it makes little difference. --- gobject/gsignal.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/gobject/gsignal.h b/gobject/gsignal.h index 536102dad..64aa9d6a8 100644 --- a/gobject/gsignal.h +++ b/gobject/gsignal.h @@ -451,13 +451,14 @@ void g_clear_signal_handler (gulong *handler_id_ptr, #define g_clear_signal_handler(handler_id_ptr, instance) \ G_STMT_START { \ - G_STATIC_ASSERT (sizeof *(handler_id_ptr) == sizeof (gulong)); \ - gulong _handler_id = *(handler_id_ptr); \ + gpointer const _instance = (instance); \ + gulong *const _handler_id_ptr = (handler_id_ptr); \ + const gulong _handler_id = *_handler_id_ptr; \ \ if (_handler_id > 0) \ { \ - g_signal_handler_disconnect ((instance), _handler_id); \ - *(handler_id_ptr) = 0; \ + *_handler_id_ptr = 0; \ + g_signal_handler_disconnect (_instance, _handler_id); \ } \ } G_STMT_END \ GLIB_AVAILABLE_MACRO_IN_2_62 From 8416211231db16932db1bf944d7559f8d341a3f7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 26 Jan 2021 14:16:26 +0100 Subject: [PATCH 3/3] gsignal: use g_clear_signal_handler() macro to implement g_clear_signal_handler() function We have a "good" implementation of g_clear_signal_handler() in form of a macro. Use it, and don't duplicate the code. Also add a comment to the documentation that "instance" in fact must not point to a valid GObject instance -- if the handler ID is unset. Also reword the documentation about the reasoning for why a macro version exists. The reason is not to use the function "without pointer cast". I don't think the non-macro version requires any pointer cast, since "instance" is a void pointer. Was this referring to the handler_id_ptr? That doesn't seem right either, because the caller should always provide a "gulong *" pointer and nothing else. --- gobject/gsignal.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/gobject/gsignal.c b/gobject/gsignal.c index 937e106f8..726185b95 100644 --- a/gobject/gsignal.c +++ b/gobject/gsignal.c @@ -4012,6 +4012,7 @@ g_signal_accumulator_first_wins (GSignalInvocationHint *ihint, * g_clear_signal_handler: * @handler_id_ptr: A pointer to a handler ID (of type #gulong) of the handler to be disconnected. * @instance: (type GObject.Object): The instance to remove the signal handler from. + * This pointer may be %NULL or invalid, if the handler ID is zero. * * Disconnects a handler from @instance so it will not be called during * any future or currently ongoing emissions of the signal it has been @@ -4019,21 +4020,20 @@ g_signal_accumulator_first_wins (GSignalInvocationHint *ihint, * * If the handler ID is 0 then this function does nothing. * - * A macro is also included that allows this function to be used without - * pointer casts. + * There is also a macro version of this function so that the code + * will be inlined. * * Since: 2.62 */ -#undef g_clear_signal_handler void -g_clear_signal_handler (gulong *handler_id_ptr, - gpointer instance) +(g_clear_signal_handler) (gulong *handler_id_ptr, + gpointer instance) { g_return_if_fail (handler_id_ptr != NULL); - if (*handler_id_ptr != 0) - { - g_signal_handler_disconnect (instance, *handler_id_ptr); - *handler_id_ptr = 0; - } +#ifndef g_clear_signal_handler +#error g_clear_signal_handler() macro is not defined +#endif + + g_clear_signal_handler (handler_id_ptr, instance); }