RFR (XL) 8173715: Remove FlatProfiler
gerard.ziemski at oracle.com
Thu Aug 17 19:05:28 UTC 2017
> 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.:
>> - 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,
>> 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()
More information about the hotspot-dev