[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v13 01/14] qobject: Simplify QObject
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v13 01/14] qobject: Simplify QObject |
Date: |
Thu, 26 Nov 2015 15:27:11 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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.
- Re: [Qemu-devel] [PATCH v13 10/14] qapi: Track enum values by QAPISchemaMember, not string, (continued)
- [Qemu-devel] [PATCH v13 11/14] qapi: Populate info['name'] for each entity, Eric Blake, 2015/11/20
- [Qemu-devel] [PATCH v13 09/14] qapi: Prepare new QAPISchemaMember base class, Eric Blake, 2015/11/20
- [Qemu-devel] [PATCH v13 14/14] qapi: Detect base class loops, Eric Blake, 2015/11/20
- [Qemu-devel] [PATCH v13 04/14] qapi: Simplify visiting of alternate types, Eric Blake, 2015/11/20
- [Qemu-devel] [PATCH v13 12/14] qapi: Enforce (or whitelist) case conventions on qapi members, Eric Blake, 2015/11/20
- [Qemu-devel] [PATCH v13 01/14] qobject: Simplify QObject, Eric Blake, 2015/11/20
- [Qemu-devel] [PATCH v13 02/14] qobject: Rename qtype_code to QType, Eric Blake, 2015/11/20
- [Qemu-devel] [PATCH v13 06/14] qapi: Fix alternates that accept 'number' but not 'int', Eric Blake, 2015/11/20
- [Qemu-devel] [PATCH v13 13/14] qapi: Move duplicate collision checks to schema check(), Eric Blake, 2015/11/20
- Re: [Qemu-devel] [PATCH v13 00/14] qapi member collision (post-introspection cleanups, subset D), Markus Armbruster, 2015/11/27