Re: [EXTERNAL] Re: [PATCH v4] perf inject --jit: Remove //anon mmap events

[Ian Rogers <irogers@google.com>]

On Fri, Jun 12, 2020 at 12:00 PM Steve MacLean
<Steve.MacLean@microsoft.com> wrote:
>
> >>> Hi Ian,
> >>>
> >>>> I tried this as well with latest perf/core. The difference is that
> >>> unresolved addresses currently look like:
> >>>
> >>>      0.00%  java             [JIT] tid 221782       [.] 0x0000ffff451499a4
> >>>      0.00%  java             [JIT] tid 221782       [.] 0x0000ffff4514f3e8
> >>>      0.00%  java             [JIT] tid 221782       [.] 0x0000ffff45149394
> >>>
> >>> But after Steve's patch this becomes:
> >>>
> >>>      0.00%  java             [unknown]              [.] 0x0000ffff58557d14
> >>>      0.00%  java             [unknown]              [.] 0x0000ffff785c03b4
> >>>      0.00%  java             [unknown]              [.] 0x0000ffff58386520
> >>>
> >>> I couldn't see any events that were symbolised before but are no
> >>> longer symbolised after this patch.
> >>
> >> I see this, thanks for digging into the explanation! Were you able to
> >> get a test case where the unknowns went down? For example, by forcing
> >> the code cache size to be small? This is the result I'd expect to see.
> >
> >I tried the same Dacapo benchmark as you with different values of InitialCodeCacheSize and grepped for -e '\[unknown\]' -e '\[JIT\]'.
> >
> >       Base   Patched
> > 100M  338    373
> > 50M   333    315
> > 25M   323    368
> > 15M   1238   309
> > 10M   2600   333
> > 1M    6035   337
> >
> >This looks fairly convincing to me: the cliff at 15M is where the code cache starts needing to be enlarged.
> >
>
> Removing the anonymous mappings causes a small regression.  Specifically,
> the reporting of the module name goes from "[JIT] tid <tid>" to "[unknown]".
> This occurs when the JIT fails to report memory used in jitdump before it
> is used.
>
> However there is also confirmation that JAVA does see the reported issue
> when using a small code cache.  The current patch resolves the issue in
> this case.
>
> I see two options:
>
> + Accept the regression. Since this is a regression for a jit dump
> reporting synchronization error, this may be a reasonable option.
>
> + Design a more complicated patch. Either
>     + Only strip parts of // anon mmap events overlapping existing
>       jitted-<pid>-<code_index>.so mmap events.
>     + Only strip parts of // anon mmap events overlapping prior
>       // anon mmap events
>
> Any opinions?

Hi Steve,

I think we should merge this change. I wanted to get a test case
together showing the benefit. Based on Nick Gasson's feedback I was
trying with command lines like:
/tmp/perf/perf record -k 1 -e cycles:u -o /tmp/perf.data
/usr/lib/jvm/java-14-openjdk-amd64/bin/java
-agentpath:/tmp/perf/libperf-jvmti.so -XX:+PreserveFramePointer
-XX:InitialCodeCacheSize=10M -jar dacapo-9.12-bach.jar jython

I wanted to be able to demonstrate a non-zero effect in getting
samples. I spent limited time on the test and didn't get a result that
demonstrated a benefit, things weren't worse. I think the lack of
benefit is another bug where HotSpot's JVMTI code runs in parallel to
the new code being available in the JIT cache. We get samples ahead of
when the jitdump thinks the code is there and so it symbolizes them as
unknown. We should get HotSpot to provide the earliest timestamp to
avoid this problem.

I think a good way forward would be to merge this code as other issues
can be fixed as a follow up. Arnaldo, does that work for you? If so,
Acked-by: Ian Rogers <irogers@google.com>.

Thanks,
Ian