Re: [PATCH v1 43/58] perf sched-migration: Port sched-migration/SchedGui to use python module

sashiko-bot
2026-04-20T00:33:50Z

Sashiko AI review found 5 potential issue(s):
- [High] Python 3 float division causes TypeErrors in wxPython UI layout and generates invalid fractional CPU keys, breaking click functionality.
- [High] The application crashes with an `IndexError` on startup if `perf.data` lacks scheduler events.
- [High] Event rectangle widths are calculated incorrectly, causing them to stretch and overlap across the timeline.
- [High] `__migrate_in` incorrectly mutates a shared `RunqueueSnapshot` instance, retroactively corrupting the event history of previous timeslices.
- [High] Trace rendering completely disappears if the view's start time is requested slightly before the first recorded event.
--

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

perf sched-migration: Port sched-migration/SchedGui to use python module

This commit ports sched-migration and SchedGui to use the perf.session
Python module structure. It refactors the tool to use class encapsulation
and cleans up Python 2 compatibility artifacts.

> diff --git a/tools/perf/python/SchedGui.py b/tools/perf/python/SchedGui.py
> new file mode 100755
> index 0000000000000..321b258548838
> --- /dev/null
> +++ b/tools/perf/python/SchedGui.py

[ ... ]

> + # scrollable container
> + self.scroll = wx.ScrolledWindow(self.panel)
> + self.scroll.SetScrollbars(self.scroll_scale, self.scroll_scale, self.width_virtual / self.scroll_scale, self.height_virtual / self.scroll_scale)
> + self.scroll.EnableScrolling(True, True)
> + self.scroll.SetFocus()
> +
> + # scrollable drawing area
> + self.scroll_panel = wx.Panel(self.scroll, size=(self.screen_width - 15, self.screen_height / 2))

In Python 3, division (/) returns a float. Does wxPython Phoenix strict
type checking cause a TypeError when a float is passed for the height here?
Should this use integer division (//) instead?

[ ... ]

> + def paint_rectangle_zone(self, nr, color, top_color, start, end):
> + offset_px = self.us_to_px(start - self.ts_start)
> + width_px = self.us_to_px(end - self.ts_start)

Does this calculate the absolute distance from the start of the trace to the
event's end time, rather than its duration?

If so, will this cause the drawn rectangles to be excessively wide and overlap
across the timeline? Should width_px be calculated as self.us_to_px(end - start)?

> +
> + offset_py = RootFrame.Y_OFFSET + (nr * (RootFrame.RECT_HEIGHT + RootFrame.RECT_SPACE))

[ ... ]

> + def rect_from_ypixel(self, y):
> + y -= RootFrame.Y_OFFSET
> + rect = y / (RootFrame.RECT_HEIGHT + RootFrame.RECT_SPACE)

Since this uses float division (/), will it return a fractional value?

When this is used in mouse_down() to index into ts.rqs[cpu], will it fail to
find the integer CPU key, resulting in an empty runqueue snapshot on click?
Would integer division (//) be more appropriate here?

> + height = y % (RootFrame.RECT_HEIGHT + RootFrame.RECT_SPACE)

[ ... ]

> diff --git a/tools/perf/python/sched-migration.py b/tools/perf/python/sched-migration.py
> new file mode 100755
> index 0000000000000..299c8b44064b2
> --- /dev/null
> +++ b/tools/perf/python/sched-migration.py

[ ... ]

> + def __migrate_in(self, new: int, event):
> + if new in self.tasks:
> + self.event = event
> + return self

Since TimeSlice instances share RunqueueSnapshot objects to save memory,
does mutating self.event here corrupt the recorded event history for all
previous timeslices that reference this same snapshot?

Should this return a newly instantiated RunqueueSnapshot instead?

> + next_tasks = self.tasks + tuple([new])
> +
> + return RunqueueSnapshot(next_tasks, event)

[ ... ]

> + def find_time_slice(self, ts: int) -> int:
> + """Binary search for time slice containing timestamp."""
> + start = 0
> + end = len(self.data)
> + found = -1
> + searching = True
> + while searching:
> + if start in (end, end - 1):
> + searching = False
> +
> + i = (end + start) // 2
> + if self.data[i].start <= ts <= self.data[i].end:

If the parsed perf.data file lacks scheduler events, self.data is empty.
Will this loop fail to break out when searching is set to False, leading
to an IndexError when attempting to access self.data[i] here?

> + found = i
> + break

[ ... ]

> + def fill_zone(self, start: int, end: int):
> + """Fill zone in GUI."""
> + i = self.find_time_slice(start)
> + if i == -1:
> + return

If the user scrolls such that the requested start time is slightly before
the first recorded event, find_time_slice() returns -1. Does returning
early here cause the timeline to abruptly go completely blank, even if the
end timestamp still overlaps with valid data?

Should it fall back to starting at index 0 instead of returning?

> +
> + for idx in range(i, len(self.data)):

[ ... ]

> + def nr_rectangles(self) -> int:
> + """Return maximum CPU number."""
> + last_ts = self.data[-1]

Similarly, if self.data is empty because no events were parsed, will
accessing self.data[-1] raise an IndexError and crash the application on
startup?

> + max_cpu = 0
> + for cpu in last_ts.rqs:
> + max_cpu = max(max_cpu, cpu)
> + return max_cpu

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