Sort the json files entries on conversion to C. The sort order tries to
replicated cmp_sevent from pmu.c so that the input there is already
sorted except for sysfs events.

Add the topic to JsonEvent on reading to simplify. Remove an unnecessary
lambda in the json reading.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/pmu-events/jevents.py | 48 +++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 13 deletions(-)

diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 12d2daf3570c..30e0e792221a 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -18,6 +18,8 @@ _sys_event_tables = []
 _arch_std_events = {}
 # Track whether an events table is currently being defined and needs closing.
 _close_table = False
+# Events to write out when the table is closed
+_pending_events = []
 
 
 def removesuffix(s: str, suffix: str) -> str:
@@ -127,6 +129,7 @@ class JsonEvent:
       eventcode |= int(jd['ExtSel']) << 8
     configcode = int(jd['ConfigCode'], 0) if 'ConfigCode' in jd else None
     self.name = jd['EventName'].lower() if 'EventName' in jd else None
+    self.topic = ''
     self.compat = jd.get('Compat')
     self.desc = fixdesc(jd.get('BriefDescription'))
     self.long_desc = fixdesc(jd.get('PublicDescription'))
@@ -199,7 +202,7 @@ class JsonEvent:
         s += f'\t{attr} = {value},\n'
     return s + '}'
 
-  def to_c_string(self, topic_local: str) -> str:
+  def to_c_string(self) -> str:
     """Representation of the event as a C struct initializer."""
 
     def attr_string(attr: str, value: str) -> str:
@@ -211,25 +214,27 @@ class JsonEvent:
       return attr_string(attr, getattr(self, attr))
 
     s = '{\n'
-    s += f'\t.topic = "{topic_local}",\n'
     for attr in [
         'aggr_mode', 'compat', 'deprecated', 'desc', 'event', 'long_desc',
         'metric_constraint', 'metric_expr', 'metric_group', 'metric_name',
-        'name', 'perpkg', 'pmu', 'unit'
+        'name', 'perpkg', 'pmu', 'topic', 'unit'
     ]:
       s += str_if_present(self, attr)
     s += '},\n'
     return s
 
 
-def read_json_events(path: str) -> Sequence[JsonEvent]:
+def read_json_events(path: str, topic: str) -> Sequence[JsonEvent]:
   """Read json events from the specified file."""
 
   try:
-    return json.load(open(path), object_hook=lambda d: JsonEvent(d))
+    result = json.load(open(path), object_hook=JsonEvent)
   except BaseException as err:
     print(f"Exception processing {path}")
     raise
+  for event in result:
+    event.topic = topic
+  return result
 
 
 def preprocess_arch_std_files(archpath: str) -> None:
@@ -237,7 +242,7 @@ def preprocess_arch_std_files(archpath: str) -> None:
   global _arch_std_events
   for item in os.scandir(archpath):
     if item.is_file() and item.name.endswith('.json'):
-      for event in read_json_events(item.path):
+      for event in read_json_events(item.path, topic=''):
         if event.name:
           _arch_std_events[event.name.lower()] = event
 
@@ -251,19 +256,36 @@ def print_events_table_prefix(tblname: str) -> None:
   _close_table = True
 
 
-def print_events_table_entries(item: os.DirEntry, topic: str) -> None:
-  """Create contents of an events table."""
+def add_events_table_entries(item: os.DirEntry, topic: str) -> None:
+  """Add contents of file to _pending_events table."""
   if not _close_table:
     raise IOError('Table entries missing prefix')
-  for event in read_json_events(item.path):
-    _args.output_file.write(event.to_c_string(topic))
+  for e in read_json_events(item.path, topic):
+    _pending_events.append(e)
 
 
 def print_events_table_suffix() -> None:
   """Optionally close events table."""
+
+  def event_cmp_key(j: JsonEvent):
+    def fix_none(s: str):
+      if s is None:
+        return ''
+      return s
+
+    return (not j.desc is None, fix_none(j.topic), fix_none(j.name), fix_none(j.pmu),
+            fix_none(j.metric_name))
+
   global _close_table
