RFR (XL) 8173715: Remove FlatProfiler

[David Holmes]

On 18/08/2017 5:05 AM, Gerard Ziemski wrote:
> 
>> On Aug 17, 2017, at 9:34 AM, Gerard Ziemski <gerard.ziemski at oracle.com> wrote:
>>
>> hi David,
>>
>> Thank you for the review!
>>
>>
>>> On Aug 16, 2017, at 8:09 PM, David Holmes <david.holmes at oracle.com> wrote:
>>>
>>> Hi Gerard,
>>>
>>> On 17/08/2017 3:48 AM, Gerard Ziemski wrote:
>>>> hi all,
>>>> (this is hotspot part of the change, the jdk part is being reviewed on jdk10-dev at openjdk.java.net)
>>>> The FlatProfiler (i.e. -Xprof) has been deprecated for a while, and now it’s the time to remove it.
>>>
>>> That is fine for removing the support, but shouldn't the actual -Xprof flab be obsoleted before being actually removed? Though this is -X not -XX so I'm not sure what the launcher protocol is for this. ??
>>
>> So normally for -XX flags it’s deprecated -> obsolete -> remove, correct?
>>
>> I’m not sure about -X flag either, but it was explained to me that making -Xprof flag work in jdk10 would take considerable amount of effort, so instead of supporting already deprecated flag, we simply remove it - it was officially deprecated in jdk9 using JDK-8176098.
>>
>>
>>>> The changes are XL, but straightforward.
>>>> If anyone knows of any other remnants that should still be removed, please let me know.
>>>> issue:  https://bugs.openjdk.java.net/browse/JDK-8173715
>>>> webrev: http://cr.openjdk.java.net/~gziemski/8173715_rev1_hotspot
>>>
>>> There are a few places where you have removed reference to the flat profiler, or -Xprof, but you haven't actually changed the code that only seemed to have existed to support the flat-profiler e.g.:
>>>
>>> src/cpu/sparc/vm/macroAssembler_sparc.cpp
>>>
>>> -    save_frame_and_mov(0, Lmethod, Lmethod);   // to avoid clobbering O0 (and propagate Lmethod for -Xprof)
>>> +    save_frame_and_mov(0, Lmethod, Lmethod);   // to avoid clobbering O0 (and propagate Lmethod)
>>>
>>> Are we still propagating Lmethod here? If so why? Ditto for other places in the file.
>>
>> I couldn’t figure out whether we still needed to propagate the Lmethod, and assumed that it was safe to leave it in - the extra code is saving a register, which seems harmless to me and there are other places in the file where we do it.
>>
>>
>>> In: src/share/vm/runtime/thread.cpp
>>>
>>> -    // eventually timeout, but that takes time. Making this wait a
>>> -    // suspend-equivalent condition solves that timeout problem.
>>> -    //
>>>     Terminator_lock->wait(!Mutex::_no_safepoint_check_flag, 0,
>>>                           Mutex::_as_suspend_equivalent_flag);
>>>
>>> So we should no longer need the Mutex::_as_suspend_equivalent_flag here (and in another place).
>>
>> I wasn’t completely sure about this, and was going back and forth on this, but I figured it was safer to leave it in, since there are code paths that still use suspend/resume mechanism, i.e. the deprecated java.lang.Thread.suspend()/resume() APIs.
>>
>> Are we sure it’s OK to remove these locks?
> 
> 
> I verified that j.l.T.suspend()/resume() mechanism doesn’t use the signal based SR APIs, but instead uses JavaThread::java_suspend/java_resume, and since the comment specifically refers to FlatProfiler, I’m going to go ahead and remove these lock->wait() calls.

Hold on! That's not what I meant. It is only the use of the 
Mutex::_as_suspend_equivalent_flag that was needed due to interactions 
with flat-profiler! We still have to do the rest of the logic.

David
-----

> 
>>
>>>
>>> ---
>>>
>>> I'm a little perplexed by the changes to the os_*.cpp file regarding low-level suspend/resume:
>>>
>>> -//  within hotspot. Now there is a single use-case for this:
>>> -//    - calling get_thread_pc() on the VMThread by the flat-profiler task
>>> -//      that runs in the watcher thread.
>>> +//  within hotspot. Needed for fetch_frame_from_ucontext(), which is used by:
>>> +//    - Forte Analyzer: AsyncGetCallTrace()
>>> +//    - StackBanging: get_frame_at_stack_banging_point()
>>>
>>> I don't see any use of low-level suspend/resume with the two cases you added ??
>>
>> Those cases depend on “ucontext” being set up using the resume_clear_context()/suspend_save_context() APIs, which are part of the resume/suspend mechanism - I will try to re-do the comment.
> 
> After looking into it further I propose the following comment here:
> 
> ////////////////////////////////////////////////////////////////////////////////
> // suspend/resume support
> 
> //  The low-level signal-based suspend/resume support is a remnant from the
> //  old VM-suspension that used to be for java-suspension, safepoints etc,
> //  within hotspot. Currently used by JFR's OSThreadSampler
> //
> //  The remaining code is greatly simplified from the more general suspension
> //  code that used to be used.
> //
> //  The protocol is quite simple:
> //  - suspend:
> //      - sends a signal to the target thread
> //      - polls the suspend state of the osthread using a yield loop
> //      - target thread signal handler (SR_handler) sets suspend state
> //        and blocks in sigsuspend until continued
> //  - resume:
> //      - sets target osthread state to continue
> //      - sends signal to end the sigsuspend loop in the SR_handler
> //
> //  Note that the SR_lock plays no role in this suspend/resume protocol,
> //  but is checked for NULL in SR_handler as a thread termination indicator.
> //  The SR_lock is, however, used by JavaThread::java_suspend()/java_resume() APIs.
> //
> //  Note that resume_clear_context() and suspend_save_context() are needed
> //  by SR_handler(), so that fetch_frame_from_ucontext() works,
> //  which in part is used by:
> //    - Forte Analyzer: AsyncGetCallTrace()
> //    - StackBanging: get_frame_at_stack_banging_point()
> 
> 
> cheers
> 
>