Re: [PATCH v2 15/18] 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:
- [Medium] The `_init_worker` function is defined as a local function, which breaks the build under `spawn` multiprocessing semantics because it cannot be pickled.
- [Medium] Parallelizing the JSON parsing breaks architecture isolation, allowing standard events from later architectures to overwrite those of earlier architectures.
- [Low] The `collect_json` callback omits the directory depth filter, leading to redundant parsing of deeply nested JSON files.
--

commit a8c22dabeaaa676d9d6b5a7b1c0a3df0bfcd2345
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 across all available CPU cores using ProcessPoolExecutor. It aims
to accelerate the execution time by reclaiming multi-core concurrency during
the build generation phase.

> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> --- 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'):

The commit message states the code will "apply depth filtering to prevent
redundant worker tasks".

Should there be a check on the depth of the directory structure here, to
avoid redundantly parsing deeply nested JSON files?

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

By preprocessing all architecture standard files before the worker pool
evaluates the tasks, does this break architecture isolation?

Because archs.sort() evaluates architectures alphabetically, it appears that
architectures processed later might overwrite overlapping standard event
definitions of earlier architectures in the global _arch_std_events dictionary.

When the worker pool executes, will earlier architectures incorrectly resolve
standard events against these overwritten definitions?

> +
> +  def _init_worker(std_events: dict) -> None:

Is it possible to use a local function as an initializer for
ProcessPoolExecutor when using the spawn multiprocessing start method?

The commit message notes it guarantees inheritance under spawn semantics.
However, Python's multiprocessing framework uses pickle to serialize the
initializer function for child processes, and local/nested functions cannot be
pickled. Will this cause an AttributeError and crash the build?

> +    global _arch_std_events
> +    _arch_std_events = std_events
> +
> +  with concurrent.futures.ProcessPoolExecutor(initializer=_init_worker, initargs=(_arch_std_events,)) as executor:
> +    for path, topic, events in executor.map(_parallel_read_json_events, tasks):
> +      _json_cache[(path, topic)] = events

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