Re: [Qemu-devel] [PATCH v9 29/37] qapi: Eliminate empty visit_type_FOO_fields

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> For empty structs, such as the 'Abort' helper type used as part
> of the 'transaction' command, we were emitting a no-op
> visit_type_FOO_fields().  Optimize things to instead omit calls
> for empty structs.  Generated code changes resemble:
>
> |-static void visit_type_Abort_fields(Visitor *v, Abort **obj, Error **errp)
> |-{
> |-    Error *err = NULL;
> |-
> |-    error_propagate(errp, err);
> |-}
> |-
> | void visit_type_Abort(Visitor *v, const char *name, Abort **obj, Error **errp)
> | {
> |     Error *err = NULL;
> |@@ -112,7 +105,6 @@ void visit_type_Abort(Visitor *v, Abort
> |     if (!*obj) {
> |         goto out_obj;
> |     }
> |-    visit_type_Abort_fields(v, obj, &err);
> | out_obj:
> |     error_propagate(errp, err);
>
> Another reason for doing this optimization is that it gets us
> closer to merging the code for visiting structs and unions:
> since flat unions have no local members, they do not need to
> have a visit_type_UNION_fields() emitted, even when they start
> sharing the code used to visit structs.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> ---
> v9: no change
> v8: rebase to earlier changes
> v7: rebase to earlier changes
> v6: new patch
> ---
>  scripts/qapi-visit.py | 41 +++++++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 18 deletions(-)
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 573bb81..6537a20 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -35,22 +35,22 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_type)sobj, Error **

Two clean ways to skin this cat (at least).  One emphasizes that
gen_visit_fields_decl() & friends do nothing when the type is empty.
The other emphasizes that they generate at most once.  Matter of taste.

The two ways differ in how they treat the seen sets:

   # visit_type_FOO_implicit() is emitted as needed; track if it has already
   # been output.
   implicit_structs_seen = set()

   # visit_type_FOO_fields() is always emitted; track if a forward declaration
   # or implementation has already been output.
   struct_fields_seen = set()

To emphasize do nothing when empty, test empty first.  iEmpty types are
never added to these seen sets.

To emphasize generate at most once, test the seen set first.  Empty
types are added normally, even though nothing is output for them.

>
>
>  def gen_visit_fields_decl(typ):
> -    ret = ''
> -    if typ.name not in struct_fields_seen:
> -        ret += mcgen('''
> +    if typ.is_empty() or typ.name in struct_fields_seen:
> +        return ''
> +
> +    struct_fields_seen.add(typ.name)
> +    return mcgen('''
>
>  static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error **errp);
>  ''',
> -                     c_type=typ.c_name())
> -        struct_fields_seen.add(typ.name)
> -    return ret
> +                 c_type=typ.c_name())

Emphasize do nothing when empty:

   def gen_visit_fields_decl(typ):
       if typ.is_empty():
           return ''
       if typ.name in struct_fields_seen:
           return ''
       struct_fields_seen.add(typ.name)
       return mcgen('''

   static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error **errp);
   ''',
                        c_type=typ.c_name())

Same as yours, except yours merges the conditionals.  I left them
unmerged to make the

    if X not in S:
        return ''
    S.add(X)

pattern stand out.  Matter of taste.

Emphasize generate at most once:

   def gen_visit_fields_decl(typ):
       if typ.name in struct_fields_seen:
           return ''
       struct_fields_seen.add(typ.name)
       if typ.is_empty():
           return ''
       return mcgen('''

   static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error **errp);
   ''',
                        c_type=typ.c_name())

>
>
>  def gen_visit_implicit_struct(typ):
> -    if typ in implicit_structs_seen:
> +    if typ.is_empty() or typ in implicit_structs_seen:
>          return ''
> +
>      implicit_structs_seen.add(typ)
> -
>      ret = gen_visit_fields_decl(typ)
>

Likewise.

>      ret += mcgen('''
> @@ -74,7 +74,10 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error *
>  def gen_visit_struct_fields(name, base, members):
>      ret = ''
>
> -    if base:
> +    if (not base or base.is_empty()) and not members:
> +        return ret

The condition predicts whether the rest of the function won't generate
anything useful.  Such predictions can be brittle, but this one seems
still simple enough.

> +
> +    if base and not base.is_empty():
>          ret += gen_visit_fields_decl(base)

"if base" suffices, because gen_visit_fields_decl(base) returns '' when
base.is_empty().  I guess you test it anyway, for symmetry with the next
hunk.  Okay.

>
>      struct_fields_seen.add(name)
> @@ -87,7 +90,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
>  ''',
>                   c_name=c_name(name))
>
> -    if base:
> +    if base and not base.is_empty():
>          ret += mcgen('''
>      visit_type_%(c_type)s_fields(v, (%(c_type)s **)obj, &err);
>  ''',
> @@ -96,13 +99,9 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
>
>      ret += gen_visit_fields(members, prefix='(*obj)->')
>
> -    # 'goto out' produced for base, and by gen_visit_fields() for each member
> -    if base or members:
> -        ret += mcgen('''
> +    ret += mcgen('''
>
>  out:
> -''')
> -    ret += mcgen('''
>      error_propagate(errp, err);
>  }
>  ''')

The optimization earns a small part of its keep here: we don't need a
conditional for the out label anymore.

> @@ -129,7 +128,14 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
>      if (!*obj) {
>          goto out_obj;
>      }
> +''',
> +                 name=name, c_name=c_name(name))
> +    if (base and not base.is_empty()) or members:

Duplicates gen_visit_struct_fields()' guard.  Could be avoided by
testing whether gen_visit_struct_fields() returned anything.  Either way
is kind of ugly, though.  Pick the poison you hate less.

> +        ret += mcgen('''
>      visit_type_%(c_name)s_fields(v, obj, &err);
> +''',
> +                     c_name=c_name(name))
> +    ret += mcgen('''
>  out_obj:
>      error_propagate(errp, err);
>      err = NULL;
> @@ -137,8 +143,7 @@ out_obj:
>  out:
>      error_propagate(errp, err);
>  }
> -''',
> -                 c_name=c_name(name))
> +''')
>
>      return ret
>
> @@ -300,7 +305,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
>  ''',
>                           c_type=simple_union_type.c_name(),
>                           c_name=c_name(var.name))
> -        else:
> +        elif not var.type.is_empty():
>              ret += mcgen('''
>          visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err);
>  ''',

Similar, for gen_visit_implicit_struct().  The condition is more obvious
here.

I'm not sure the optimization is worthwhile by itself.  Empty structs
are rare.  I'm reserving judgement until I see the struct/union
unification.