Eric Blake <ebl...@redhat.com> 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 <arm...@redhat.com>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
>
> ---
>
> 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.
