| Message ID | 1438703896-12553-33-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/04/2015 09:58 AM, Markus Armbruster wrote: > To eliminate the temptation for clients to look up types by name > (which are not ABI), replace all type names by meaningless strings. > > Reduces output of query-schema by 13 out of 85KiB. I'm starting to be more in favor of this patch, both for ABI reasons and for size shavings. It definitely looks better than in v2, where munged names are just an arbitrary number, and where builtins are not munged. > > TODO Either generate comments with the true type names, or provide an > option to generate without type name hiding. See also my comments on 30/32 about whether we should mask array types differently (and yes, the lack of comments made it a bit harder to chase down types for my commentary in that mail). > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > docs/qapi-code-gen.txt | 14 +++++++------- > scripts/qapi-introspect.py | 24 +++++++++++++++++++++--- > 2 files changed, 28 insertions(+), 10 deletions(-) > If we don't do anything to array names, then: Reviewed-by: Eric Blake <eblake@redhat.com> If we DO touch array names, I suspect this will look a lot different, and have to drop R-b for v4. > diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt > index ed04770..3a78cf4 100644 > --- a/docs/qapi-code-gen.txt > +++ b/docs/qapi-code-gen.txt > @@ -868,13 +868,13 @@ Example: > [Uninteresting stuff omitted...] > > const char example_qmp_schema_json[] = "[" > - "{ \"arg-type\": \":empty\", \"name\": \"MY_EVENT\", \"meta-type\": \"event\" }, " > - "{ \"meta-type\": \"builtin\", \"name\": \"int\", \"json-type\": \"int\" }, " > - "{ \"meta-type\": \"builtin\", \"name\": \"str\", \"json-type\": \"string\" }, " > - "{ \"meta-type\": \"object\", \"name\": \":empty\", \"members\": [ ] }, " > - "{ \"meta-type\": \"object\", \"name\": \":obj-my-command-arg\", \"members\": [{ \"type\": \"UserDefOne\", \"name\": \"arg1\" } ] }, " > - "{ \"meta-type\": \"object\", \"name\": \"UserDefOne\", \"members\": [{ \"type\": \"int\", \"name\": \"integer\" }, { \"type\": \"str\", \"name\": \"string\" } ] }, " > - "{ \"arg-type\": \":obj-my-command-arg\", \"ret-type\": \"UserDefOne\", \"name\": \"my-command\", \"meta-type\": \"command\" } ]"; > + "{ \"arg-type\": \"0\", \"meta-type\": \"event\", \"name\": \"MY_EVENT\" }, " > + "{ \"arg-type\": \"1\", \"meta-type\": \"command\", \"name\": \"my-command\", \"ret-type\": \"2\" }, " > + "{ \"members\": [ ], \"meta-type\": \"object\", \"name\": \"0\" }, " > + "{ \"members\": [{ \"name\": \"arg1\", \"type\": \"2\" } ], \"meta-type\": \"object\", \"name\": \"1\" }, " > + "{ \"members\": [{ \"name\": \"integer\", \"type\": \"int\" }, { \"name\": \"string\", \"type\": \"str\" } ], \"meta-type\": \"object\", \"name\": \"2\" }, " > + "{ \"json-type\": \"int\", \"meta-type\": \"builtin\", \"name\": \"int\" }, " > + "{ \"json-type\": \"string\", \"meta-type\": \"builtin\", \"name\": \"str\" } ]"; Some churn here once you fix the sorting in 30. > + > + def _name(self, name): > + if name not in self.name_map: > + n = len(self.name_map) > + self.name_map[name] = '%s' % n When string-izing an integer, isn't it better to use: self.name_map[name] = '%d' % len(self.name_map)
Eric Blake <eblake@redhat.com> writes: > On 08/04/2015 09:58 AM, Markus Armbruster wrote: >> To eliminate the temptation for clients to look up types by name >> (which are not ABI), replace all type names by meaningless strings. >> >> Reduces output of query-schema by 13 out of 85KiB. > > I'm starting to be more in favor of this patch, both for ABI reasons and > for size shavings. It definitely looks better than in v2, where munged > names are just an arbitrary number, and where builtins are not munged. Yes, much easier to read, and smaller, too. Review pays :) >> TODO Either generate comments with the true type names, or provide an >> option to generate without type name hiding. > > See also my comments on 30/32 about whether we should mask array types > differently (and yes, the lack of comments made it a bit harder to chase > down types for my commentary in that mail). > >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> docs/qapi-code-gen.txt | 14 +++++++------- >> scripts/qapi-introspect.py | 24 +++++++++++++++++++++--- >> 2 files changed, 28 insertions(+), 10 deletions(-) >> > > If we don't do anything to array names, then: > Reviewed-by: Eric Blake <eblake@redhat.com> > > If we DO touch array names, I suspect this will look a lot different, > and have to drop R-b for v4. > >> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt >> index ed04770..3a78cf4 100644 >> --- a/docs/qapi-code-gen.txt >> +++ b/docs/qapi-code-gen.txt >> @@ -868,13 +868,13 @@ Example: >> [Uninteresting stuff omitted...] >> >> const char example_qmp_schema_json[] = "[" >> - "{ \"arg-type\": \":empty\", \"name\": \"MY_EVENT\", \"meta-type\": \"event\" }, " >> - "{ \"meta-type\": \"builtin\", \"name\": \"int\", \"json-type\": \"int\" }, " >> - "{ \"meta-type\": \"builtin\", \"name\": \"str\", \"json-type\": \"string\" }, " >> - "{ \"meta-type\": \"object\", \"name\": \":empty\", \"members\": [ ] }, " >> - "{ \"meta-type\": \"object\", \"name\": \":obj-my-command-arg\", \"members\": [{ \"type\": \"UserDefOne\", \"name\": \"arg1\" } ] }, " >> - "{ \"meta-type\": \"object\", \"name\": \"UserDefOne\", \"members\": [{ \"type\": \"int\", \"name\": \"integer\" }, { \"type\": \"str\", \"name\": \"string\" } ] }, " >> - "{ \"arg-type\": \":obj-my-command-arg\", \"ret-type\": \"UserDefOne\", \"name\": \"my-command\", \"meta-type\": \"command\" } ]"; >> + "{ \"arg-type\": \"0\", \"meta-type\": \"event\", \"name\": \"MY_EVENT\" }, " >> + "{ \"arg-type\": \"1\", \"meta-type\": \"command\", \"name\": \"my-command\", \"ret-type\": \"2\" }, " >> + "{ \"members\": [ ], \"meta-type\": \"object\", \"name\": \"0\" }, " >> + "{ \"members\": [{ \"name\": \"arg1\", \"type\": \"2\" } ], \"meta-type\": \"object\", \"name\": \"1\" }, " >> + "{ \"members\": [{ \"name\": \"integer\", \"type\": \"int\" }, { \"name\": \"string\", \"type\": \"str\" } ], \"meta-type\": \"object\", \"name\": \"2\" }, " >> + "{ \"json-type\": \"int\", \"meta-type\": \"builtin\", \"name\": \"int\" }, " >> + "{ \"json-type\": \"string\", \"meta-type\": \"builtin\", \"name\": \"str\" } ]"; > > Some churn here once you fix the sorting in 30. > >> + >> + def _name(self, name): >> + if name not in self.name_map: >> + n = len(self.name_map) >> + self.name_map[name] = '%s' % n > > When string-izing an integer, isn't it better to use: > > self.name_map[name] = '%d' % len(self.name_map) Yes, will change. Thanks!
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt index ed04770..3a78cf4 100644 --- a/docs/qapi-code-gen.txt +++ b/docs/qapi-code-gen.txt @@ -868,13 +868,13 @@ Example: [Uninteresting stuff omitted...] const char example_qmp_schema_json[] = "[" - "{ \"arg-type\": \":empty\", \"name\": \"MY_EVENT\", \"meta-type\": \"event\" }, " - "{ \"meta-type\": \"builtin\", \"name\": \"int\", \"json-type\": \"int\" }, " - "{ \"meta-type\": \"builtin\", \"name\": \"str\", \"json-type\": \"string\" }, " - "{ \"meta-type\": \"object\", \"name\": \":empty\", \"members\": [ ] }, " - "{ \"meta-type\": \"object\", \"name\": \":obj-my-command-arg\", \"members\": [{ \"type\": \"UserDefOne\", \"name\": \"arg1\" } ] }, " - "{ \"meta-type\": \"object\", \"name\": \"UserDefOne\", \"members\": [{ \"type\": \"int\", \"name\": \"integer\" }, { \"type\": \"str\", \"name\": \"string\" } ] }, " - "{ \"arg-type\": \":obj-my-command-arg\", \"ret-type\": \"UserDefOne\", \"name\": \"my-command\", \"meta-type\": \"command\" } ]"; + "{ \"arg-type\": \"0\", \"meta-type\": \"event\", \"name\": \"MY_EVENT\" }, " + "{ \"arg-type\": \"1\", \"meta-type\": \"command\", \"name\": \"my-command\", \"ret-type\": \"2\" }, " + "{ \"members\": [ ], \"meta-type\": \"object\", \"name\": \"0\" }, " + "{ \"members\": [{ \"name\": \"arg1\", \"type\": \"2\" } ], \"meta-type\": \"object\", \"name\": \"1\" }, " + "{ \"members\": [{ \"name\": \"integer\", \"type\": \"int\" }, { \"name\": \"string\", \"type\": \"str\" } ], \"meta-type\": \"object\", \"name\": \"2\" }, " + "{ \"json-type\": \"int\", \"meta-type\": \"builtin\", \"name\": \"int\" }, " + "{ \"json-type\": \"string\", \"meta-type\": \"builtin\", \"name\": \"str\" } ]"; $ cat qapi-generated/example-qmp-introspect.h [Uninteresting stuff omitted...] diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py index 05d30e5..5e99d4b 100644 --- a/scripts/qapi-introspect.py +++ b/scripts/qapi-introspect.py @@ -42,25 +42,29 @@ class QAPISchemaGenIntrospectVisitor(QAPISchemaVisitor): self.schema = None self.jsons = None self.used_types = None + self.name_map = None def visit_begin(self, schema): self.schema = schema self.jsons = [] self.used_types = [] + self.name_map = {} return QAPISchemaType # don't visit types for now def visit_end(self): # visit the types that are actually used + jsons = self.jsons + self.jsons = [] for typ in self.used_types: typ.visit(self) - self.jsons.sort() # generate C + jsons.extend(self.jsons) name = prefix + 'qmp_schema_json' self.decl = mcgen(''' extern const char %(c_name)s[]; ''', c_name=c_name(name)) - lines = to_json(self.jsons).split('\n') + lines = to_json(jsons).split('\n') c_string = '\n '.join([to_c_string(line) for line in lines]) self.defn = mcgen(''' const char %(c_name)s[] = %(c_string)s; @@ -70,6 +74,13 @@ const char %(c_name)s[] = %(c_string)s; self.schema = None self.jsons = None self.used_types = None + self.name_map = None + + def _name(self, name): + if name not in self.name_map: + n = len(self.name_map) + self.name_map[name] = '%s' % n + return self.name_map[name] def _use_type(self, typ): # Map the various integer types to plain int @@ -81,9 +92,16 @@ const char %(c_name)s[] = %(c_string)s; # Add type to work queue if new if typ not in self.used_types: self.used_types.append(typ) - return typ.name + # Clients should examine commands and events, not types. Hide + # type names to reduce the temptation. Also saves a few + # characters. + if isinstance(typ, QAPISchemaBuiltinType): + return typ.name + return self._name(typ.name) def _gen_json(self, name, mtype, obj={}): + if mtype != 'command' and mtype != 'event' and mtype != 'builtin': + name = self._name(name) obj['name'] = name obj['meta-type'] = mtype self.jsons.append(obj)
To eliminate the temptation for clients to look up types by name (which are not ABI), replace all type names by meaningless strings. Reduces output of query-schema by 13 out of 85KiB. TODO Either generate comments with the true type names, or provide an option to generate without type name hiding. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- docs/qapi-code-gen.txt | 14 +++++++------- scripts/qapi-introspect.py | 24 +++++++++++++++++++++--- 2 files changed, 28 insertions(+), 10 deletions(-)