Re: [Qemu-devel] [PATCH] fixup? qapi: Simplify QObject

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> [If we squash, replace the old commit message with this; you'll
> also have some merge conflicts in 29/36 and 30/36.  Note that
> while this mail is net lines added, squashing would still result
> in a net reduction:
>  16 files changed, 67 insertions(+), 83 deletions(-)
> ]
>
> qapi: Simplify QObject
>
> The QObject hierarchy is small enough, and unlikely to grow further
> (since we only use it to map to JSON and already cover all JSON
> types), that we can simplify things by not tracking a separate
> vtable, but just inline the refcnt element of the vtable QType
> directly into QObject, and track a separate array of destroy
> functions.  We can drop qnull_destroy_obj() in the process.
>
> The remaining QObject subclasses must export their destructor.
>
> This also has the nice benefit of moving the typename 'QType'
> out of the way, so that the next patch can repurpose it for a
> nicer name for 'qtype_code'.
>
> The various objects are still the same size (so no change in cache
> line pressure), but now have less indirection (although I didn't
> bother benchmarking to see if there is a noticeable speedup, as
> we don't have hard evidence that this was in a performance hotspot
> in the first place).
>
> Suggested-by: Markus Armbruster <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
>
> If we don't want to squash, I can instead rebase this to appear
> after 30/36 as a standalone patch, with a message something like:
>
> qapi: Don't duplicate destructor pointer per QObject
>
> The previous patch "qapi: Simplify QObject" made each QObject
> instance larger; restore the original size for less cache line
> pressure by moving the destroy function pointer out of each
> object and instead maintaining a global table of destroy
> functions.  This requires exporting the various functions,
> and is made easier to read through a typedef.
> ---
>  include/qapi/qmp/qbool.h   |  1 +
>  include/qapi/qmp/qdict.h   |  1 +
>  include/qapi/qmp/qfloat.h  |  1 +
>  include/qapi/qmp/qint.h    |  1 +
>  include/qapi/qmp/qlist.h   |  1 +
>  include/qapi/qmp/qobject.h | 16 ++++++++--------
>  include/qapi/qmp/qstring.h |  1 +
>  qobject/Makefile.objs      |  2 +-
>  qobject/qbool.c            |  6 ++----
>  qobject/qdict.c            |  6 ++----
>  qobject/qfloat.c           |  6 ++----
>  qobject/qint.c             |  6 ++----
>  qobject/qlist.c            |  6 ++----
>  qobject/qnull.c            |  1 -
>  qobject/qobject.c          | 26 ++++++++++++++++++++++++++
>  qobject/qstring.c          |  6 ++----
>  16 files changed, 53 insertions(+), 34 deletions(-)
>  create mode 100644 qobject/qobject.c
>
> diff --git a/include/qapi/qmp/qbool.h b/include/qapi/qmp/qbool.h
> index d9256e4..836d078 100644
> --- a/include/qapi/qmp/qbool.h
> +++ b/include/qapi/qmp/qbool.h
> @@ -25,5 +25,6 @@ typedef struct QBool {
>  QBool *qbool_from_bool(bool value);
>  bool qbool_get_bool(const QBool *qb);
>  QBool *qobject_to_qbool(const QObject *obj);
> +void qbool_destroy_obj(QObject *obj);
>
>  #endif /* QBOOL_H */
> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
> index 787c658..6c2a0e5 100644
> --- a/include/qapi/qmp/qdict.h
> +++ b/include/qapi/qmp/qdict.h
> @@ -48,6 +48,7 @@ void qdict_iter(const QDict *qdict,
>                  void *opaque);
>  const QDictEntry *qdict_first(const QDict *qdict);
>  const QDictEntry *qdict_next(const QDict *qdict, const QDictEntry *entry);
> +void qdict_destroy_obj(QObject *obj);
>
>  /* Helper to qdict_put_obj(), accepts any object */
>  #define qdict_put(qdict, key, obj) \
> diff --git a/include/qapi/qmp/qfloat.h b/include/qapi/qmp/qfloat.h
> index 46745e5..a8af2a8 100644
> --- a/include/qapi/qmp/qfloat.h
> +++ b/include/qapi/qmp/qfloat.h
> @@ -25,5 +25,6 @@ typedef struct QFloat {
>  QFloat *qfloat_from_double(double value);
>  double qfloat_get_double(const QFloat *qi);
>  QFloat *qobject_to_qfloat(const QObject *obj);
> +void qfloat_destroy_obj(QObject *obj);
>
>  #endif /* QFLOAT_H */
> diff --git a/include/qapi/qmp/qint.h b/include/qapi/qmp/qint.h
> index 339a9ab..049e528 100644
> --- a/include/qapi/qmp/qint.h
> +++ b/include/qapi/qmp/qint.h
> @@ -24,5 +24,6 @@ typedef struct QInt {
>  QInt *qint_from_int(int64_t value);
>  int64_t qint_get_int(const QInt *qi);
>  QInt *qobject_to_qint(const QObject *obj);
> +void qint_destroy_obj(QObject *obj);
>
>  #endif /* QINT_H */
> diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h
> index b1bf785..a84117e 100644
> --- a/include/qapi/qmp/qlist.h
> +++ b/include/qapi/qmp/qlist.h
> @@ -49,6 +49,7 @@ QObject *qlist_peek(QList *qlist);
>  int qlist_empty(const QList *qlist);
>  size_t qlist_size(const QList *qlist);
>  QList *qobject_to_qlist(const QObject *obj);
> +void qlist_destroy_obj(QObject *obj);
>
>  static inline const QListEntry *qlist_first(const QList *qlist)
>  {
> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
> index 83ed70b..4e89a7f 100644
> --- a/include/qapi/qmp/qobject.h
> +++ b/include/qapi/qmp/qobject.h
> @@ -52,15 +52,17 @@ typedef struct QObject QObject;
>  struct QObject {
>      qtype_code type;
>      size_t refcnt;
> -    void (*destroy)(QObject *);
>  };
>
> +typedef void (*QDestroy)(QObject *);
> +extern QDestroy qdestroy[QTYPE_MAX];
> +
>  /* Get the 'base' part of an object */
>  #define QOBJECT(obj) (&(obj)->base)
>
>  /* High-level interface for qobject_init() */
> -#define QOBJECT_INIT(obj, type, destroy) \
> -    qobject_init(QOBJECT(obj), type, destroy)
> +#define QOBJECT_INIT(obj, type) \
> +    qobject_init(QOBJECT(obj), type)
>
>  /* High-level interface for qobject_incref() */
>  #define QINCREF(obj)      \
> @@ -71,13 +73,11 @@ struct QObject {
>      qobject_decref(obj ? QOBJECT(obj) : NULL)
>
>  /* Initialize an object to default values */
> -static inline void qobject_init(QObject *obj, qtype_code type,
> -                                void (*destroy)(struct QObject *))
> +static inline void qobject_init(QObject *obj, qtype_code type)
>  {
>      assert(type);
>      obj->refcnt = 1;
>      obj->type = type;
> -    obj->destroy = destroy;
>  }
>
>  /**
> @@ -97,8 +97,8 @@ static inline void qobject_decref(QObject *obj)
>  {
>      assert(!obj || obj->refcnt);
>      if (obj && --obj->refcnt == 0) {
> -        assert(obj->destroy);
> -        obj->destroy(obj);
> +        assert(qdestroy[obj->type]);
> +        qdestroy[obj->type](obj);
>      }
>  }
>

Preexisting: the assertion is of little value, because all it does is
convert one kind of crash into another.

Asserting obj->type is in range might be more useful, i.e.

  -        assert(obj->destroy);
  -        obj->destroy(obj);
  +        assert(QTYPE_QNULL < obj->type && obj->type < QTYPE_MAX);
  +        qdestroy[obj->type](obj);

We could outline the conditional part, and keep qdestroy[] static.
Could save a bit of code (unless NDEBUG, but we shouldn't optimize for
that).

> diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
> index 34675a7..df7df55 100644
> --- a/include/qapi/qmp/qstring.h
> +++ b/include/qapi/qmp/qstring.h
> @@ -32,5 +32,6 @@ void qstring_append_int(QString *qstring, int64_t value);
>  void qstring_append(QString *qstring, const char *str);
>  void qstring_append_chr(QString *qstring, int c);
>  QString *qobject_to_qstring(const QObject *obj);
> +void qstring_destroy_obj(QObject *obj);
>
>  #endif /* QSTRING_H */
> diff --git a/qobject/Makefile.objs b/qobject/Makefile.objs
> index 0031e8b..bed5508 100644
> --- a/qobject/Makefile.objs
> +++ b/qobject/Makefile.objs
> @@ -1,2 +1,2 @@
>  util-obj-y = qnull.o qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o
> -util-obj-y += qjson.o json-lexer.o json-streamer.o json-parser.o
> +util-obj-y += qjson.o qobject.o json-lexer.o json-streamer.o json-parser.o
> diff --git a/qobject/qbool.c b/qobject/qbool.c
> index 9f46e67..895fd4d 100644
> --- a/qobject/qbool.c
> +++ b/qobject/qbool.c
> @@ -15,8 +15,6 @@
>  #include "qapi/qmp/qobject.h"
>  #include "qemu-common.h"
>
> -static void qbool_destroy_obj(QObject *obj);
> -
>  /**
>   * qbool_from_bool(): Create a new QBool from a bool
>   *
> @@ -28,7 +26,7 @@ QBool *qbool_from_bool(bool value)
>
>      qb = g_malloc(sizeof(*qb));
>      qb->value = value;
> -    QOBJECT_INIT(qb, QTYPE_QBOOL, qbool_destroy_obj);
> +    QOBJECT_INIT(qb, QTYPE_QBOOL);
>
>      return qb;
>  }
> @@ -56,7 +54,7 @@ QBool *qobject_to_qbool(const QObject *obj)
>   * qbool_destroy_obj(): Free all memory allocated by a
>   * QBool object
>   */
> -static void qbool_destroy_obj(QObject *obj)
> +void qbool_destroy_obj(QObject *obj)
>  {
>      assert(obj != NULL);
>      g_free(qobject_to_qbool(obj));
> diff --git a/qobject/qdict.c b/qobject/qdict.c
> index cf62269..dd3f1c2 100644
> --- a/qobject/qdict.c
> +++ b/qobject/qdict.c
> @@ -19,8 +19,6 @@
>  #include "qemu/queue.h"
>  #include "qemu-common.h"
>
> -static void qdict_destroy_obj(QObject *obj);
> -
>  /**
>   * qdict_new(): Create a new QDict
>   *
> @@ -31,7 +29,7 @@ QDict *qdict_new(void)
>      QDict *qdict;
>
>      qdict = g_malloc0(sizeof(*qdict));
> -    QOBJECT_INIT(qdict, QTYPE_QDICT, qdict_destroy_obj);
> +    QOBJECT_INIT(qdict, QTYPE_QDICT);
>
>      return qdict;
>  }
> @@ -436,7 +434,7 @@ void qdict_del(QDict *qdict, const char *key)
>  /**
>   * qdict_destroy_obj(): Free all the memory allocated by a QDict
>   */
> -static void qdict_destroy_obj(QObject *obj)
> +void qdict_destroy_obj(QObject *obj)
>  {
>      int i;
>      QDict *qdict;
> diff --git a/qobject/qfloat.c b/qobject/qfloat.c
> index 2472189..1dffb54 100644
> --- a/qobject/qfloat.c
> +++ b/qobject/qfloat.c
> @@ -15,8 +15,6 @@
>  #include "qapi/qmp/qobject.h"
>  #include "qemu-common.h"
>
> -static void qfloat_destroy_obj(QObject *obj);
> -
>  /**
>   * qfloat_from_int(): Create a new QFloat from a float
>   *
> @@ -28,7 +26,7 @@ QFloat *qfloat_from_double(double value)
>
>      qf = g_malloc(sizeof(*qf));
>      qf->value = value;
> -    QOBJECT_INIT(qf, QTYPE_QFLOAT, qfloat_destroy_obj);
> +    QOBJECT_INIT(qf, QTYPE_QFLOAT);
>
>      return qf;
>  }
> @@ -56,7 +54,7 @@ QFloat *qobject_to_qfloat(const QObject *obj)
>   * qfloat_destroy_obj(): Free all memory allocated by a
>   * QFloat object
>   */
> -static void qfloat_destroy_obj(QObject *obj)
> +void qfloat_destroy_obj(QObject *obj)
>  {
>      assert(obj != NULL);
>      g_free(qobject_to_qfloat(obj));
> diff --git a/qobject/qint.c b/qobject/qint.c
> index 765307f..112ee68 100644
> --- a/qobject/qint.c
> +++ b/qobject/qint.c
> @@ -14,8 +14,6 @@
>  #include "qapi/qmp/qobject.h"
>  #include "qemu-common.h"
>
> -static void qint_destroy_obj(QObject *obj);
> -
>  /**
>   * qint_from_int(): Create a new QInt from an int64_t
>   *
> @@ -27,7 +25,7 @@ QInt *qint_from_int(int64_t value)
>
>      qi = g_malloc(sizeof(*qi));
>      qi->value = value;
> -    QOBJECT_INIT(qi, QTYPE_QINT, qint_destroy_obj);
> +    QOBJECT_INIT(qi, QTYPE_QINT);
>
>      return qi;
>  }
> @@ -55,7 +53,7 @@ QInt *qobject_to_qint(const QObject *obj)
>   * qint_destroy_obj(): Free all memory allocated by a
>   * QInt object
>   */
> -static void qint_destroy_obj(QObject *obj)
> +void qint_destroy_obj(QObject *obj)
>  {
>      assert(obj != NULL);
>      g_free(qobject_to_qint(obj));
> diff --git a/qobject/qlist.c b/qobject/qlist.c
> index 1e2098a..28be4f6 100644
> --- a/qobject/qlist.c
> +++ b/qobject/qlist.c
> @@ -15,8 +15,6 @@
>  #include "qemu/queue.h"
>  #include "qemu-common.h"
>
> -static void qlist_destroy_obj(QObject *obj);
> -
>  /**
>   * qlist_new(): Create a new QList
>   *
> @@ -28,7 +26,7 @@ QList *qlist_new(void)
>
>      qlist = g_malloc(sizeof(*qlist));
>      QTAILQ_INIT(&qlist->head);
> -    QOBJECT_INIT(qlist, QTYPE_QLIST, qlist_destroy_obj);
> +    QOBJECT_INIT(qlist, QTYPE_QLIST);
>
>      return qlist;
>  }
> @@ -146,7 +144,7 @@ QList *qobject_to_qlist(const QObject *obj)
>  /**
>   * qlist_destroy_obj(): Free all the memory allocated by a QList
>   */
> -static void qlist_destroy_obj(QObject *obj)
> +void qlist_destroy_obj(QObject *obj)
>  {
>      QList *qlist;
>      QListEntry *entry, *next_entry;
> diff --git a/qobject/qnull.c b/qobject/qnull.c
> index 22d59e9..5f7ba4d 100644
> --- a/qobject/qnull.c
> +++ b/qobject/qnull.c
> @@ -16,5 +16,4 @@
>  QObject qnull_ = {
>      .type = QTYPE_QNULL,
>      .refcnt = 1,
> -    .destroy = NULL,
>  };
> diff --git a/qobject/qobject.c b/qobject/qobject.c
> new file mode 100644
> index 0000000..db86571
> --- /dev/null
> +++ b/qobject/qobject.c
> @@ -0,0 +1,26 @@
> +/*
> + * QObject
> + *
> + * Copyright (C) 2015 Red Hat, Inc.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1
> + * or later.  See the COPYING.LIB file in the top-level directory.
> + */
> +
> +#include "qemu-common.h"
> +#include "qapi/qmp/qbool.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qfloat.h"
> +#include "qapi/qmp/qint.h"
> +#include "qapi/qmp/qlist.h"
> +#include "qapi/qmp/qstring.h"
> +
> +QDestroy qdestroy[QTYPE_MAX] = {
> +    [QTYPE_QBOOL] = qbool_destroy_obj,
> +    [QTYPE_QDICT] = qdict_destroy_obj,
> +    [QTYPE_QFLOAT] = qfloat_destroy_obj,
> +    [QTYPE_QINT] = qint_destroy_obj,
> +    [QTYPE_QLIST] = qlist_destroy_obj,
> +    [QTYPE_QSTRING] = qstring_destroy_obj,
> +    /* [QTYPE_QNULL] = NULL, */
> +};

Suggest

   QDestroy qdestroy[QTYPE_MAX] = {
       [QTYPE_QNULL] = NULL,       /* no such object exists */
       [QTYPE_QNULL] = NULL,       /* qnull_ is indestructible */
       ...
   };

> diff --git a/qobject/qstring.c b/qobject/qstring.c
> index ba59d00..a2f5903 100644
> --- a/qobject/qstring.c
> +++ b/qobject/qstring.c
> @@ -14,8 +14,6 @@
>  #include "qapi/qmp/qstring.h"
>  #include "qemu-common.h"
>
> -static void qstring_destroy_obj(QObject *obj);
> -
>  /**
>   * qstring_new(): Create a new empty QString
>   *
> @@ -52,7 +50,7 @@ QString *qstring_from_substr(const char *str, int start, 
> int end)
>      memcpy(qstring->string, str + start, qstring->length);
>      qstring->string[qstring->length] = 0;
>
> -    QOBJECT_INIT(qstring, QTYPE_QSTRING, qstring_destroy_obj);
> +    QOBJECT_INIT(qstring, QTYPE_QSTRING);
>
>      return qstring;
>  }
> @@ -133,7 +131,7 @@ const char *qstring_get_str(const QString *qstring)
>   * qstring_destroy_obj(): Free all memory allocated by a QString
>   * object
>   */
> -static void qstring_destroy_obj(QObject *obj)
> +void qstring_destroy_obj(QObject *obj)
>  {
>      QString *qs;

If you'd like to respin, I'd prefer a non-incremental [PATCH v12.5
28/36] replacing v12.

If not, I'm happy to squash in trivial changes like the one I suggested
for qdestroy[]'s initializer.