RFR (XL) 8173715: Remove FlatProfiler

[Gerard Ziemski]

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


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