[RFR]: [S390] Implement JFR profiling

[Doerr, Martin]

Hi Gunter,

I suggest to call the function "ijava_state_unchecked".

In addition to Volker's change requests, the indentation in thread_linux_s390.cpp looks odd (regarding one of the else cases).

Thanks,
Martin


-----Original Message-----
From: hotspot-dev <hotspot-dev-bounces at openjdk.java.net> On Behalf Of Haug, Gunter
Sent: Montag, 8. Oktober 2018 15:45
To: Volker Simonis <volker.simonis at gmail.com>
Cc: HotSpot Open Source Developers <hotspot-dev at openjdk.java.net>
Subject: Re: [RFR]: [S390] Implement JFR profiling

Thanks a lot, Volker!

>    I think the last two lines after "return false" are a leftover from
>    the old version and should be removed. Strange you didn't got a
>    compiler warning because of unreachable code.

You are completely right! I'll remove it.

>    In the comment you mention that we can only check completeness for
>    runtime stubs and nmethods but later in the code you only check for
>    the stubs case and not for nmethods. Which is right, the code or the
>    comment?

The comment is a leftover from PPC, as well. There is no frame complete in compiled methods on s390. I'll remove the comment.

>    I think from a philosophical point of view it is unfortunate, that the
>    ijava_state_unsafe() is "public" while ijava_state() is "private", but
>    I don't mind if you don't change this any more :)

I see what you mean. S390 is the only platform where there is a magic number in the i_state in debug builds and this is asserted ijava_state(). I'm with you that the naming is a little unfortunate but I simply head no better idea. If you have one, let me know, I would be happy to incorporate it!

Thanks again,
Gunter

On 08.10.18, 15:23, "Volker Simonis" <volker.simonis at gmail.com> wrote:

    Hi Gunter,
    
    thanks for fixing this issue on s390. In general your change looks
    good. Just some minor comments:
    
    thread_linux_s390.cpp
    =================
    
    +  // nothing else to try
    +  return false;
    +
    +  ucontext_t* uc = (ucontext_t*) ucontext;
       return true;
     }
    
    I think the last two lines after "return false" are a leftover from
    the old version and should be removed. Strange you didn't got a
    compiler warning because of unreachable code.
    
    frame_s390.cpp
    ============
    
    +    // 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
    +    if (!_cb->is_frame_complete_at(_pc)) {
    +      if (_cb->is_adapter_blob() || _cb->is_runtime_stub()) {
    
    In the comment you mention that we can only check completeness for
    runtime stubs and nmethods but later in the code you only check for
    the stubs case and not for nmethods. Which is right, the code or the
    comment?
    
    frame_s390.hpp
    ============
    I think from a philosophical point of view it is unfortunate, that the
    ijava_state_unsafe() is "public" while ijava_state() is "private", but
    I don't mind if you don't change this any more :)
    
    Regards,
    Volker
    On Mon, Oct 8, 2018 at 2:48 PM Haug, Gunter <gunter.haug at sap.com> wrote:
    >
    > Hi all,
    >
    > can I please have reviews and a sponsor fort the following change:
    >
    > https://bugs.openjdk.java.net/browse/JDK-8211768
    > http://cr.openjdk.java.net/~ghaug/webrevs/8211768/
    >
    > The implementation is basically the same as for PPC. It was only adapted for the different use of registers and the slightly different frame layout of s390. The respective tests don't fail anymore on s390 with this patch.
    >
    > Thanks,
    > Gunter
    >
    >
    >
    >