[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);
 
----------------
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?

  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123869