| Message ID | 1453219845-30939-37-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > 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> > > --- > v9: no change > 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(-) > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > index 41f2da0..0eba901 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 8df4ba1..dbbbcdb 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; Less trickery, I like it. Member padding appears to be unused. > > /** > @@ -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 259e0cb..ed4de71 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); Tighter than size != 0 would be size >= GenericList. Same elsewhere. > + 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) [...] Rest looks good. Didn't look as closely as for the previous patches (getting tired), but so far I like the idea.
On 01/28/2016 08:34 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> 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'. Should I attempt this? >> typedef struct GenericList >> { >> - union { >> - void *value; >> - uint64_t padding; >> - }; >> struct GenericList *next; >> + char padding[]; >> } GenericList; > > Less trickery, I like it. > > Member padding appears to be unused. or just leave it at this? >> 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); > > Tighter than size != 0 would be size >= GenericList. Same elsewhere. Makes sense. > > Rest looks good. Didn't look as closely as for the previous patches > (getting tired), but so far I like the idea. Okay, I'll keep it and drop the RFC.
Eric Blake <eblake@redhat.com> writes: > On 01/28/2016 08:34 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> 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'. > > Should I attempt this? A quick grep for '(GenericList' finds two in qapi-code-gen.txt, and two in qapi-visit-py. I doubt avoiding them is worth much of your time or mine :) >>> typedef struct GenericList >>> { >>> - union { >>> - void *value; >>> - uint64_t padding; >>> - }; >>> struct GenericList *next; >>> + char padding[]; >>> } GenericList; >> >> Less trickery, I like it. >> >> Member padding appears to be unused. > > or just leave it at this? I'd say good enough. >>> 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); >> >> Tighter than size != 0 would be size >= GenericList. Same elsewhere. > > Makes sense. > >> >> Rest looks good. Didn't look as closely as for the previous patches >> (getting tired), but so far I like the idea. > > Okay, I'll keep it and drop the RFC. Thanks!
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index 41f2da0..0eba901 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 8df4ba1..dbbbcdb 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 259e0cb..ed4de71 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 6b4ad68..16c18b3 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 ce592d2..78567d0 100644 --- a/qapi/qmp-output-visitor.c +++ b/qapi/qmp-output-visitor.c @@ -120,7 +120,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(); @@ -131,7 +132,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; }
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> --- v9: no change 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(-)