Re: [PATCH v5 35/58] perf event_analyzing_sample: Port event_analyzing_sample to use python module

sashiko-bot
2026-04-24T17:12:59Z

Sashiko AI review found 2 potential issue(s):
- [High] Severe performance degradation due to SQLite autocommit mode on persistent disk.
- [High] Uncaught OverflowError during SQLite insertion crashes the script.
--

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

perf event_analyzing_sample: Port event_analyzing_sample to use python module

This commit adds a port of the event_analyzing_sample script that uses the
perf python module directly. This avoids creating intermediate dictionaries
for all event fields, resulting in faster processing.

> diff --git a/tools/perf/python/event_analyzing_sample.py b/tools/perf/python/event_analyzing_sample.py
> new file mode 100755
> index 00000000000000..7507917e16be37
> --- /dev/null
> +++ b/tools/perf/python/event_analyzing_sample.py
[ ... ]
> +def trace_begin(db_path: str) -> None:
> + """Initialize database tables."""
> + print("In trace_begin:\n")
> + global con
> + con = sqlite3.connect(db_path)
> + con.isolation_level = None

Does setting the isolation level to None cause a performance regression now
that the default database path has changed to a persistent disk?

With the default path changing from /dev/shm/perf.db to perf.db,
disabling implicit transactions means every insert statement executes
as its own transaction. This forces a disk sync for every single event
and can cause massive overhead for typical traces.

Would it be better to remove this isolation level setting and explicitly
commit the transaction in trace_end() instead?

> + assert con is not None
> +
> + # Will create several tables at the start, pebs_ll is for PEBS data with
[ ... ]
> +def process_event(sample: perf.sample_event) -> None:
> + """Callback for processing events."""
[ ... ]
> + # Create the event object and insert it to the right table in database
> + try:
> + event = create_event(name, comm, dso, symbol, sample.raw_buf)
> + insert_db(event)
> + except (sqlite3.Error, ValueError, TypeError) as e:

Could this exception block miss an OverflowError on 64-bit unsigned integers?

If the event status has bit 63 set, the sqlite3 driver will raise an
OverflowError since it natively supports up to 64-bit signed integers.
Since OverflowError inherits from ArithmeticError, it bypasses this catch
block and terminates the processing loop prematurely.

> + print(f"Error creating/inserting event: {e}")

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