Re: [Qemu-devel] [PATCH v13 01/14] qobject: Simplify QObject

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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>
[...]
> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
> index 4b96ed5..7e5b9b1 100644
> --- a/include/qapi/qmp/qobject.h
> +++ b/include/qapi/qmp/qobject.h
> @@ -47,21 +47,20 @@ typedef enum {
>      QTYPE_MAX,
>  } qtype_code;
>
> -struct QObject;
> -
> -typedef struct QType {
> -    qtype_code code;
> -    void (*destroy)(struct QObject *);
> -} QType;
> -
>  typedef struct QObject {
> -    const QType *type;
> +    qtype_code type;
>      size_t refcnt;
>  } QObject;

Future opportunity: uint32_t refcnt should be more than enough.  Halves
the size to eight bytes.

> +typedef void (*QDestroy)(QObject *);
> +

Typedef used exactly once, in qobject.c.  Let's drop it.

>  /* Get the 'base' part of an object */
>  #define QOBJECT(obj) (&(obj)->base)
>
> +/* High-level interface for qobject_init() */
> +#define QOBJECT_INIT(obj, type) \
> +    qobject_init(QOBJECT(obj), type)
> +

Not really high-level; it's used only internally, and its users are the
high-level constructors.

I wouldn't mind dropping the macro.

