On Wed, May 27, 2020 at 1:59 PM Ian Rogers <irog...@google.com> wrote: > > On Wed, May 27, 2020 at 12:27 PM Steve MacLean > <steve.macl...@microsoft.com> wrote: > > > > >> ** Implemented solution > > >> > > >> This patch solves the issue by removing // anon mmap events for any > > >> process which has a valid jit-<pid>.dump file. > > >> > > >> It tracks on a per process basis to handle the case where some running > > >> apps support jit-<pid>.dump, but some only support perf-<pid>.map. > > >> > > >> It adds new assumptions: > > >> * // anon mmap events are only required for perf-<pid>.map support. > > >> * An app that uses jit-<pid>.dump, no longer needs perf-<pid>.map > > >> support. It assumes that any perf-<pid>.map info is inferior. > > >> > > >> *** Details > > >> > > >> Use thread->priv to store whether a jitdump file has been processed > > >> > > >> During "perf inject --jit", discard "//anon*" mmap events for any pid > > >> which has sucessfully processed a jitdump file. > > > > > > > > > Thanks Steve this is an important fix! As //anon could be for malloc or > > > other uses, should the stripping behavior be behind a flag? > > > > > > Ian > > > > I hadn't anticipated a need to preserve the //anon mmap events when > > profiling JIT generated code. > > > > As far as I know mmap events are captured by perf only for mapping code to > > symbols. File mappings are kept > > by the change. Only // anon mappings are stripped. (Only for processes > > which emitted jitdump files.) > > And these are stripped only during the `perf inject --jit` step. I believe > > the // Anon mapping are only > > generally useful for mapping JIT code. > > > > I suppose if someone was trying to count mmap events it might be confusing, > > but `perf inject --jit` creates > > synthetic mmap file events which would also make this scenario confusing. > > > > I personally don't see a good reason to add a flag. I also don't see a > > simple way either. Not running `perf inject --jit` > > would preserve existing behavior w/o jitdump support. Without stripping > > the anon events jitdump support is painfully > > broken.... > > Agreed that things are broken. In general only executable mappings are > held onto by perf, so it could be I'm over worrying about //anon > stripping breaking around memory allocations. We have some other use > cases for //anon at Google but they aren't impacted by jitdump. We > have also been trying to migrate jit caches to using memfd_create, > which has the same problem that this patch fixes for //anon. Fixing > memfd_create is a separate issue to //anon. I'll try to get a repro > for Java that demonstrates the problem and then add a Tested-by. > > Thanks, > Ian
So on tip/perf/core with: 1c0cd2dbb993 perf jvmti: Fix jitdump for methods without debug info 3ce17c1e52f4 perf jvmti: remove redundant jitdump line table entries I've been trying variants of: Before: /tmp/perf/perf record -k 1 -e cycles:u -o /tmp/perf.data java -agentpath:/tmp/perf/libperf-jvmti.so -XX:+PreserveFramePointer -XX:InitialCodeCacheSize=20M -XX:ReservedCodeCacheSize=1G -jar dacapo-9.12-bach.jar jython /tmp/perf/perf inject -i /tmp/perf.data -o /tmp/perf-jit.data -j /tmp/perf/perf report -i /tmp/perf-jit.data |grep class\ |wc -l 578 /tmp/perf/perf report -i /tmp/perf-jit.data |grep unknown |wc -l 6 After: /tmp/perf/perf record -k 1 -e cycles:u -o /tmp/perf.data java -agentpath:/tmp/perf/libperf-jvmti.so -XX:+PreserveFramePointer -XX:InitialCodeCacheSize=20M -XX:ReservedCodeCacheSize=1G -jar dacapo-9.12-bach.jar jython /tmp/perf/perf inject -i /tmp/perf.data -o /tmp/perf-jit.data -j /tmp/perf/perf report -i /tmp/perf-jit.data |grep class\ |wc -l 589 /tmp/perf/perf report -i /tmp/perf-jit.data |grep unknown |wc -l 60 So maybe the jit cache isn't behaving the way that the patch cures, the uptick in unknowns appears consistent in my testing though. I expect user error, is there an obvious explanation I'm missing? Thanks, Ian
