RFR (XL) 8173715: Remove FlatProfiler

[coleen.phillimore at oracle.com]

On 8/18/17 4:51 PM, Gerard Ziemski wrote:
> Thank you David for clarifications.
>
>> On Aug 17, 2017, at 4:38 PM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> Hi Gerard,
>>
>> On 18/08/2017 12:34 AM, Gerard Ziemski 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?
>> Yes.
>>
>>> 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.
>> Obsoleting the flag still allows removal of all the functionality, it just means use of the flag itself doesn't produce an error, but a warning telling you the flag is obsolete and has no effect and will be removed in the future.
> I have obsoleted the flag, so it’s still accepted, but it will now print a warning “Ignoring option -Xprof; support was removed in 10.0"
>
>
>>>>> 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.
>> This needs to be assessed by compiler folk. If we leave in code that was only needed for flat-profiler but remove all such comments then no-one will know why we are doing something. If this code is needed then the comment should reflect why; if not needed it should be removed.
> I will ask on the compiler mailing list about it.

If this is a small piece of a large change, can we file an RFE to 
evaluate this and clean it up later?
>
>
>>>> 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?
>> As per other email it is only the use of the flag that could be removed, if it truly only was needed due to flat-profiler interactions. But looking at the full block of code in context it seems that the need to be "suspend equivalent" was already part of this logic and that that was sufficient to fix the flat-profiler issue. So it looks like the code should remain as-is.
> Left the code alone.
>
>
>>>> ---
>>>>
>>>> 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.
>> That comment pertained to actual use of the low-level suspend/resume mechanism which is not used by AGCT or stack-banging. I would just terminate the comment at "within hotspot" and not add any additional text.
> I would really like to keep my 2nd attempt at the comment in - it identifies who is still using the suspend/resume mechanism, with a note about user context, in case someone is tempted to remove it (like I originally did before realizing it’s still needed)
>
> Rev 2 webrev coming up shortly…
>

Looking forward to it.

Thanks,
Coleen

> cheers
>