[Spice-devel] [PATCH 14/19] Replace RedsPrivate::char_devs_state with a GList

[Jonathon Jongsma]

On Fri, 2016-02-19 at 07:00 -0500, Frediano Ziglio wrote:
> > 
> > From: Christophe Fergeau <cfergeau at redhat.com>
> > 
> > The code was introducing an intermediate SpiceCharDevStateItem type to
> > hold linked list elements, resulting in a memory handling behaviour very
> > similar to a GList. Using GList directly makes the code shorter and more
> > readable.
> 
> Did we ever considered that ring_remove is O(1) while g_list_remove is O(n) ?
> Well, in this case it's not a problem.

Yes, I did consider this, and I think that for the cases I converted this
performance issue does not matter much. They're generally not on a 'hot' path,
and/or the lists are of a very small size. But there may be cases where you
disagree with my analysis. 

Of course, now I see that this is Christophe's patch and not mine. But I'm quite
sure he considered it as well ;)


> 
> Frediano
> 
> > ---
> >  server/reds-private.h |  7 +------
> >  server/reds.c         | 41 ++++++++++-------------------------------
> >  2 files changed, 11 insertions(+), 37 deletions(-)
> > 
> > diff --git a/server/reds-private.h b/server/reds-private.h
> > index f567929..beb9d35 100644
> > --- a/server/reds-private.h
> > +++ b/server/reds-private.h
> > @@ -125,11 +125,6 @@ typedef struct RedsMigWaitDisconnectClient {
> >      RedClient *client;
> >  } RedsMigWaitDisconnectClient;
> >  
> > -typedef struct SpiceCharDeviceStateItem {
> > -    RingItem link;
> > -    SpiceCharDeviceState *st;
> > -} SpiceCharDeviceStateItem;
> > -
> >  /* Intermediate state for on going monitors config message from a single
> >   * client, being passed to the guest */
> >  typedef struct RedsClientMonitorsConfig {
> > @@ -187,7 +182,7 @@ struct RedsState {
> >      SpiceTimer *mig_timer;
> >  
> >      int vm_running;
> > -    Ring char_devs_states; /* list of SpiceCharDeviceStateItem */
> > +    GList *char_devices; /* list of SpiceCharDeviceState */
> >      int seamless_migration_enabled; /* command line arg */
> >      int keepalive_timeout;
> >  
> > diff --git a/server/reds.c b/server/reds.c
> > index 73074e4..1349285 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -3089,28 +3089,13 @@ SPICE_GNUC_VISIBLE const char**
> > spice_server_char_device_recognized_subtypes(voi
> >  
> >  static void reds_char_device_add_state(RedsState *reds,
> > SpiceCharDeviceState
> >  *st)
> >  {
> > -    SpiceCharDeviceStateItem *item = spice_new0(SpiceCharDeviceStateItem,
> > 1);
> > -
> > -    item->st = st;
> > -
> > -    ring_add(&reds->char_devs_states, &item->link);
> > +    reds->char_devices = g_list_append(reds->char_devices, st);
> >  }
> >  
> >  static void reds_char_device_remove_state(RedsState *reds,
> >  SpiceCharDeviceState *st)
> >  {
> > -    RingItem *item;
> > -
> > -    RING_FOREACH(item, &reds->char_devs_states) {
> > -        SpiceCharDeviceStateItem *st_item;
> > -
> > -        st_item = SPICE_CONTAINEROF(item, SpiceCharDeviceStateItem, link);
> > -        if (st_item->st == st) {
> > -            ring_remove(item);
> > -            free(st_item);
> > -            return;
> > -        }
> > -    }
> > -    spice_error("char dev state not found %p", st);
> > +    g_warn_if_fail(g_list_find(reds->char_devices, st) != NULL);
> > +    reds->char_devices = g_list_remove(reds->char_devices, st);
> >  }
> >  
> >  void reds_on_char_device_state_destroy(RedsState *reds,
> > SpiceCharDeviceState
> >  *dev)
> > @@ -3382,7 +3367,7 @@ static int do_spice_init(RedsState *reds,
> > SpiceCoreInterface *core_interface)
> >      reds->main_dispatcher = main_dispatcher_new(reds, reds->core);
> >      ring_init(&reds->channels);
> >      ring_init(&reds->mig_target_clients);
> > -    ring_init(&reds->char_devs_states);
> > +    reds->char_devices = NULL;
> >      ring_init(&reds->mig_wait_disconnect_clients);
> >      reds->vm_running = TRUE; /* for backward compatibility */
> >  
> > @@ -4003,28 +3988,22 @@ SPICE_GNUC_VISIBLE int
> > spice_server_migrate_switch(SpiceServer *reds)
> >  
> >  SPICE_GNUC_VISIBLE void spice_server_vm_start(SpiceServer *reds)
> >  {
> > -    RingItem *item;
> > +    GList *it;
> >  
> >      reds->vm_running = TRUE;
> > -    RING_FOREACH(item, &reds->char_devs_states) {
> > -        SpiceCharDeviceStateItem *st_item;
> > -
> > -        st_item = SPICE_CONTAINEROF(item, SpiceCharDeviceStateItem, link);
> > -        spice_char_device_start(st_item->st);
> > +    for (it = reds->char_devices; it != NULL; it = it->next) {
> > +        spice_char_device_start((SpiceCharDeviceState *)it->data);
> >      }
> >      reds_on_vm_start(reds);
> >  }
> >  
> >  SPICE_GNUC_VISIBLE void spice_server_vm_stop(SpiceServer *reds)
> >  {
> > -    RingItem *item;
> > +    GList *it;
> >  
> >      reds->vm_running = FALSE;
> > -    RING_FOREACH(item, &reds->char_devs_states) {
> > -        SpiceCharDeviceStateItem *st_item;
> > -
> > -        st_item = SPICE_CONTAINEROF(item, SpiceCharDeviceStateItem, link);
> > -        spice_char_device_stop(st_item->st);
> > +    for (it = reds->char_devices; it != NULL; it = it->next) {
> > +        spice_char_device_stop((SpiceCharDeviceState *)it->data);
> >      }
> >      reds_on_vm_stop(reds);
> >  }
> > --
> > 2.5.0
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel