diff mbox

[v8,35/35] RFC: qapi: Adjust layout of FooList types

Message ID 1450717720-9627-36-git-send-email-eblake@redhat.com
State New
Headers show

Commit Message

Eric Blake Dec. 21, 2015, 5:08 p.m. UTC 
By sticking the next pointer first, we don't need a union with
64-bit padding for smaller types.  On 32-bit platforms, this
can reduce the size of uint8List from 16 bytes (or 12, depending
on whether 64-bit ints can tolerate 4-byte alignment) down to 8.
It has no effect on 64-bit platforms (where alignment still
dictates a 16-byte struct); but fewer anonymous unions is still
a win in my book.

However, this requires visit_start_list() and visit_next_list()
to gain a size parameter, to know what size element to allocate.

I debated about going one step further, to allow for fewer casts,
by doing:
    typedef GenericList GenericList;
    struct GenericList {
        GenericList *next;
    };
    struct FooList {
        GenericList base;
        Foo value;
    };
so that you convert to 'GenericList *' by '&foolist->base', and
back by 'container_of(generic, GenericList, base)' (as opposed to
the existing '(GenericList *)foolist' and '(FooList *)generic').
But doing that would require hoisting the declaration of
GenericList prior to inclusion of qapi-types.h, rather than its
current spot in visitor.h; it also makes iteration a bit more
verbose through 'foolist->base.next' instead of 'foolist->next'.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v8: rebase to earlier changes
v7: new patch; probably too invasive to be worth it, especially if
we can't prove that our current size for uint8List is a bottleneck
---
 hw/ppc/spapr_drc.c           |  2 +-
 include/qapi/visitor-impl.h  |  5 +++--
 include/qapi/visitor.h       | 39 +++++++++++++++++++--------------------
 qapi/opts-visitor.c          |  9 +++++----
 qapi/qapi-dealloc-visitor.c  |  5 +++--
 qapi/qapi-visit-core.c       | 14 +++++++++-----
 qapi/qmp-input-visitor.c     |  8 ++++----
 qapi/qmp-output-visitor.c    |  5 +++--
 qapi/string-input-visitor.c  |  9 +++++----
 qapi/string-output-visitor.c |  5 +++--
 scripts/qapi-types.py        |  5 +----
 scripts/qapi-visit.py        |  4 ++--
 12 files changed, 58 insertions(+), 52 deletions(-)

Comments

Marc-André Lureau Jan. 5, 2016, 5:22 p.m. UTC | #1 
Hi

On Mon, Dec 21, 2015 at 6:08 PM, Eric Blake <eblake@redhat.com> wrote:
> By sticking the next pointer first, we don't need a union with
> 64-bit padding for smaller types.  On 32-bit platforms, this
> can reduce the size of uint8List from 16 bytes (or 12, depending
> on whether 64-bit ints can tolerate 4-byte alignment) down to 8.
> It has no effect on 64-bit platforms (where alignment still
> dictates a 16-byte struct); but fewer anonymous unions is still
> a win in my book.

small win, but a win

> However, this requires visit_start_list() and visit_next_list()
> to gain a size parameter, to know what size element to allocate.

If this is the only "drawback", I'd be fine with it.

> I debated about going one step further, to allow for fewer casts,
> by doing:
>     typedef GenericList GenericList;
>     struct GenericList {
>         GenericList *next;
>     };
>     struct FooList {
>         GenericList base;
>         Foo value;
>     };
> so that you convert to 'GenericList *' by '&foolist->base', and
> back by 'container_of(generic, GenericList, base)' (as opposed to
> the existing '(GenericList *)foolist' and '(FooList *)generic').
> But doing that would require hoisting the declaration of
> GenericList prior to inclusion of qapi-types.h, rather than its
> current spot in visitor.h; it also makes iteration a bit more
> verbose through 'foolist->base.next' instead of 'foolist->next'.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>

otherwise, patch looks good to me, waiting for non-rfc version if all
agree with it, for the review-by tag.
diff mbox

