Created attachment 313237 [details] [review]
wayland-surface: disconnect signals on destroy

Otherwise signal handlers will be called on garbage

Review of attachment 313237 [details] [review]:

Hm, not g_signal_connect_object?

(In reply to Jasper St. Pierre from comment #2)
> Review of attachment 313237 [details] [review] [review]:
> 
> Hm, not g_signal_connect_object?

You still want to disconnect when you change output, might as well disconnect when you're destroyed...

I really don't like storing the signal ID in the hash map without renaming it. It seems unobvious and sketchy to me.

Created attachment 313239 [details] [review]
wayland-surface: disconnect signals on destroy

Otherwise signal handlers will be called on garbage

Ok, I renamed the hash table. I would like to avoid
g_signal_handlers_disconnect_by_func because it's really slow.

Created attachment 313240 [details] [review]
wayland-surface: disconnect signals on destroy

Otherwise signal handlers will be called on garbage

Ok, I renamed the hash table. I would like to avoid
g_signal_handlers_disconnect_by_func because it's really slow.

(In reply to Giovanni Campagna from comment #6)
> Created attachment 313240 [details] [review] [review]
> wayland-surface: disconnect signals on destroy
> 
> Otherwise signal handlers will be called on garbage
> 
> Ok, I renamed the hash table. I would like to avoid
> g_signal_handlers_disconnect_by_func because it's really slow.

Will the speed of disconnect_by_func really ever be of any significance though? It's particularly insignificant when changing outputs since it's only one surface disconnecting from a single output. I don't really see any point from a performance perspective of doing this.

Anyway, assuming we still doing this, the name "outputs_to_signal" makes no sense to me, since there are no signals in the map. It's now a output_destroy_handler_ids map that double as a output set, so maybe just call it that?

I've seen several slowdowns in various apps caused by g_signal_handlers_disconnect_by_func at the wrong time, so even though I can't measure it I think it's just a good idea to avoid it.

Now, I seem to recall that at some point it was O(n) in the total number of signal connections, not just the signal connections on the instance. That does not seem to be the case now, looking at the code, so maybe it's not that ugly.

(It is still linear on the number of connections on the output, which means that destroying an output would have cost quadratic on the number of windows on that output. Probably not a big deal: even with > 20 windows, at worst it might cause us to miss a frame.)

Nevertheless, I would prefer to use g_signal_handler_disconnect when it's easy enough, just as a matter of good practice.
But I'm not the maintainer of this code, so I'll stick to whatever you prefer - either a renamed hash table or the old disconnect_by_func.

Review of attachment 313240 [details] [review]:

::: src/wayland/meta-wayland-surface.c
@@ +947,3 @@
+                             surface);
+      g_hash_table_replace (surface->outputs_to_signal, wayland_output,
+                            GSIZE_TO_POINTER ((gsize)id));

This should be a g_hash_table_insert right? Because we will never replace the entry, and if we did, we'd leak a handler.

(In reply to Giovanni Campagna from comment #8)
> I've seen several slowdowns in various apps caused by
> g_signal_handlers_disconnect_by_func at the wrong time, so even though I
> can't measure it I think it's just a good idea to avoid it.
> 
> Now, I seem to recall that at some point it was O(n) in the total number of
> signal connections, not just the signal connections on the instance. That
> does not seem to be the case now, looking at the code, so maybe it's not
> that ugly.
> 
> (It is still linear on the number of connections on the output, which means
> that destroying an output would have cost quadratic on the number of windows
> on that output. Probably not a big deal: even with > 20 windows, at worst it
> might cause us to miss a frame.)
> 
> Nevertheless, I would prefer to use g_signal_handler_disconnect when it's
> easy enough, just as a matter of good practice.
> But I'm not the maintainer of this code, so I'll stick to whatever you
> prefer - either a renamed hash table or the old disconnect_by_func.

I'm not a maintainer either, and since you are the one writing the code, I'd say lets go with your approach. I'd only prefer not having something else than "to_signal" as a prefix only because the map has no signals.

(In reply to Jonas Ådahl from comment #9)
> Review of attachment 313240 [details] [review] [review]:
> 
> ::: src/wayland/meta-wayland-surface.c
> @@ +947,3 @@
> +                             surface);
> +      g_hash_table_replace (surface->outputs_to_signal, wayland_output,
> +                            GSIZE_TO_POINTER ((gsize)id));
> 
> This should be a g_hash_table_insert right? Because we will never replace
> the entry, and if we did, we'd leak a handler.

insert and replace are the same thing, they only differ in the memory management of the key

(In reply to Giovanni Campagna from comment #11)
> (In reply to Jonas Ådahl from comment #9)
> > Review of attachment 313240 [details] [review] [review] [review]:
> > 
> > ::: src/wayland/meta-wayland-surface.c
> > @@ +947,3 @@
> > +                             surface);
> > +      g_hash_table_replace (surface->outputs_to_signal, wayland_output,
> > +                            GSIZE_TO_POINTER ((gsize)id));
> > 
> > This should be a g_hash_table_insert right? Because we will never replace
> > the entry, and if we did, we'd leak a handler.
> 
> insert and replace are the same thing, they only differ in the memory
> management of the key

Sure, but I was more talking about readability here. And I guess since it's guaranteed that we will never have an entry here, the memory management would effectively be the same as well?

Created attachment 313693 [details] [review]
wayland-surface: disconnect signals on destroy

Otherwise signal handlers will be called on garbage

Review of attachment 313693 [details] [review]:

Looks good to me.

Attachment 313693 [details] pushed as a4f763a - wayland-surface: disconnect signals on destroy