RFR (XL) 8173715: Remove FlatProfiler

[Gerard Ziemski]

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


> 
> ---
> 
> src/os_cpu/linux_s390/vm/thread_linux_s390.hpp	
> 
> /*
> - * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
> 
> That's an incorrect copyright update - should be "2016, 2017,”

Thank you for catching this.


> 
> ---
> 
> src/os/solaris/vm/osThread_solaris.hpp
> 
>  // ***************************************************************
> - // interrupt support.  interrupts (using signals) are used to get
> - // the thread context (get_thread_pc), to set the thread context
> - // (set_thread_pc), and to implement java.lang.Thread.interrupt.
> + // interrupt support.  interrupts (using signals) are used to
> + // implement java.lang.Thread.interrupt.
>  // ***************************************************************
> 
> That entire comment block can be deleted. We haven't used signals for j.l.T.interrupt for years.

Thank you for catching this.


cheers