Patch

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index e3e2127..13bd8b0 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -299,7 +299,7 @@  static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
             int i;
             prop = fdt_get_property_by_offset(fdt, fdt_offset, &prop_len);
             name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
-            visit_start_list(v, name, NULL, &err);
+            visit_start_list(v, name, NULL, 0, &err);
             if (err) {
                 error_propagate(errp, err);
                 return;
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index c32e5f5..a7df148 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -41,9 +41,10 @@  struct Visitor

     /* Must be set */
     bool (*start_list)(Visitor *v, const char *name, GenericList **list,
-                       Error **errp);
+                       size_t size, Error **errp);
     /* Must be set */
-    GenericList *(*next_list)(Visitor *v, GenericList *element, Error **errp);
+    GenericList *(*next_list)(Visitor *v, GenericList *element, size_t size,
+                              Error **errp);
     /* Must be set */
     void (*end_list)(Visitor *v);

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 4eae633..c446726 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -56,11 +56,8 @@ 
  * created by the qapi generator. */
 typedef struct GenericList
 {
-    union {
-        void *value;
-        uint64_t padding;
-    };
     struct GenericList *next;
+    char padding[];
 } GenericList;

 /**
@@ -130,19 +127,19 @@  void visit_end_implicit_struct(Visitor *v);
 /**
  * Prepare to visit a list tied to an object key @name.
  * @name will be NULL if this is visited as part of another list.
- * Input visitors malloc a qapi List struct into *@list, or set it to
- * NULL if there are no elements in the list; and output visitors
- * expect *@list to point to the start of the list, if any.  On
- * return, if *@list is non-NULL, the caller should enter a loop
+ * Input visitors malloc a qapi List struct into *@list of size @size,
+ * or set it to NULL if there are no elements in the list; and output
+ * visitors expect *@list to point to the start of the list, if any.
+ * On return, if *@list is non-NULL, the caller should enter a loop
  * visiting the current element, then using visit_next_list() to
  * advance to the next element, until that returns NULL; then
  * visit_end_list() must be used to complete the visit.
  *
- * If supported by a visitor, @list can be NULL to indicate that there
- * is no qapi List struct, and that the upcoming visit calls are
- * parsing input to or creating output from some other representation;
- * in this case, visit_next_list() will not be needed, but
- * visit_end_list() is still mandatory.
+ * If supported by a visitor, @list can be NULL (and @size 0) to
+ * indicate that there is no qapi List struct, and that the upcoming
+ * visit calls are parsing input to or creating output from some other
+ * representation; in this case, visit_next_list() will not be needed,
+ * but visit_end_list() is still mandatory.
  *
  * Returns true if *@list was allocated; if that happens, and an error
  * occurs any time before the matching visit_end_list(), then the
@@ -150,17 +147,19 @@  void visit_end_implicit_struct(Visitor *v);
  * allocation before returning control further.
  */
 bool visit_start_list(Visitor *v, const char *name, GenericList **list,
-                      Error **errp);
+                      size_t size, Error **errp);
 /**
  * Iterate over a GenericList during a list visit.
  * Before calling this function, the caller should use the appropriate
- * visit_type_FOO() for the current list element at @element->value, and
- * check for errors. @element must not be NULL; on the first iteration,
- * it should be the value in *list after visit_start_list(); on other
- * calls it should be the previous return value.  This function
- * returns NULL once there are no further list elements.
+ * visit_type_FOO() for the current list element at @element->value,
+ * and check for errors. @element must not be NULL; on the first
+ * iteration, it should be the value in *list after
+ * visit_start_list(); on other calls it should be the previous return
+ * value.  @size should be the same as for visit_start_list().  This
+ * function returns NULL once there are no further list elements.
  */
