[PATCH] D123869: [llvm-profgen] Add process filter for perf reader

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.h:535
+  static std::unique_ptr<PerfReaderBase>
+  create(ProfiledBinary *Binary, PerfInputFile &PerfInput, uint32_t PIDFilter);
 
----------------
hoy wrote:
> wenlei wrote:
> > hoy wrote:
> > > Can we just check against the switch `ProcessId` in `convertPerfDataToTrace`? Feel like PID is perf data specific and changing the general interface here may not be necessary. We have other filters that are for a particular reader, as an example `ignore-stack-samples`.
> > > Can we just check against the switch ProcessId in convertPerfDataToTrace?
> > 
> > We also need to check PID in updateBinaryAddress. I think PID is in the same category as Binary, both are filters for perf events. 
> > 
> > > Feel like PID is perf data specific and changing the general interface here may not be necessary. 
> > 
> > Isn't any perf reader type under PerfReaderBase supposed to be perf data specific? The only thing that is not perf data related is UnsymbolizedProfileReader, but then I think the real problem is really why we have UnsymbolizedProfileReader inheriting from perf reader while it's clearly not reading perf data/script.
> > We also need to check PID in updateBinaryAddress.
> 
> The PID filter isn't that meaningful in perf script file. Though we can still use pid to filter out the mmap events there, but the main LBR/stack samples mixed from multiple processes are not separable. So PID filter should be applied when converting perf.data to perf script, which makes me think it is perf data specific.
> 
> > Isn't any perf reader type under PerfReaderBase supposed to be perf data specific? 
> 
> Not necessarily. Perf script and our internal lbr profile do not see 
> everything that perf.data has. The two formats assume all data in them are for one process. Do you see with pid filter, `convertPerfDataToTrace` still gives mixed mmap evens from all processes?
> 
>   
I see, looks like mmap events are special from other sample events in that they cannot be filtered by pid. I was hoping that perf script output could be made dedicate to a process since that's what `perf script --pid` is shooting for? Once filtered, the processing of perf script output should no longer need to deal with pid again. This sounds like a bug of perf script.

The `PerfReaderBase` hierarchy seems improper in naming. Both perf.data and perf script output are processed by `PerfScriptReader` and the base class `PerfReaderBase` is there only to provide an interface to create a specific reader instance, including `UnsymbolizedProfileReader`. I think the intention of including `UnsymbolizedProfileReader` in the hierarchy was to avoid a special path in the driver, but then `PerfReaderBase` should be named `ReaderBase` or something. 

It then makes sense to pass pid to the perf reader hierarchy and separating `UnsymbolizedProfileReader` out. The separation can be done in a different patch. 

Use `optional<uint32_t>` for `PIDFilter` in case zero is a valid kernel process?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123869/new/

https://reviews.llvm.org/D123869