PPC64: jfr profiling doesn't work (PPC64 only)

[Volker Simonis]

Hi Gunter,

thanks for fixing this! The change looks god in general. Please find
some comments questions below:


src/hotspot/cpu/ppc/frame_ppc.cpp
==========================

  78    // an fp must be within the stack and above (but not equal) sp
  79   bool fp_safe = (fp <= thread->stack_base()) &&  (fp > sp) &&
((fp - sp) >= (ijava_state_size + top_ijava_frame_abi_size));

Is this check for interpreter frames only? Then better name it
'fp_interp_safe' and adapt the comment. Otherwise, why does the 'fp -
sp' have to be larger than the java interpreter state?
Also the line is quite long. Better break it after '&&'

  81   // We know sp/unextended_sp are safe only fp is questionable here

Better put a comma (or period) after 'safe' to make it more readable.

  88     // First check if frame is complete and tester is reliable
  89     // Unfortunately we can only check frame complete for runtime
stubs and nmethod
  90     // other generic buffer blobs are more problematic so we just
assume they are
  91     // ok. adapter blobs never have a frame complete and are never ok.

Better: "First check if the frame is complete and the test is
reliable. Unfortunately we can only check frame completeness for
runtime stubs and nmethods. Other generic buffer blobs are more
problematic so we just assume they are OK. Adapter blobs never have a
complete frame and are never OK."

In general please start comments with an uppercase letter and use a
period at the end of sentences.

  98     // Could just be some random pointer within the codeBlob
  99     if (!_cb->code_contains(_pc)) {

Shouldn't this be the first, basic check after we know that '_cb !=
NULL'  (i.e. even before we check for frame completeness)?

 103     // Entry frame checks
 104     if (is_entry_frame()) {
 105       // an entry frame must have a valid fp.
 106       return fp_safe && is_entry_frame_valid(thread);
 107     }

An entry frame is not an interpreter frame but you use 'fp_safe' as
computed for interpreter frames which is probably too conservative.
Maybe the check in 'is_entry_frame_valid()' is sufficient already?

 118     CodeBlob* sender_blob = CodeCache::find_blob_unsafe(sender_pc);
 119     if (sender_pc == NULL ||  sender_blob == NULL) {
 120       return false;
 121     }

'find_blob_unsafe()' returns NULL if the 'sender_pc' is NULL so
there's no need for the extra 'sender_pc == NULL' check in the
if-clause.

 135     // an fp must be within the stack and above (but not equal)
current frame's _FP
 136
 137     bool sender_fp_safe = (sender_fp <= thread->stack_base()) &&
(sender_fp > fp);
 138
 139     if (!sender_fp_safe) {
 140       return false;
 141     }

Shorter:

 135     // sender_fp must be within the stack and above (but not
equal) current frame's fp
 137     if (sender_fp > thread->stack_base() || sender_fp <= fp) {
 140       return false;
 141     }

 158     if (sender.is_entry_frame()) {
 159       // Validate the JavaCallWrapper an entry frame must have
 160
 161       address jcw = (address)sender.entry_frame_call_wrapper();
 162
 163       bool jcw_safe = (jcw <= thread->stack_base()) && (jcw > sender_fp);
 164       return jcw_safe;
 165     }

Why don't you use 'sender.is_entry_frame_valid()' valid instead of
duplicating that code here?

 173     // Could put some more validation for the potential
non-interpreted sender
 174     // frame we'd create by calling sender if I could think of
any. Wait for next crash in forte...
 175
 176     // One idea is seeing if the sender_pc we have is one that
we'd expect to call to current cb

I think these comments are leftovers from other architectures which
can be removed (we don't support 'forte' on ppc :)

 184   // Must be native-compiled frame. Since sender will try and use
fp to find
 185   // linkages it must be safe
 186
 187   if (!fp_safe) return false;

If it's a native compiled frame the 'fp_safe' check is too strict
because it was computed for interpreter frames.

 189   // could try and do some more potential verification of native
frame if we could think of some...

Useless comment - can be removed.

src/hotspot/os_cpu/linux_ppc/thread_linux_ppc.cpp
=====================================

  45   assert(this->is_Java_thread(), "must be JavaThread");
  46   JavaThread* jt = (JavaThread *)this;

'this' is already a 'JavaThread' so no need for the assertion and the
new local variable 'jt'.

  81            if (ProfileInterpreter) {
  82           }

Unused - can be deleted.

Regards,
Volker


On Thu, Jul 19, 2018 at 12:53 PM, Haug, Gunter <gunter.haug at sap.com> wrote:
> Hi all,
>
> can I please have reviews and a sponsor for the following fix:
>
> https://bugs.openjdk.java.net/projects/JDK/issues/JDK-8207392?filter=allopenissues
> http://cr.openjdk.java.net/~ghaug/webrevs/8207392/
>
> JFR profiling on linux PPC64 has not been implemented correctly so far, the VM crashes when it is turned on. Therefore hotspot/jtreg/runtime/appcds/TestWithProfiler.java fails. With this fix the test succeeds. I've analyzed a couple of benchmarks with JMC and results look plausible when compared to linux x86.
>
> Thanks and best regards,
> Gunter
>
>
>
>
>