Re: [PATCH v1 13/14] perf pmu-events: Parallelize JSON and metric pre-computation in jevents.py

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The nested `_init_worker` function causes a fatal `PicklingError` under `spawn` multiprocessing semantics.
- [High] Parallelizing the JSON parsing breaks architecture isolation for standard events, leading to state contamination across architectures.
- [Medium] The depth filtering logic is missing from the `collect_json` callback, causing redundant parallel parsing of deeply nested files.
--

commit 90db323cf161fc6e18235c0d700851677b283082
Author: Ian Rogers <irogers@google.com>

perf pmu-events: Parallelize JSON and metric pre-computation in jevents.py

This commit refactors jevents.py to pre-populate its internal JSON AST cache
in parallel using ProcessPoolExecutor. It aims to accelerate execution by
parallelizing the parsing of JSON event and metric files across available
cores.

> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index 70a45e62f5d16..eb72c154f946e 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py

[ ... ]

> @@ -1459,9 +1467,29 @@ struct pmu_table_entry {
>      raise IOError(f'Missing architecture directory \'{_args.arch}\'')
>  
>    archs.sort()
> +  import concurrent.futures
> +  tasks = []
> +  def collect_json(parents: Sequence[str], item: os.DirEntry) -> None:
> +    if len(parents) == 0:
> +      return
> +    if item.is_file() and item.name.endswith('.json') and not item.name.endswith('metricgroups.json'):
> +      tasks.append((item.path, get_topic(item.name)))

Is the depth filtering logic missing from collect_json()?

The commit message mentions applying depth filtering to prevent redundant
worker tasks. However, collect_json does not seem to limit the directory
depth.

This could cause the main thread to queue worker tasks for deeply nested
files, which are fully parsed by the worker pool but subsequently ignored
during the actual generation phase.

> +
>    for arch in archs:
>      arch_path = f'{_args.starting_dir}/{arch}'
>      preprocess_arch_std_files(arch_path)
> +    ftw(arch_path, [], collect_json)

Does this loop break architecture isolation for standard events?

By looping over all architectures to populate a single global _arch_std_events
dictionary before parsing the JSON files, standard events with identical
names from later architectures might overwrite definitions from earlier
architectures.

When the parallel worker processes evaluate the earlier architecture JSON
files, they may use this polluted global dictionary and inherit fields from
the later architectures, leading to incorrect PMU tables.

> +
> +  def _init_worker(std_events: dict) -> None:
> +    global _arch_std_events
> +    _arch_std_events = std_events
> +
> +  with concurrent.futures.ProcessPoolExecutor(initializer=_init_worker, initargs=(_arch_std_events,)) as executor:

Could defining _init_worker locally cause a pickling error under spawn
multiprocessing semantics?

_init_worker is defined as a local function inside main(). Under spawn
multiprocessing semantics, Python uses pickle to serialize objects to child
processes, but pickle cannot serialize local functions.

This regression might result in an attribute error when initializing the
executor, which seems to contradict the commit message's claim of supporting
spawn semantics.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512053539.3410189-1-irogers@google.com?part=13