[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 29/37] qapi: Eliminate empty visit_type_FOO_f
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v9 29/37] qapi: Eliminate empty visit_type_FOO_fields |
Date: |
Mon, 25 Jan 2016 18:04:43 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> 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 <address@hidden>
> Reviewed-by: Marc-André Lureau <address@hidden>
>
> ---
> 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.
- [Qemu-devel] [PATCH 2/3] qapi-visit: Clean up code generated around visit_end_union(), (continued)
[Qemu-devel] [PATCH v9 18/37] qapi: Drop unused error argument for list and implicit struct, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 17/37] qapi: Drop unused 'kind' for struct/enum visit, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 29/37] qapi: Eliminate empty visit_type_FOO_fields, Eric Blake, 2016/01/19
- Re: [Qemu-devel] [PATCH v9 29/37] qapi: Eliminate empty visit_type_FOO_fields,
Markus Armbruster <=
[Qemu-devel] [PATCH v9 27/37] qapi: Add type.is_empty() helper, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 37/37] qapi: Update docs to match recent generator changes, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 22/37] qapi: Add visit_type_null() visitor, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 25/37] spapr_drc: Expose 'null' in qom-get when there is no fdt, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 24/37] qmp: Tighten output visitor rules, Eric Blake, 2016/01/19