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

Mon Jul 23 14:27:28 UTC 2018

Thanks a lot, Volker for the review!

Here is an updated webrev:

http://cr.openjdk.java.net/~ghaug/webrevs/8207392.v1

I have incorporated all the suggestions you made. Moreover, Goetz' improvements are in as well.

I'll ask Goetz to sponsor it tomorrow if nobody else objects.

Best regards,
Gunter

On 20.07.18, 14:27, "Volker Simonis" <volker.simonis at gmail.com> wrote:

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