From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39825) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aNkZQ-0000rg-Gx for qemu-devel@nongnu.org; Mon, 25 Jan 2016 12:04:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aNkZM-00017C-8t for qemu-devel@nongnu.org; Mon, 25 Jan 2016 12:04:52 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51628) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aNkZL-00016i-Ul for qemu-devel@nongnu.org; Mon, 25 Jan 2016 12:04:48 -0500 From: Markus Armbruster References: <1453219845-30939-1-git-send-email-eblake@redhat.com> <1453219845-30939-30-git-send-email-eblake@redhat.com> Date: Mon, 25 Jan 2016 18:04:43 +0100 In-Reply-To: <1453219845-30939-30-git-send-email-eblake@redhat.com> (Eric Blake's message of "Tue, 19 Jan 2016 09:10:37 -0700") Message-ID: <87a8nt380k.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v9 29/37] qapi: Eliminate empty visit_type_FOO_fields List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: marcandre.lureau@redhat.com, qemu-devel@nongnu.org, Michael Roth Eric Blake 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 **er= rp) > |-{ > |- Error *err =3D NULL; > |- > |- error_propagate(errp, err); > |-} > |- > | void visit_type_Abort(Visitor *v, const char *name, Abort **obj, Error = **errp) > | { > | Error *err =3D 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 > Reviewed-by: Marc-Andr=C3=A9 Lureau > > --- > 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 *na= me, %(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 =3D set() # visit_type_FOO_fields() is always emitted; track if a forward declarat= ion # or implementation has already been output. struct_fields_seen =3D 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 =3D '' > - if typ.name not in struct_fields_seen: > - ret +=3D 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, E= rror **errp); > ''', > - c_type=3Dtyp.c_name()) > - struct_fields_seen.add(typ.name) > - return ret > + c_type=3Dtyp.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, E= rror **errp); ''', c_type=3Dtyp.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, E= rror **errp); ''', c_type=3Dtyp.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 =3D gen_visit_fields_decl(typ) > Likewise. > ret +=3D 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 =3D '' > > - 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 +=3D 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=3Dc_name(name)) > > - if base: > + if base and not base.is_empty(): > ret +=3D 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 +=3D gen_visit_fields(members, prefix=3D'(*obj)->') > > - # 'goto out' produced for base, and by gen_visit_fields() for each m= ember > - if base or members: > - ret +=3D mcgen(''' > + ret +=3D mcgen(''' > > out: > -''') > - ret +=3D 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 *n= ame, %(c_name)s **obj, Error > if (!*obj) { > goto out_obj; > } > +''', > + name=3Dname, c_name=3Dc_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 +=3D mcgen(''' > visit_type_%(c_name)s_fields(v, obj, &err); > +''', > + c_name=3Dc_name(name)) > + ret +=3D mcgen(''' > out_obj: > error_propagate(errp, err); > err =3D NULL; > @@ -137,8 +143,7 @@ out_obj: > out: > error_propagate(errp, err); > } > -''', > - c_name=3Dc_name(name)) > +''') > > return ret > > @@ -300,7 +305,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *na= me, %(c_name)s **obj, Error > ''', > c_type=3Dsimple_union_type.c_name(), > c_name=3Dc_name(var.name)) > - else: > + elif not var.type.is_empty(): > ret +=3D 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.