[v7,05/14] qapi: Lazy creation of array types

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
for our new Dummy structs, and the new struct itself, don't
affect QMP).

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

---
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(-)

Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com>
>
> ---
> 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

On 10/07/2015 10:38 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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!
>>

>> +    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.

pep8 doesn't like it:

scripts/qapi.py:1299:17: E126 continuation line over-indented for
hanging indent

but it was okay with:

    type = (self._make_implicit_object_type(
            type, ...))

and that does look a little better.

Eric Blake <eblake@redhat.com> writes:

> On 10/07/2015 10:38 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> 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!
>>>
>
>>> +    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.
>
> pep8 doesn't like it:
>
> scripts/qapi.py:1299:17: E126 continuation line over-indented for
> hanging indent

Meh.

See also https://github.com/PyCQA/pep8/issues/167

> but it was okay with:
>
>     type = (self._make_implicit_object_type(
>             type, ...))
>
> and that does look a little better.

I find it a bit stilted.  Let's stick with your first version.