PPC64: jfr profiling doesn't work (PPC64 only)
Volker Simonis
volker.simonis at gmail.com
Tue Jul 24 09:06:44 UTC 2018
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