mstange / samply

Command-line sampling profiler for macOS and Linux
Apache License 2.0
1.96k stars 48 forks source link

Ignore early COMM records emitted by simpleperf. #246

Closed mstange closed 1 month ago

mstange commented 1 month ago

Before: https://share.firefox.dev/3RjMLuv After: https://share.firefox.dev/3RgpGsz

simpleperf is very eager about putting thread names in the perf.data file: When it processes the first sample in a process, it lists the threads in that process and emits COMM records for all of them, with a timestamp that matches the first sample. It does this both for procesess that already exist when profiling starts, and for new processes that are created during profiling.

The trouble is that the enumeration of threads happens long after the kernel collects the first sample - perf event records are buffered and read in chunks, so by the time simpleperf reads the sample, a long time can have elapsed. In the meantime, a lot of new threads can already have been created - but these threads were created after the kernel collected the first sample. simpleperf then emits COMM events for those new threads, with the timestamp for a sample that occurred before these threads existed.

samply uses the timestamp of the first event for a each TID to mark the thread's start time. The Firefox Profiler UI shows the start of a thread visually. Since the COMM event was the first event, and since it has a timestamp that's too early, the Firefox Profiler UI was showing the thread starting too early.

This fix overwrites a thread's start time with the timestamp of the FORK record for any existing threads which don't have any samples yet.

It would also be good if simpleperf stopped emitting the extra COMM records. It could just do the existing thread enumeration only for processes that exist at the start of profiling and not for processes that get created during profiling - the kernel will emit FORK and COMM records for any new threads in those processes and those are sufficient.