-GenericList *visit_next_list(Visitor *v, GenericList *element, Error **errp);
+GenericList *visit_next_list(Visitor *v, GenericList *element, size_t size,
+                             Error **errp);
 /**
  * Complete the list started earlier.
  * Must be called after any successful use of visit_start_list(),
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 38d1e68..28f9a8a 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -214,7 +214,8 @@  lookup_distinct(const OptsVisitor *ov, const char *name, Error **errp)


 static bool
-opts_start_list(Visitor *v, const char *name, GenericList **list, Error **errp)
+opts_start_list(Visitor *v, const char *name, GenericList **list, size_t size,
+                Error **errp)
 {
     OptsVisitor *ov = to_ov(v);

@@ -225,7 +226,7 @@  opts_start_list(Visitor *v, const char *name, GenericList **list, Error **errp)
     ov->repeated_opts = lookup_distinct(ov, name, errp);
     if (ov->repeated_opts) {
         ov->list_mode = LM_IN_PROGRESS;
-        *list = g_new0(GenericList, 1);
+        *list = g_malloc0(size);
         return true;
     } else {
         *list = NULL;
@@ -235,7 +236,7 @@  opts_start_list(Visitor *v, const char *name, GenericList **list, Error **errp)


 static GenericList *
-opts_next_list(Visitor *v, GenericList *list, Error **errp)
+opts_next_list(Visitor *v, GenericList *list, size_t size, Error **errp)
 {
     OptsVisitor *ov = to_ov(v);

@@ -269,7 +270,7 @@  opts_next_list(Visitor *v, GenericList *list, Error **errp)
         abort();
     }

-    list->next = g_new0(GenericList, 1);
+    list->next = g_malloc0(size);
     return list->next;
 }

diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 0990199..d498f29 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -90,7 +90,8 @@  static void qapi_dealloc_end_implicit_struct(Visitor *v)
 }

 static bool qapi_dealloc_start_list(Visitor *v, const char *name,
-                                    GenericList **list, Error **errp)
+                                    GenericList **list, size_t size,
+                                    Error **errp)
 {
     QapiDeallocVisitor *qov = to_qov(v);
     qapi_dealloc_push(qov, NULL);
@@ -98,7 +99,7 @@  static bool qapi_dealloc_start_list(Visitor *v, const char *name,
 }

 static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList *list,
-                                           Error **errp)
+                                           size_t size, Error **errp)
 {
     GenericList *next = list->next;
     g_free(list);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 977df85..0476ec2 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -80,19 +80,23 @@  void visit_end_implicit_struct(Visitor *v)
 }

 bool visit_start_list(Visitor *v, const char *name, GenericList **list,
-                      Error **errp)
+                      size_t size, Error **errp)
 {
-    bool result = v->start_list(v, name, list, errp);
+    bool result;
+
+    assert(list ? size : !size);
+    result = v->start_list(v, name, list, size, errp);
     if (result) {
         assert(list && *list);
     }
     return result;
 }

-GenericList *visit_next_list(Visitor *v, GenericList *list, Error **errp)
+GenericList *visit_next_list(Visitor *v, GenericList *list, size_t size,
+                             Error **errp)
 {
-    assert(list);
-    return v->next_list(v, list, errp);
+    assert(list && size);
+    return v->next_list(v, list, size, errp);
 }

 void visit_end_list(Visitor *v)
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 2427a80..f5adeb2 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -168,7 +168,7 @@  static bool qmp_input_start_implicit_struct(Visitor *v, void **obj,
 }

 static bool qmp_input_start_list(Visitor *v, const char *name,
-                                 GenericList **list, Error **errp)
+                                 GenericList **list, size_t size, Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
     QObject *qobj = qmp_input_get_object(qiv, name, true);
@@ -184,7 +184,7 @@  static bool qmp_input_start_list(Visitor *v, const char *name,
     qmp_input_push(qiv, qobj, entry, errp);
     if (list) {
         if (entry) {
-            *list = g_new0(GenericList, 1);
+            *list = g_malloc0(size);
             return true;
         } else {
             *list = NULL;
@@ -194,7 +194,7 @@  static bool qmp_input_start_list(Visitor *v, const char *name,
 }

 static GenericList *qmp_input_next_list(Visitor *v, GenericList *list,
-                                        Error **errp)
+                                        size_t size, Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
     StackObject *so = &qiv->stack[qiv->nb_stack - 1];
@@ -202,7 +202,7 @@  static GenericList *qmp_input_next_list(Visitor *v, GenericList *list,
     if (!so->entry) {
         return NULL;
     }
-    list->next = g_new0(GenericList, 1);
+    list->next = g_malloc0(size);
     return list->next;
 }

diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index cd45ffb..59238e8 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -121,7 +121,8 @@  static void qmp_output_end_struct(Visitor *v)
 }

 static bool qmp_output_start_list(Visitor *v, const char *name,
-                                  GenericList **listp, Error **errp)
+                                  GenericList **listp, size_t size,
+                                  Error **errp)
 {
     QmpOutputVisitor *qov = to_qov(v);
     QList *list = qlist_new();
@@ -132,7 +133,7 @@  static bool qmp_output_start_list(Visitor *v, const char *name,
 }

 static GenericList *qmp_output_next_list(Visitor *v, GenericList *list,
-                                         Error **errp)
+                                         size_t size, Error **errp)
 {
     return list->next;
 }
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 0e4a07c..86bddd1 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -121,7 +121,8 @@  error:
 }

 static bool
-start_list(Visitor *v, const char *name, GenericList **list, Error **errp)
+start_list(Visitor *v, const char *name, GenericList **list, size_t size,
+           Error **errp)
 {
     StringInputVisitor *siv = to_siv(v);
     Error *err = NULL;
@@ -141,7 +142,7 @@  start_list(Visitor *v, const char *name, GenericList **list, Error **errp)
         if (r) {
             siv->cur = r->begin;
         }
-        *list = g_new0(GenericList, 1);
+        *list = g_malloc0(size);
         return true;
     } else {
         *list = NULL;
@@ -150,7 +151,7 @@  start_list(Visitor *v, const char *name, GenericList **list, Error **errp)
 }

 static GenericList *
-next_list(Visitor *v, GenericList *list, Error **errp)
+next_list(Visitor *v, GenericList *list, size_t size, Error **errp)
 {
     StringInputVisitor *siv = to_siv(v);
     Range *r;
@@ -176,7 +177,7 @@  next_list(Visitor *v, GenericList *list, Error **errp)
         siv->cur = r->begin;
     }

-    list->next = g_new0(GenericList, 1);
+    list->next = g_malloc0(size);
     return list->next;
 }

diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
index 2666802..104f8a4 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -264,7 +264,8 @@  static void print_type_number(Visitor *v, const char *name, double *obj,
 }

 static bool
-start_list(Visitor *v, const char *name, GenericList **list, Error **errp)
+start_list(Visitor *v, const char *name, GenericList **list, size_t size,
+           Error **errp)
 {
     StringOutputVisitor *sov = to_sov(v);

@@ -280,7 +281,7 @@  start_list(Visitor *v, const char *name, GenericList **list, Error **errp)
 }

 static GenericList *
-next_list(Visitor *v, GenericList *list, Error **errp)
+next_list(Visitor *v, GenericList *list, size_t size, Error **errp)
 {
     StringOutputVisitor *sov = to_sov(v);
     GenericList *ret = list->next;
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 5cf20c2..47a6523 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -26,11 +26,8 @@  def gen_array(name, element_type):
     return mcgen('''

 struct %(c_name)s {
-    union {
-        %(c_type)s value;
-        uint64_t padding;
-    };
     %(c_name)s *next;
+    %(c_type)s value;
 };
 ''',
                  c_name=c_name(name), c_type=element_type.c_type())
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index eebb5f7..193a1b6 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -130,7 +130,7 @@  void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
     %(c_name)s *elt;
     bool allocated;

-    allocated = visit_start_list(v, name, (GenericList **)obj, &err);
+    allocated = visit_start_list(v, name, (GenericList **)obj, sizeof(%(c_name)s), &err);
     if (err) {
         goto out;
     }
@@ -140,7 +140,7 @@  void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
         if (err) {
             break;
         }
-        elt = (%(c_name)s *)visit_next_list(v, (GenericList *)elt, &err);
+        elt = (%(c_name)s *)visit_next_list(v, (GenericList *)elt, sizeof(%(c_name)s), &err);
         if (err) {
             break;
         }