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

[Eric Blake]

[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);
     }
 }

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, */
+};
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;

-- 
2.4.3