From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56365) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zjrjy-0004sA-IV for qemu-devel@nongnu.org; Wed, 07 Oct 2015 12:38:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zjrjt-00056V-TB for qemu-devel@nongnu.org; Wed, 07 Oct 2015 12:38:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50448) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zjrjt-000563-JQ for qemu-devel@nongnu.org; Wed, 07 Oct 2015 12:38:49 -0400 From: Markus Armbruster References: <1443930073-19359-1-git-send-email-eblake@redhat.com> <1443930073-19359-6-git-send-email-eblake@redhat.com> Date: Wed, 07 Oct 2015 18:38:45 +0200 In-Reply-To: <1443930073-19359-6-git-send-email-eblake@redhat.com> (Eric Blake's message of "Sat, 3 Oct 2015 21:41:04 -0600") Message-ID: <874mi2ws4q.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v7 05/14] qapi: Lazy creation of array types List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: marcandre.lureau@redhat.com, qemu-devel@nongnu.org, ehabkost@redhat.com, Michael Roth Eric Blake writes: > Commit ac88219a had several TODO markers about whether we needed > to automatically create the corresponding array type alongside > any other type. It turns out that most of the time, we don't! > > As part of lazy creation of array types, this patch now assigns > an 'info' to array types at their point of first instantiation, > rather than leaving it None. > > There are a few exceptions: 1) We have a few situations where we > use an array type in internal code but do not expose that type > through QMP; fix it by declaring a dummy type that forces the > generator to see that we want to use the array type. > > 2) The builtin arrays (such as intList for QAPI ['int']) must > always be generated, because of the way our QAPI_TYPES_BUILTIN > compile guard works: we have situations (at the very least > tests/test-qmp-output-visitor.c) that include both top-level > "qapi-types.h" (via "error.h") and a secondary > "test-qapi-types.h". If we were to only emit the builtin types > when used locally, then the first .h file would not include all > types, but the second .h does not declare anything at all because > the first .h set QAPI_TYPES_BUILTIN, and we would end up with > compilation error due to things like unknown type 'int8List'. > > Actually, we may need to revisit how we do type guards, and > change from a single QAPI_TYPES_BUILTIN over to a different > usage pattern that does one #ifdef per qapi type - right now, > the only types that are declared multiple times between two qapi > .json files for inclusion by a single .c file happen to be the > builtin arrays. But now that we have QAPI 'include' statements, > it is logical to assume that we will soon reach a point where > we want to reuse non-builtin types (yes, I'm thinking about what > it will take to add introspection to QGA, where we will want to > reuse the SchemaInfo type and friends). One #ifdef per type > will help ensure that generating the same qapi type into more > than one qapi-types.h won't cause collisions when both are > included in the same .c file; but we also have to solve how to > avoid creating duplicate qapi-types.c entry points. So that > is a problem left for another day. > > Generated code for qapi-types and qapi-visit is drastically > reduced; less than a third of the arrays that were blindly > created were actually needed (a quick grep shows we dropped > from 219 to 69 *List types), and the .o files lost more than > 30% of their bulk. [For best results, diff the generated > files with 'git diff --patience --no-index pre post'.] > > Interestingly, the introspection output is unchanged - this is > because we already cull all types that are not indirectly > reachable from a command or event, so introspection was already > using only a subset of array types. The subset of types > introspected is now a much larger percentage of the overall set > of array types emitted in qapi-types.h (since the larger set > shrunk), but still not 100% (proof that the array types emitted Evidence, not proof :) > for our new Dummy structs, and the new struct itself, don't > affect QMP). > > Signed-off-by: Eric Blake > > --- > v7: improve commit message, add comments, rename dummy type, > better line wrapping > v6: new patch > --- > qapi-schema.json | 11 +++++++ > scripts/qapi.py | 52 ++++++++++++++++++--------------- > tests/qapi-schema/qapi-schema-test.json | 4 +++ > tests/qapi-schema/qapi-schema-test.out | 3 ++ > 4 files changed, 47 insertions(+), 23 deletions(-) > > diff --git a/qapi-schema.json b/qapi-schema.json > index 8b0520c..81d2e18 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -3396,6 +3396,17 @@ > 'features': 'int' } } > > ## > +# @DummyForceArrays > +# > +# Not used by QMP; hack to let us use X86CPUFeatureWordInfoList internally > +# > +# Since 2.5 > +## > +{ 'struct': 'DummyForceArrays', > + 'data': { 'unused': ['X86CPUFeatureWordInfo'] } } > + > + > +## > # @RxState: > # > # Packets receiving state > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 19cc78e..b1134b8 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -793,6 +793,11 @@ class QAPISchemaEntity(object): > def __init__(self, name, info): > assert isinstance(name, str) > self.name = name > + # For explicitly defined entities, info points to the (explicit) > + # definition. For builtins (and their arrays), info is None. > + # TODO For other implicitly defined entities, it should point to > + # a place that triggers implicit definition; there may be more > + # than one such place. The TODO suggests that info doesn't yet point to a place that triggers implicit definition. Yet your patch makes it point at least for arrays. To fix, you could insert "For other arrays, info points to a place that implicitly defines the array." > self.info = info > > def c_name(self): > @@ -1160,7 +1165,12 @@ class QAPISchema(object): > def _def_builtin_type(self, name, json_type, c_type, c_null): > self._def_entity(QAPISchemaBuiltinType(name, json_type, > c_type, c_null)) > - self._make_array_type(name) # TODO really needed? > + # TODO As long as we have QAPI_TYPES_BUILTIN to share multiple > + # qapi-types.h from a single .c, all arrays of builtins must be > + # declared in the first file whether or not they are used. Nicer > + # would be to use lazy instantiation, while figuring out how to > + # avoid compilation issues with multiple qapi-types.h. > + self._make_array_type(name, None) > > def _def_predefineds(self): > for t in [('str', 'string', 'char' + pointer_suffix, 'NULL'), > @@ -1187,10 +1197,10 @@ class QAPISchema(object): > self._def_entity(QAPISchemaEnumType(name, None, values, None)) > return name > > - def _make_array_type(self, element_type): > + def _make_array_type(self, element_type, info): > name = element_type + 'List' > if not self.lookup_type(name): > - self._def_entity(QAPISchemaArrayType(name, None, element_type)) > + self._def_entity(QAPISchemaArrayType(name, info, element_type)) > return name > > def _make_implicit_object_type(self, name, role, members): > @@ -1207,20 +1217,19 @@ class QAPISchema(object): > data = expr['data'] > prefix = expr.get('prefix') > self._def_entity(QAPISchemaEnumType(name, info, data, prefix)) > - self._make_array_type(name) # TODO really needed? > > - def _make_member(self, name, typ): > + def _make_member(self, name, typ, info): > optional = False > if name.startswith('*'): > name = name[1:] > optional = True > if isinstance(typ, list): > assert len(typ) == 1 > - typ = self._make_array_type(typ[0]) > + typ = self._make_array_type(typ[0], info) > return QAPISchemaObjectTypeMember(name, typ, optional) > > - def _make_members(self, data): > - return [self._make_member(key, value) > + def _make_members(self, data, info): > + return [self._make_member(key, value, info) > for (key, value) in data.iteritems()] > > def _def_struct_type(self, expr, info): > @@ -1228,19 +1237,18 @@ class QAPISchema(object): > base = expr.get('base') > data = expr['data'] > self._def_entity(QAPISchemaObjectType(name, info, base, > - self._make_members(data), > + self._make_members(data, info), > None)) > - self._make_array_type(name) # TODO really needed? > > def _make_variant(self, case, typ): > return QAPISchemaObjectTypeVariant(case, typ) > > - def _make_simple_variant(self, case, typ): > + def _make_simple_variant(self, case, typ, info): > if isinstance(typ, list): > assert len(typ) == 1 > - typ = self._make_array_type(typ[0]) > - typ = self._make_implicit_object_type(typ, 'wrapper', > - [self._make_member('data', typ)]) > + typ = self._make_array_type(typ[0], info) > + typ = self._make_implicit_object_type( > + typ, 'wrapper', [self._make_member('data', typ, info)]) I'd indent the hanging intent a bit more, to make the = stand out. > return QAPISchemaObjectTypeVariant(case, typ) > > def _make_tag_enum(self, type_name, variants): > @@ -1257,16 +1265,15 @@ class QAPISchema(object): > variants = [self._make_variant(key, value) > for (key, value) in data.iteritems()] > else: > - variants = [self._make_simple_variant(key, value) > + variants = [self._make_simple_variant(key, value, info) > for (key, value) in data.iteritems()] > tag_enum = self._make_tag_enum(name, variants) > self._def_entity( > QAPISchemaObjectType(name, info, base, > - self._make_members(OrderedDict()), > + self._make_members(OrderedDict(), info), > QAPISchemaObjectTypeVariants(tag_name, > tag_enum, > variants))) > - self._make_array_type(name) # TODO really needed? > > def _def_alternate_type(self, expr, info): > name = expr['alternate'] > @@ -1279,7 +1286,6 @@ class QAPISchema(object): > QAPISchemaObjectTypeVariants(None, > tag_enum, > variants))) > - self._make_array_type(name) # TODO really needed? > > def _def_command(self, expr, info): > name = expr['command'] > @@ -1288,11 +1294,11 @@ class QAPISchema(object): > gen = expr.get('gen', True) > success_response = expr.get('success-response', True) > if isinstance(data, OrderedDict): > - data = self._make_implicit_object_type(name, 'arg', > - self._make_members(data)) > + data = self._make_implicit_object_type( > + name, 'arg', self._make_members(data, info)) Likewise. > if isinstance(rets, list): > assert len(rets) == 1 > - rets = self._make_array_type(rets[0]) > + rets = self._make_array_type(rets[0], info) > self._def_entity(QAPISchemaCommand(name, info, data, rets, gen, > success_response)) > > @@ -1300,8 +1306,8 @@ class QAPISchema(object): > name = expr['event'] > data = expr.get('data') > if isinstance(data, OrderedDict): > - data = self._make_implicit_object_type(name, 'arg', > - self._make_members(data)) > + data = self._make_implicit_object_type( > + name, 'arg', self._make_members(data, info)) Likewise. > self._def_entity(QAPISchemaEvent(name, info, data)) > > def _def_exprs(self): > diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json > index abe59fd..020ff2e 100644 > --- a/tests/qapi-schema/qapi-schema-test.json > +++ b/tests/qapi-schema/qapi-schema-test.json > @@ -31,6 +31,10 @@ > 'data': { 'string0': 'str', > 'dict1': 'UserDefTwoDict' } } > > +# dummy struct to force generation of array types not otherwise mentioned > +{ 'struct': 'ForceArrays', > + 'data': { 'unused1':['UserDefOne'], 'unused2':['UserDefTwo'] } } > + > # for testing unions > # Among other things, test that a name collision between branches does > # not cause any problems (since only one branch can be in use at a time), > diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out > index 8f81784..2a8c82e 100644 > --- a/tests/qapi-schema/qapi-schema-test.out > +++ b/tests/qapi-schema/qapi-schema-test.out > @@ -86,6 +86,9 @@ object EventStructOne > member struct1: UserDefOne optional=False > member string: str optional=False > member enum2: EnumOne optional=True > +object ForceArrays > + member unused1: UserDefOneList optional=False > + member unused2: UserDefTwoList optional=False > object NestedEnumsOne > member enum1: EnumOne optional=False > member enum2: EnumOne optional=True