Date:   Wed, 10 Aug 2022 07:23:53 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     John Garry <john.garry@...wei.com>
Cc:     Will Deacon <will@...nel.org>, James Clark <james.clark@....com>,
        Mike Leach <mike.leach@...aro.org>,
        Leo Yan <leo.yan@...aro.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Andi Kleen <ak@...ux.intel.com>,
        Zhengjun Xing <zhengjun.xing@...ux.intel.com>,
        Ravi Bangoria <ravi.bangoria@....com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-perf-users@...r.kernel.org,
        Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH v4 07/17] perf jevents: Sort json files entries

On Fri, Aug 5, 2022 at 3:49 AM John Garry <john.garry@...wei.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@...gle.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:
>

Powered by blists - more mailing lists