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

[Wenlei He via Phabricator via llvm-commits]

wenlei 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?
> 
>   
> > 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.

We need to use pid to filter mmap event, otherwise we don't know which mmap to use to set base address and the result would be wrong, so pid is meaningful for perf script. 

> 
> > 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. 
>   

Maybe I didn't understand what you're trying to say. I don't see a problem because: 1) there's no fundamental difference between perf data and perf script; 2) By the name `PerfReaderBase`, the entire hierarchy is perf specific; 3) then adding pid to general interface isn't a problem. 

Also I don't see pid is being very different from binary name, it's another filter, similar to binary name. 

> Do you see with pid filter, `convertPerfDataToTrace` still gives mixed mmap evens from all processes?

Yes, if you are asking about output from `perf script ..` with pid filter. mmap from processes are still there. 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123869