>  /* High-level interface for qobject_incref() */
>  #define QINCREF(obj)      \
>      qobject_incref(QOBJECT(obj))
> @@ -71,9 +70,12 @@ typedef struct QObject {
>      qobject_decref(obj ? QOBJECT(obj) : NULL)
>
>  /* Initialize an object to default values */
> -#define QOBJECT_INIT(obj, qtype_type)   \
> -    obj->base.refcnt = 1;               \
> -    obj->base.type   = qtype_type
> +static inline void qobject_init(QObject *obj, qtype_code type)
> +{
> +    assert(type);

QTYPE_NONE < type && type < QTYPE_MAX would be tighter.

Aside: I dislike our use of _MAX as the last enum.  _MAX should be the
largest *valid* value (see: INT_MAX), not one more.

> +    obj->refcnt = 1;
> +    obj->type = type;
> +}
>
>  /**
>   * qobject_incref(): Increment QObject's reference count
> @@ -85,6 +87,11 @@ static inline void qobject_incref(QObject *obj)
>  }
>
>  /**
> + * qobject_destroy(): Free resources used by the object
> + */
> +void qobject_destroy(QObject *obj);
> +
> +/**
>   * qobject_decref(): Decrement QObject's reference count, deallocate
>   * when it reaches zero
>   */
> @@ -92,9 +99,7 @@ static inline void qobject_decref(QObject *obj)
>  {
>      assert(!obj || obj->refcnt);
>      if (obj && --obj->refcnt == 0) {
> -        assert(obj->type != NULL);
> -        assert(obj->type->destroy != NULL);
> -        obj->type->destroy(obj);
> +        qobject_destroy(obj);
>      }
>  }
>
> @@ -103,8 +108,8 @@ static inline void qobject_decref(QObject *obj)
>   */
>  static inline qtype_code qobject_type(const QObject *obj)
>  {
> -    assert(obj->type != NULL);
> -    return obj->type->code;
> +    assert(obj->type);

Likewise.

> +    return obj->type;
>  }
>
>  extern QObject qnull_;
> 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 bc6535f..895fd4d 100644
> --- a/qobject/qbool.c
> +++ b/qobject/qbool.c
> @@ -15,13 +15,6 @@
>  #include "qapi/qmp/qobject.h"
>  #include "qemu-common.h"
>
> -static void qbool_destroy_obj(QObject *obj);
> -
> -static const QType qbool_type = {
> -    .code = QTYPE_QBOOL,
> -    .destroy = qbool_destroy_obj,
> -};
> -
>  /**
>   * qbool_from_bool(): Create a new QBool from a bool
>   *
> @@ -33,7 +26,7 @@ QBool *qbool_from_bool(bool value)
>
>      qb = g_malloc(sizeof(*qb));
>      qb->value = value;
> -    QOBJECT_INIT(qb, &qbool_type);
> +    QOBJECT_INIT(qb, QTYPE_QBOOL);

Good opportunity to clean up the order: init goes first.

>
>      return qb;
>  }
> @@ -61,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 2d67bf1..dd3f1c2 100644
> --- a/qobject/qdict.c
> +++ b/qobject/qdict.c
> @@ -19,13 +19,6 @@
>  #include "qemu/queue.h"
>  #include "qemu-common.h"
>
> -static void qdict_destroy_obj(QObject *obj);
> -
> -static const QType qdict_type = {
> -    .code = QTYPE_QDICT,
> -    .destroy = qdict_destroy_obj,
> -};
> -
>  /**
>   * qdict_new(): Create a new QDict
>   *
> @@ -36,7 +29,7 @@ QDict *qdict_new(void)
>      QDict *qdict;
>
>      qdict = g_malloc0(sizeof(*qdict));
> -    QOBJECT_INIT(qdict, &qdict_type);
> +    QOBJECT_INIT(qdict, QTYPE_QDICT);
>
>      return qdict;
>  }
> @@ -441,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 c865163..1dffb54 100644
> --- a/qobject/qfloat.c
> +++ b/qobject/qfloat.c
> @@ -15,13 +15,6 @@
>  #include "qapi/qmp/qobject.h"
>  #include "qemu-common.h"
>
> -static void qfloat_destroy_obj(QObject *obj);
> -
> -static const QType qfloat_type = {
> -    .code = QTYPE_QFLOAT,
> -    .destroy = qfloat_destroy_obj,
> -};
> -
>  /**
>   * qfloat_from_int(): Create a new QFloat from a float
>   *
> @@ -33,7 +26,7 @@ QFloat *qfloat_from_double(double value)
>
>      qf = g_malloc(sizeof(*qf));
>      qf->value = value;
> -    QOBJECT_INIT(qf, &qfloat_type);
> +    QOBJECT_INIT(qf, QTYPE_QFLOAT);


Likewise.

>
>      return qf;
>  }
> @@ -61,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 999688e..112ee68 100644
> --- a/qobject/qint.c
> +++ b/qobject/qint.c
> @@ -14,13 +14,6 @@
>  #include "qapi/qmp/qobject.h"
>  #include "qemu-common.h"
>
> -static void qint_destroy_obj(QObject *obj);
> -
> -static const QType qint_type = {
> -    .code = QTYPE_QINT,
> -    .destroy = qint_destroy_obj,
> -};
> -
>  /**
>   * qint_from_int(): Create a new QInt from an int64_t
>   *
> @@ -32,7 +25,7 @@ QInt *qint_from_int(int64_t value)
>
>      qi = g_malloc(sizeof(*qi));
>      qi->value = value;
> -    QOBJECT_INIT(qi, &qint_type);
> +    QOBJECT_INIT(qi, QTYPE_QINT);

Likewise.

>
>      return qi;
>  }
> @@ -60,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 298003a..28be4f6 100644
> --- a/qobject/qlist.c
> +++ b/qobject/qlist.c
> @@ -15,13 +15,6 @@
>  #include "qemu/queue.h"
>  #include "qemu-common.h"
>
> -static void qlist_destroy_obj(QObject *obj);
> -
> -static const QType qlist_type = {
> -    .code = QTYPE_QLIST,
> -    .destroy = qlist_destroy_obj,
> -};
> - 
>  /**
>   * qlist_new(): Create a new QList
>   *
> @@ -33,7 +26,7 @@ QList *qlist_new(void)
>
>      qlist = g_malloc(sizeof(*qlist));
>      QTAILQ_INIT(&qlist->head);
> -    QOBJECT_INIT(qlist, &qlist_type);
> +    QOBJECT_INIT(qlist, QTYPE_QLIST);
>
>      return qlist;
>  }
> @@ -151,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 9873e26..5f7ba4d 100644
> --- a/qobject/qnull.c
> +++ b/qobject/qnull.c
> @@ -13,17 +13,7 @@
>  #include "qemu-common.h"
>  #include "qapi/qmp/qobject.h"
>
> -static void qnull_destroy_obj(QObject *obj)
> -{
> -    assert(0);
> -}
> -
> -static const QType qnull_type = {
> -    .code = QTYPE_QNULL,
> -    .destroy = qnull_destroy_obj,
> -};
> -
>  QObject qnull_ = {
> -    .type = &qnull_type,
> +    .type = QTYPE_QNULL,
>      .refcnt = 1,
>  };
> diff --git a/qobject/qobject.c b/qobject/qobject.c
> new file mode 100644
> index 0000000..5325447
> --- /dev/null
> +++ b/qobject/qobject.c
> @@ -0,0 +1,34 @@
> +/*
> + * 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"
> +
> +static QDestroy qdestroy[QTYPE_MAX] = {
> +    [QTYPE_NONE] = NULL, /* No such object exists */
> +    [QTYPE_QNULL] = NULL, /* qnull_ is indestructible */

I'd prefer to have the comments off to the right, like this:

  +    [QTYPE_NONE] = NULL,       /* No such object exists */
  +    [QTYPE_QNULL] = NULL,      /* qnull_ is indestructible */

> +    [QTYPE_QINT] = qint_destroy_obj,
> +    [QTYPE_QSTRING] = qstring_destroy_obj,
> +    [QTYPE_QDICT] = qdict_destroy_obj,
> +    [QTYPE_QLIST] = qlist_destroy_obj,
> +    [QTYPE_QFLOAT] = qfloat_destroy_obj,
> +    [QTYPE_QBOOL] = qbool_destroy_obj,
> +};
> +
> +void qobject_destroy(QObject *obj)
> +{
> +    assert(!obj->refcnt);
> +    assert(QTYPE_QNULL < obj->type && obj->type < QTYPE_MAX);
> +    qdestroy[obj->type](obj);
> +}
> diff --git a/qobject/qstring.c b/qobject/qstring.c
> index cb72dfb..a2f5903 100644
> --- a/qobject/qstring.c
> +++ b/qobject/qstring.c
> @@ -14,13 +14,6 @@
>  #include "qapi/qmp/qstring.h"
>  #include "qemu-common.h"
>
> -static void qstring_destroy_obj(QObject *obj);
> -
> -static const QType qstring_type = {
> -    .code = QTYPE_QSTRING,
> -    .destroy = qstring_destroy_obj,
> -};
> -
>  /**
>   * qstring_new(): Create a new empty QString
>   *
> @@ -57,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, &qstring_type);
> +    QOBJECT_INIT(qstring, QTYPE_QSTRING);

Another "init goes first".

>
>      return qstring;
>  }
> @@ -138,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;

Patch is basically fine.