Message ID | 1453219845-30939-30-git-send-email-eblake@redhat.com |
---|---|
State | New |
Headers | show |
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.
On 01/25/2016 10:04 AM, Markus Armbruster wrote: > 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: >> >> 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. >> > > 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. We managed to pull off unification without this patch, and your argument about making the generator more verbose with no-ops if it is less maintenance is still resonating with me. I think I'm going to drop this and 30/37 in my next round of patches, with no real loss in functionality.
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 ** 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()) 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) 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 + + if base and not base.is_empty(): ret += gen_visit_fields_decl(base) 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); } ''') @@ -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: + 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); ''',