PPC64: jfr profiling doesn't work (PPC64 only)
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Tue Jul 24 13:42:40 UTC 2018
Hi Gunter,
I'll sponsor this for you.
I'll change the bug to [ppc64] Implement JFR profiling.
I think this better expresses what's going on.
The bug still states that this fixes crashes, so this
still is a bugfix acceptable for RDP1.
Best regards,
Goetz.
> -----Original Message-----
> From: Volker Simonis <volker.simonis at gmail.com>
> Sent: Dienstag, 24. Juli 2018 11:07
> To: Haug, Gunter <gunter.haug at sap.com>
> Cc: hotspot-runtime-dev at openjdk.java.net; Lindenmaier, Goetz
> <goetz.lindenmaier at sap.com>
> Subject: Re: PPC64: jfr profiling doesn't work (PPC64 only)
>
> Hi Gunter,
>
> looks pretty good now :)
>
> Just some minor issues (no need for a new webrev):
>
> - Indentation is wrong (do you have tabs in your file?). I think
> jcheck should normally detect that, but I'm not sure.
>
> 96 // Now check if the frame is complete and the test is
> 97 // reliable. Unfortunately we can only check frame completeness for
> 98 // runtime stubs and nmethods. Other generic buffer blobs are more
> 99 // problematic so we just assume they are OK. Adapter
> blobs never have a
> 100 // complete frame and are never OK
> 101 if (!_cb->is_frame_complete_at(_pc)) {
>
> - Also the following comment is still weird - maybe you can reword it
> to something more understandable ?
>
> 163 // If the frame size is 0 something (or less) is bad because
> every nmethod has a non-zero frame size
> 164 // because you must allocate window space.
>
> And finally I just realized that in thread_linux_ppc.cpp the method
> 'JavaThread::pd_get_top_frame_for_signal_handler()' is still
> unimplemented. Looking at the signature (and the other platforms) I
> think it can be simply implemented by forwarding the call to
> 'pd_get_top_frame_for_profiling()'. With that simple change we should
> get support for the async profile [1] on ppc which I think would be
> cool.
>
> Regards,
> Volker
>
> [1] https://github.com/jvm-profiling-tools/async-profiler
>
>
>
> On Mon, Jul 23, 2018 at 4:27 PM, Haug, Gunter <gunter.haug at sap.com>
> wrote:
> > 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
> > >
> > >
> > >
> > >
> > >
> >
> >
More information about the hotspot-runtime-dev
mailing list