Re: [PATCH v2 46/58] perf task-analyzer: Port task-analyzer to use python module

sashiko-bot
2026-04-23T04:30:20Z

Sashiko AI review found 4 potential issue(s):
- [Medium] Missing TGID resolution and latent AttributeError / TypeError crashes.
- [Medium] Complete removal of the Summary statistical calculations.
- [Medium] Task histories are truncated to 1000 items, destroying statistical accuracy.
- [Medium] Missing validation on --time-limit argument causes a ValueError crash.
--

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

perf task-analyzer: Port task-analyzer to use python module

This commit ports task-analyzer.py to use a class-based architecture in
tools/perf/python. It adds support for file and live modes and updates
testing to run the command directly instead of using perf script report.

> diff --git a/tools/perf/python/task-analyzer.py b/tools/perf/python/task-analyzer.py
> new file mode 100755
> index 0000000000000..869168c59207b
> --- /dev/null
> +++ b/tools/perf/python/task-analyzer.py

[ ... ]

> + def _record_cleanup(self, _list: list[Any]) -> list[Any]:
> + need_summary = (self.args.summary or self.args.summary_extended or
> + self.args.summary_only)
> + if not need_summary and len(_list) > 1:
> + return _list[len(_list) - 1:]
> + if len(_list) > 1000:
> + return _list[len(_list) - 1000:]
> + return _list

Does truncating the task history to 1000 items destroy the statistical accuracy
for tasks executed more than 1000 times? To compute summary statistics across
an entire trace, the analyzer needs to retain the full runtimes list.

[ ... ]

> + def _is_within_timelimit(self, time_ns: int) -> bool:
> + if not self.args.time_limit:
> + return True
> + time_s = decimal.Decimal(time_ns) / decimal.Decimal(1e9)
> + lower_bound, upper_bound = self.args.time_limit.split(":")

If the --time-limit argument is provided without a colon, will split(":")
return a single element and cause a ValueError crash?

[ ... ]

> + # Task finish for previous task
> + if self.session:
> + prev_tgid = self.session.process(prev_pid).pid # type: ignore
> + else:
> + prev_tgid = prev_pid # Fallback

Is self.session ever assigned? It looks like _run_file() and _run_live()
instantiate session as a local variable.

If self.session were assigned, would self.session.process(prev_pid).pid crash
with an AttributeError? The C extension perf.thread only exposes a .comm()
method, not .pid.

Also, can session.process() raise a TypeError if a thread is not found in the
perf session (e.g. the idle task)?

[ ... ]

> + def print_summary(self) -> None:
> + """Calculate and print summary."""
> + need_summary = (self.args.summary or self.args.summary_extended or
> + self.args.summary_only or self.args.csv_summary)
> + if not need_summary:
> + return
> +
> + # Simplified summary logic for brevity, full logic can be ported if needed
> + print("\nSummary (Simplified)", file=self.fd_sum)
> + if self.args.summary_extended:
> + print("Inter Task Times", file=self.fd_sum)
> + # ... port full Summary class logic here ...

Were the statistical calculations intentionally dropped here?

The original script computed Accumulated runtime, Mean, Median, Min, and Max.
The shell tests currently pass because they only grep for the word 'Summary'
instead of verifying the table contents.

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