| Message ID | 1455778109-6278-13-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > There's no reason to do two malloc's for an alternate type visiting > a QAPI struct; let's just inline the struct directly as the C union > branch of the struct. > > Surprisingly, no clients were actually using the struct member prior > to this patch outside of the testsuite; an earlier patch in the series > added some testsuite coverage to make the effect of this patch more > obvious. > > In qapi.py, c_type() gains a new is_unboxed flag to control when we > are emitting a C struct unboxed within the context of an outer > struct (different from our other two modes of usage with no flags > for normal local variable declarations, and with is_param for adding > 'const' in a parameter list). I don't know if there is any more > pythonic way of collapsing the two flags into a single parameter, > as we never have a caller setting both flags at once. > > Ultimately, we want to also unbox branches for QAPI unions, but as > that touches a lot more client code, it is better as separate > patches. But since unions and alternates share gen_variants(), I > had to hack in a way to test if we are visiting an alternate type > for setting the is_unboxed flag: look for a non-object branch. > This works because alternates have at least two branches, with at > most one object branch, while unions have only object branches. > The hack will go away in a later patch. > > The generated code difference to qapi-types.h is relatively small: > > | struct BlockdevRef { > | QType type; > | union { /* union tag is @type */ > | void *data; > |- BlockdevOptions *definition; > |+ BlockdevOptions definition; > | char *reference; > | } u; > | }; > > meanwhile, in qapi-visit.h, we trade the previous visit_type_FOO(obj) Meanwhile > (which allocates a pointer, handles a layer of {} from the JSON stream, > and visits the fields) with an inline call to visit_type_FOO(NULL) (to > consume the {} without allocation) and a direct visit of the fields: I don't see a call to visit_type_FOO(NULL). Suggest not to abbreviate argument lists, it's mildly confusing. > > | switch ((*obj)->type) { > | case QTYPE_QDICT: > |- visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err); > |+ visit_start_struct(v, name, NULL, 0, &err); > |+ if (err) { > |+ break; > |+ } > |+ visit_type_BlockdevOptions_fields(v, &(*obj)->u.definition, &err); > |+ error_propagate(errp, err); > |+ err = NULL; > |+ visit_end_struct(v, &err); > | break; > | case QTYPE_QSTRING: > | visit_type_str(v, name, &(*obj)->u.reference, &err); > > The visit of non-object fields is unchanged. > > Signed-off-by: Eric Blake <eblake@redhat.com> Patch looks good.
On 02/18/2016 09:43 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> There's no reason to do two malloc's for an alternate type visiting >> a QAPI struct; let's just inline the struct directly as the C union >> branch of the struct. >> >> >> meanwhile, in qapi-visit.h, we trade the previous visit_type_FOO(obj) > > Meanwhile > >> (which allocates a pointer, handles a layer of {} from the JSON stream, >> and visits the fields) Maybe: (which first calls visit_start_struct(v, name, obj, size, &err) to allocate a pointer and handle a layer of {} from the JSON stream, then visits the fields) >> with an inline call to visit_type_FOO(NULL) (to >> consume the {} without allocation) and a direct visit of the fields: > > I don't see a call to visit_type_FOO(NULL). Whoops, wrong function name. I meant: visit_start_struct(v, name, NULL, 0, &err). ^^^^ Especially useful if you take my earlier text suggestion to point out the difference in the visit_start_struct() calls as being the intentional so that we continue to consume {} but now opt out of a second allocation. > > Suggest not to abbreviate argument lists, it's mildly confusing. Sure. Are we still at a point that you could touch up the commit message without a patch respin? > > Patch looks good. >
Eric Blake <eblake@redhat.com> writes: > On 02/18/2016 09:43 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> There's no reason to do two malloc's for an alternate type visiting >>> a QAPI struct; let's just inline the struct directly as the C union >>> branch of the struct. >>> >>> >>> meanwhile, in qapi-visit.h, we trade the previous visit_type_FOO(obj) >> >> Meanwhile >> >>> (which allocates a pointer, handles a layer of {} from the JSON stream, >>> and visits the fields) > > Maybe: > > (which first calls visit_start_struct(v, name, obj, size, &err) to > allocate a pointer and handle a layer of {} from the JSON stream, then > visits the fields) > >>> with an inline call to visit_type_FOO(NULL) (to >>> consume the {} without allocation) and a direct visit of the fields: >> >> I don't see a call to visit_type_FOO(NULL). > > Whoops, wrong function name. I meant: > > visit_start_struct(v, name, NULL, 0, &err). > ^^^^ > Especially useful if you take my earlier text suggestion to point out > the difference in the visit_start_struct() calls as being the > intentional so that we continue to consume {} but now opt out of a > second allocation. Better, but the sentence is long enough to confuse even a German. What about: The corresponding spot in qapi-visit.c calls visit_type_FOO(), which first calls visit_start_struct() to allocate or deallocate the member and handle a layer of {} from the JSON stream, then visits the members. To peel off the indirection and the memory management that comes with it, we inline this call, then suppress allocation / deallocation by passing NULL to visit_start_struct(), and adjust the member visit: >> >> Suggest not to abbreviate argument lists, it's mildly confusing. > > Sure. Are we still at a point that you could touch up the commit message > without a patch respin? Looks like it so far. >> >> Patch looks good. >>
On 02/18/2016 11:56 AM, Markus Armbruster wrote: > Better, but the sentence is long enough to confuse even a German. What > about: lol > > The corresponding spot in qapi-visit.c calls visit_type_FOO(), which > first calls visit_start_struct() to allocate or deallocate the member > and handle a layer of {} from the JSON stream, then visits the > members. To peel off the indirection and the memory management that > comes with it, we inline this call, then suppress allocation / > deallocation by passing NULL to visit_start_struct(), and adjust the > member visit: Works for me. [Technically, visit_start_struct() never deallocates, it merely records a pointer for later deallocation during visit_end_struct(); but I don't think we need to worry about the imprecision here]
diff --git a/scripts/qapi.py b/scripts/qapi.py index 17bf633..8497777 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -824,7 +824,7 @@ class QAPISchemaVisitor(object): class QAPISchemaType(QAPISchemaEntity): - def c_type(self, is_param=False): + def c_type(self, is_param=False, is_unboxed=False): return c_name(self.name) + pointer_suffix def c_null(self): @@ -857,7 +857,7 @@ class QAPISchemaBuiltinType(QAPISchemaType): def c_name(self): return self.name - def c_type(self, is_param=False): + def c_type(self, is_param=False, is_unboxed=False): if is_param and self.name == 'str': return 'const ' + self._c_type_name return self._c_type_name @@ -891,7 +891,7 @@ class QAPISchemaEnumType(QAPISchemaType): # See QAPISchema._make_implicit_enum_type() return self.name.endswith('Kind') - def c_type(self, is_param=False): + def c_type(self, is_param=False, is_unboxed=False): return c_name(self.name) def member_names(self): @@ -987,9 +987,11 @@ class QAPISchemaObjectType(QAPISchemaType): assert not self.is_implicit() return QAPISchemaType.c_name(self) - def c_type(self, is_param=False): + def c_type(self, is_param=False, is_unboxed=False): assert not self.is_implicit() - return QAPISchemaType.c_type(self) + if is_unboxed: + return c_name(self.name) + return c_name(self.name) + pointer_suffix def json_type(self): return 'object' diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 6ea0ae6..4dabe91 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -116,6 +116,14 @@ static inline %(base)s *qapi_%(c_name)s_base(const %(c_name)s *obj) def gen_variants(variants): + # HACK: Determine if this is an alternate (at least one variant + # is not an object); unions have all branches as objects. + unboxed = False + for v in variants.variants: + if not isinstance(v.type, QAPISchemaObjectType): + unboxed = True + break + # FIXME: What purpose does data serve, besides preventing a union that # has a branch named 'data'? We use it in qapi-visit.py to decide # whether to bypass the switch statement if visiting the discriminator @@ -136,7 +144,7 @@ def gen_variants(variants): ret += mcgen(''' %(c_type)s %(c_name)s; ''', - c_type=typ.c_type(), + c_type=typ.c_type(is_unboxed=unboxed), c_name=c_name(var.name)) ret += mcgen(''' diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 089f4c4..61b7e72 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -201,11 +201,14 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s *obj, Error def gen_visit_alternate(name, variants): promote_int = 'true' + ret = '' for var in variants.variants: if var.type.alternate_qtype() == 'QTYPE_QINT': promote_int = 'false' + if isinstance(var.type, QAPISchemaObjectType): + ret += gen_visit_fields_decl(var.type) - ret = mcgen(''' + ret += mcgen(''' void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp) { @@ -221,17 +224,35 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error } switch ((*obj)->type) { ''', - c_name=c_name(name), promote_int=promote_int) + c_name=c_name(name), promote_int=promote_int) for var in variants.variants: ret += mcgen(''' case %(case)s: +''', + case=var.type.alternate_qtype()) + if isinstance(var.type, QAPISchemaObjectType): + ret += mcgen(''' + visit_start_struct(v, name, NULL, 0, &err); + if (err) { + break; + } + visit_type_%(c_type)s_fields(v, &(*obj)->u.%(c_name)s, &err); + error_propagate(errp, err); + err = NULL; + visit_end_struct(v, &err); +''', + c_type=var.type.c_name(), + c_name=c_name(var.name)) + else: + ret += mcgen(''' visit_type_%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err); - break; ''', - case=var.type.alternate_qtype(), - c_type=var.type.c_name(), - c_name=c_name(var.name)) + c_type=var.type.c_name(), + c_name=c_name(var.name)) + ret += mcgen(''' + break; +''') ret += mcgen(''' default: diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index ef836d5..47cf6aa 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -327,11 +327,11 @@ static void test_visitor_in_alternate(TestInputVisitorData *data, "'enum1':'value1', 'boolean':true}"); visit_type_UserDefAlternate(v, NULL, &tmp, &error_abort); g_assert_cmpint(tmp->type, ==, QTYPE_QDICT); - g_assert_cmpint(tmp->u.udfu->integer, ==, 1); - g_assert_cmpstr(tmp->u.udfu->string, ==, "str"); - g_assert_cmpint(tmp->u.udfu->enum1, ==, ENUM_ONE_VALUE1); - g_assert_cmpint(tmp->u.udfu->u.value1->boolean, ==, true); - g_assert_cmpint(tmp->u.udfu->u.value1->has_a_b, ==, false); + g_assert_cmpint(tmp->u.udfu.integer, ==, 1); + g_assert_cmpstr(tmp->u.udfu.string, ==, "str"); + g_assert_cmpint(tmp->u.udfu.enum1, ==, ENUM_ONE_VALUE1); + g_assert_cmpint(tmp->u.udfu.u.value1->boolean, ==, true); + g_assert_cmpint(tmp->u.udfu.u.value1->has_a_b, ==, false); qapi_free_UserDefAlternate(tmp); v = visitor_input_test_init(data, "false"); @@ -355,11 +355,11 @@ static void test_visitor_in_alternate(TestInputVisitorData *data, "'enum1':'value1', 'boolean':true} }"); visit_type_WrapAlternate(v, NULL, &wrap, &error_abort); g_assert_cmpint(wrap->alt->type, ==, QTYPE_QDICT); - g_assert_cmpint(wrap->alt->u.udfu->integer, ==, 1); - g_assert_cmpstr(wrap->alt->u.udfu->string, ==, "str"); - g_assert_cmpint(wrap->alt->u.udfu->enum1, ==, ENUM_ONE_VALUE1); - g_assert_cmpint(wrap->alt->u.udfu->u.value1->boolean, ==, true); - g_assert_cmpint(wrap->alt->u.udfu->u.value1->has_a_b, ==, false); + g_assert_cmpint(wrap->alt->u.udfu.integer, ==, 1); + g_assert_cmpstr(wrap->alt->u.udfu.string, ==, "str"); + g_assert_cmpint(wrap->alt->u.udfu.enum1, ==, ENUM_ONE_VALUE1); + g_assert_cmpint(wrap->alt->u.udfu.u.value1->boolean, ==, true); + g_assert_cmpint(wrap->alt->u.udfu.u.value1->has_a_b, ==, false); qapi_free_WrapAlternate(wrap); } diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c index 2b0f7e9..fe2f1a1 100644 --- a/tests/test-qmp-output-visitor.c +++ b/tests/test-qmp-output-visitor.c @@ -457,12 +457,11 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data, tmp = g_new0(UserDefAlternate, 1); tmp->type = QTYPE_QDICT; - tmp->u.udfu = g_new0(UserDefFlatUnion, 1); - tmp->u.udfu->integer = 1; - tmp->u.udfu->string = g_strdup("str"); - tmp->u.udfu->enum1 = ENUM_ONE_VALUE1; - tmp->u.udfu->u.value1 = g_new0(UserDefA, 1); - tmp->u.udfu->u.value1->boolean = true; + tmp->u.udfu.integer = 1; + tmp->u.udfu.string = g_strdup("str"); + tmp->u.udfu.enum1 = ENUM_ONE_VALUE1; + tmp->u.udfu.u.value1 = g_new0(UserDefA, 1); + tmp->u.udfu.u.value1->boolean = true; visit_type_UserDefAlternate(data->ov, NULL, &tmp, &error_abort); arg = qmp_output_get_qobject(data->qov);
There's no reason to do two malloc's for an alternate type visiting a QAPI struct; let's just inline the struct directly as the C union branch of the struct. Surprisingly, no clients were actually using the struct member prior to this patch outside of the testsuite; an earlier patch in the series added some testsuite coverage to make the effect of this patch more obvious. In qapi.py, c_type() gains a new is_unboxed flag to control when we are emitting a C struct unboxed within the context of an outer struct (different from our other two modes of usage with no flags for normal local variable declarations, and with is_param for adding 'const' in a parameter list). I don't know if there is any more pythonic way of collapsing the two flags into a single parameter, as we never have a caller setting both flags at once. Ultimately, we want to also unbox branches for QAPI unions, but as that touches a lot more client code, it is better as separate patches. But since unions and alternates share gen_variants(), I had to hack in a way to test if we are visiting an alternate type for setting the is_unboxed flag: look for a non-object branch. This works because alternates have at least two branches, with at most one object branch, while unions have only object branches. The hack will go away in a later patch. The generated code difference to qapi-types.h is relatively small: | struct BlockdevRef { | QType type; | union { /* union tag is @type */ | void *data; |- BlockdevOptions *definition; |+ BlockdevOptions definition; | char *reference; | } u; | }; meanwhile, in qapi-visit.h, we trade the previous visit_type_FOO(obj) (which allocates a pointer, handles a layer of {} from the JSON stream, and visits the fields) with an inline call to visit_type_FOO(NULL) (to consume the {} without allocation) and a direct visit of the fields: | switch ((*obj)->type) { | case QTYPE_QDICT: |- visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err); |+ visit_start_struct(v, name, NULL, 0, &err); |+ if (err) { |+ break; |+ } |+ visit_type_BlockdevOptions_fields(v, &(*obj)->u.definition, &err); |+ error_propagate(errp, err); |+ err = NULL; |+ visit_end_struct(v, &err); | break; | case QTYPE_QSTRING: | visit_type_str(v, name, &(*obj)->u.reference, &err); The visit of non-object fields is unchanged. Signed-off-by: Eric Blake <eblake@redhat.com> --- v11: drop visit_type_alternate_FOO(), improve commit message, fix and test bug regarding visits to a union, rework some of the hacks to make them more obvious v10: new patch --- scripts/qapi.py | 12 +++++++----- scripts/qapi-types.py | 10 +++++++++- scripts/qapi-visit.py | 33 +++++++++++++++++++++++++++------ tests/test-qmp-input-visitor.c | 20 ++++++++++---------- tests/test-qmp-output-visitor.c | 11 +++++------ 5 files changed, 58 insertions(+), 28 deletions(-)