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