Re: [Qemu-devel] [PATCH v7 05/14] qapi: Lazy creation of array types

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
>
> ---
> 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