-  if _close_table:
-    _args.output_file.write("""{
+  if not _close_table:
+    return
+
+  global _pending_events
+  for event in sorted(_pending_events, key=event_cmp_key):
+    _args.output_file.write(event.to_c_string())
+    _pending_events = []
+
+  _args.output_file.write("""{
 \t.name = 0,
 \t.event = 0,
 \t.desc = 0,
@@ -306,7 +328,7 @@ def process_one_file(parents: Sequence[str], item: os.DirEntry) -> None:
   if not item.is_file() or not item.name.endswith('.json'):
     return
 
-  print_events_table_entries(item, get_topic(item.name))
+  add_events_table_entries(item, get_topic(item.name))
 
 
 def print_mapping_table(archs: Sequence[str]) -> None:
-- 
2.37.1.559.g78731f0fdb-goog

On 04/08/2022 23:18, Ian Rogers wrote:
> Sort the json files entries on conversion to C. The sort order tries to
> replicated cmp_sevent from pmu.c so that the input there is already

replicate

> sorted except for sysfs events.
> 
> Add the topic to JsonEvent on reading to simplify. Remove an unnecessary
> lambda in the json reading.

We sort the attributes of the events alphabetically by attribute name, 
right? Is there any advantage in this? Do we need it for later?

thanks,
John

> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>   tools/perf/pmu-events/jevents.py | 48 +++++++++++++++++++++++---------
>   1 file changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index 12d2daf3570c..30e0e792221a 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py
> @@ -18,6 +18,8 @@ _sys_event_tables = []
>   _arch_std_events = {}
>   # Track whether an events table is currently being defined and needs closing.
>   _close_table = False
> +# Events to write out when the table is closed
> +_pending_events = []
>   
>   
>   def removesuffix(s: str, suffix: str) -> str:
> @@ -127,6 +129,7 @@ class JsonEvent:
>         eventcode |= int(jd['ExtSel']) << 8
>       configcode = int(jd['ConfigCode'], 0) if 'ConfigCode' in jd else None
>       self.name = jd['EventName'].lower() if 'EventName' in jd else None
> +    self.topic = ''
>       self.compat = jd.get('Compat')
>       self.desc = fixdesc(jd.get('BriefDescription'))
>       self.long_desc = fixdesc(jd.get('PublicDescription'))
> @@ -199,7 +202,7 @@ class JsonEvent:
>           s += f'\t{attr} = {value},\n'
>       return s + '}'
>   
> -  def to_c_string(self, topic_local: str) -> str:
> +  def to_c_string(self) -> str:
>       """Representation of the event as a C struct initializer."""
>   
>       def attr_string(attr: str, value: str) -> str:
> @@ -211,25 +214,27 @@ class JsonEvent:
>         return attr_string(attr, getattr(self, attr))
>   
>       s = '{\n'
> -    s += f'\t.topic = "{topic_local}",\n'
>       for attr in [
>           'aggr_mode', 'compat', 'deprecated', 'desc', 'event', 'long_desc',
>           'metric_constraint', 'metric_expr', 'metric_group', 'metric_name',
> -        'name', 'perpkg', 'pmu', 'unit'
> +        'name', 'perpkg', 'pmu', 'topic', 'unit'
>       ]:
>         s += str_if_present(self, attr)
>       s += '},\n'
>       return s
>   
>   
> -def read_json_events(path: str) -> Sequence[JsonEvent]:
> +def read_json_events(path: str, topic: str) -> Sequence[JsonEvent]:
>     """Read json events from the specified file."""
>   
>     try:
> -    return json.load(open(path), object_hook=lambda d: JsonEvent(d))
> +    result = json.load(open(path), object_hook=JsonEvent)
>     except BaseException as err:
>       print(f"Exception processing {path}")
>       raise
> +  for event in result:
> +    event.topic = topic
> +  return result
>   
>   
>   def preprocess_arch_std_files(archpath: str) -> None:
> @@ -237,7 +242,7 @@ def preprocess_arch_std_files(archpath: str) -> None:
>     global _arch_std_events
>     for item in os.scandir(archpath):
>       if item.is_file() and item.name.endswith('.json'):
> -      for event in read_json_events(item.path):
> +      for event in read_json_events(item.path, topic=''):
>           if event.name:
>             _arch_std_events[event.name.lower()] = event
>   
> @@ -251,19 +256,36 @@ def print_events_table_prefix(tblname: str) -> None:
>     _close_table = True
>   
>   
> -def print_events_table_entries(item: os.DirEntry, topic: str) -> None:
> -  """Create contents of an events table."""
> +def add_events_table_entries(item: os.DirEntry, topic: str) -> None:
> +  """Add contents of file to _pending_events table."""
>     if not _close_table:
>       raise IOError('Table entries missing prefix')
> -  for event in read_json_events(item.path):
> -    _args.output_file.write(event.to_c_string(topic))
> +  for e in read_json_events(item.path, topic):
> +    _pending_events.append(e)
>   
>   
>   def print_events_table_suffix() -> None:
>     """Optionally close events table."""
> +
> +  def event_cmp_key(j: JsonEvent):
> +    def fix_none(s: str):
> +      if s is None:
> +        return ''
> +      return s
> +
> +    return (not j.desc is None, fix_none(j.topic), fix_none(j.name), fix_none(j.pmu),
> +            fix_none(j.metric_name))
> +
>     global _close_table
> -  if _close_table:
> -    _args.output_file.write("""{
> +  if not _close_table:
> +    return
> +
> +  global _pending_events
> +  for event in sorted(_pending_events, key=event_cmp_key):
> +    _args.output_file.write(event.to_c_string())
> +    _pending_events = []
> +
> +  _args.output_file.write("""{
>   \t.name = 0,
>   \t.event = 0,
>   \t.desc = 0,
> @@ -306,7 +328,7 @@ def process_one_file(parents: Sequence[str], item: os.DirEntry) -> None:
>     if not item.is_file() or not item.name.endswith('.json'):
>       return
>   
> -  print_events_table_entries(item, get_topic(item.name))
> +  add_events_table_entries(item, get_topic(item.name))
>   
>   
>   def print_mapping_table(archs: Sequence[str]) -> None:

On Fri, Aug 5, 2022 at 3:49 AM John Garry <john.garry@huawei.com> wrote:
>
> On 04/08/2022 23:18, Ian Rogers wrote:
> > Sort the json files entries on conversion to C. The sort order tries to
> > replicated cmp_sevent from pmu.c so that the input there is already
>
> replicate

Ack.

> > sorted except for sysfs events.
> >
> > Add the topic to JsonEvent on reading to simplify. Remove an unnecessary
> > lambda in the json reading.
>
> We sort the attributes of the events alphabetically by attribute name,
> right? Is there any advantage in this? Do we need it for later?


The sort order is given by the tuple:
(not j.desc is None, fix_none(j.topic), fix_none(j.name),
fix_none(j.pmu), fix_none(j.metric_name))
which is putting events with descriptions and topics before those
without, then sorting by name, then pmu and finally metric_name. The
advantage is that when we qsort alias events:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/pmu.c?h=perf/core#n1759
the events are already in the sorted format, which should make the
code faster - it still has to qsort the sysfs events.

Longer term I'd like to make the searching for an event, or metric, by
name a binary rather than a linear search.

Thanks,
Ian

> thanks,
> John
>
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >   tools/perf/pmu-events/jevents.py | 48 +++++++++++++++++++++++---------
> >   1 file changed, 35 insertions(+), 13 deletions(-)
> >
> > diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> > index 12d2daf3570c..30e0e792221a 100755
> > --- a/tools/perf/pmu-events/jevents.py
> > +++ b/tools/perf/pmu-events/jevents.py
> > @@ -18,6 +18,8 @@ _sys_event_tables = []
> >   _arch_std_events = {}
> >   # Track whether an events table is currently being defined and needs closing.
> >   _close_table = False
> > +# Events to write out when the table is closed
> > +_pending_events = []
> >
> >
> >   def removesuffix(s: str, suffix: str) -> str:
> > @@ -127,6 +129,7 @@ class JsonEvent:
> >         eventcode |= int(jd['ExtSel']) << 8
> >       configcode = int(jd['ConfigCode'], 0) if 'ConfigCode' in jd else None
> >       self.name = jd['EventName'].lower() if 'EventName' in jd else None
> > +    self.topic = ''
> >       self.compat = jd.get('Compat')
> >       self.desc = fixdesc(jd.get('BriefDescription'))
> >       self.long_desc = fixdesc(jd.get('PublicDescription'))
> > @@ -199,7 +202,7 @@ class JsonEvent:
> >           s += f'\t{attr} = {value},\n'
> >       return s + '}'
> >
> > -  def to_c_string(self, topic_local: str) -> str:
> > +  def to_c_string(self) -> str:
> >       """Representation of the event as a C struct initializer."""
> >
> >       def attr_string(attr: str, value: str) -> str:
> > @@ -211,25 +214,27 @@ class JsonEvent:
> >         return attr_string(attr, getattr(self, attr))
> >
> >       s = '{\n'
> > -    s += f'\t.topic = "{topic_local}",\n'
> >       for attr in [
> >           'aggr_mode', 'compat', 'deprecated', 'desc', 'event', 'long_desc',
> >           'metric_constraint', 'metric_expr', 'metric_group', 'metric_name',
> > -        'name', 'perpkg', 'pmu', 'unit'
> > +        'name', 'perpkg', 'pmu', 'topic', 'unit'
> >       ]:
> >         s += str_if_present(self, attr)
> >       s += '},\n'
> >       return s
> >
> >
> > -def read_json_events(path: str) -> Sequence[JsonEvent]:
> > +def read_json_events(path: str, topic: str) -> Sequence[JsonEvent]:
> >     """Read json events from the specified file."""
> >
> >     try:
> > -    return json.load(open(path), object_hook=lambda d: JsonEvent(d))
> > +    result = json.load(open(path), object_hook=JsonEvent)
> >     except BaseException as err:
> >       print(f"Exception processing {path}")
> >       raise
> > +  for event in result:
> > +    event.topic = topic
> > +  return result
> >
> >
> >   def preprocess_arch_std_files(archpath: str) -> None:
> > @@ -237,7 +242,7 @@ def preprocess_arch_std_files(archpath: str) -> None:
> >     global _arch_std_events
> >     for item in os.scandir(archpath):
> >       if item.is_file() and item.name.endswith('.json'):
> > -      for event in read_json_events(item.path):
> > +      for event in read_json_events(item.path, topic=''):
> >           if event.name:
> >             _arch_std_events[event.name.lower()] = event
> >
> > @@ -251,19 +256,36 @@ def print_events_table_prefix(tblname: str) -> None:
> >     _close_table = True
> >
> >
> > -def print_events_table_entries(item: os.DirEntry, topic: str) -> None:
> > -  """Create contents of an events table."""
> > +def add_events_table_entries(item: os.DirEntry, topic: str) -> None:
> > +  """Add contents of file to _pending_events table."""
> >     if not _close_table:
> >       raise IOError('Table entries missing prefix')
> > -  for event in read_json_events(item.path):
> > -    _args.output_file.write(event.to_c_string(topic))
> > +  for e in read_json_events(item.path, topic):
> > +    _pending_events.append(e)
> >
> >
> >   def print_events_table_suffix() -> None:
> >     """Optionally close events table."""
> > +
> > +  def event_cmp_key(j: JsonEvent):
> > +    def fix_none(s: str):
> > +      if s is None:
> > +        return ''
> > +      return s
> > +
> > +    return (not j.desc is None, fix_none(j.topic), fix_none(j.name), fix_none(j.pmu),
> > +            fix_none(j.metric_name))
> > +
> >     global _close_table
> > -  if _close_table:
> > -    _args.output_file.write("""{
> > +  if not _close_table:
> > +    return
> > +
> > +  global _pending_events
> > +  for event in sorted(_pending_events, key=event_cmp_key):
> > +    _args.output_file.write(event.to_c_string())
> > +    _pending_events = []
> > +
> > +  _args.output_file.write("""{
> >   \t.name = 0,
> >   \t.event = 0,
> >   \t.desc = 0,
> > @@ -306,7 +328,7 @@ def process_one_file(parents: Sequence[str], item: os.DirEntry) -> None:
> >     if not item.is_file() or not item.name.endswith('.json'):
> >       return
> >
> > -  print_events_table_entries(item, get_topic(item.name))
> > +  add_events_table_entries(item, get_topic(item.name))
> >
> >
> >   def print_mapping_table(archs: Sequence[str]) -> None:
>

On 10/08/2022 15:23, Ian Rogers wrote:
>> We sort the attributes of the events alphabetically by attribute name,
>> right? Is there any advantage in this? Do we need it for later?
> 
> The sort order is given by the tuple:
> (not j.desc is None, fix_none(j.topic), fix_none(j.name),
> fix_none(j.pmu), fix_none(j.metric_name))
> which is putting events with descriptions and topics before those
> without, then sorting by name, then pmu and finally metric_name. The
> advantage is that when we qsort alias events:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/pmu.c?h=perf/core#n1759
> the events are already in the sorted format, which should make the
> code faster -

ok, so can you mention that in the commit message? Just writing that you 
want to replicate cmp_sevent from pmu.c does not tell us this clearly.

 > it still has to qsort the sysfs events.

thanks,
John