Question re -XX:-FlightRecorder

[Andrey Petushkov]

Hi Erik,

thank you very much!

On 08.10.2020 17:24, Erik Gahlin wrote:
>
>> On 7 Oct 2020, at 10:38, Andrey Petushkov <andrey.petushkov at gmail.com> wrote:
>>
>> Hi Erik,
>>
>> On 06.10.2020 18:32, Erik Gahlin wrote:
>>> Hi Andrey,
>>>
>>> The flag is legacy from JDK 7 when we needed -XX:+FlightRecorder to allocate global buffers when the VM started. That restriction was removed in 8u40. it is probably a good idea to remove the flag completely. The method JfrRecorder::is_disabled() should be removed at the same time.
>> The flag is marked as deprecated so can I just remove it or should it be
>> promoted to the next stage (obsolete)?
> Make it obsolete.
could you please suggest if this does require a CSR process?
>
>>> When it comes to earlier releases, I think we need to stick to the behaviour that exists. Changing it in an update release would cause even more confusion. 
>> Well, I assume you don't mean VM crash on assert is the behavior you'd
>> like to retain ;)
> When removing JfrRecorder::is_disabled(), also delete the assert.
>
>> So I read this as:
>> - FlightRecorder.isAvailable to return false
>> - FlightRecorder.getFlightRecorder, new Recording() etc are to work as
>> if no -XX:-FlightRecorder was specified
> If this was the behaviour in earlier releases, I think we should keep it. It is inconsistent, but changing either of these, could lead to regressions when somebody gets a security upgrade. If there was a good use case for -XX:-FlightRecorder, one could argue for a change, but then we shouldn’t remove the flag in the first place.
Yes it is. I got you, will just remove the assert for previous releases.

Process-wise assuming no CSR is required I'll submit a RFR against
current version of jdk which
1. obsoletes the flag
2. and removes JfrRecorder::is_disabled() assert

The backport it into earlier version as only the second part.

Sorry for being so verbose, I'm new to jdk/jdk processes

Regards,
Andrey
>
> Thanks
> Erik
>
>>> Do you know why users want to use -XX:-FlightRecorder?
>> Not really. It might be that this were mostly related to JFR backport
>> into 8u and the customers were cautious about it's performance impact
>>> Thanks
>>> Erik
>> Thank you,
>> Andrey
>>>> On 6 Oct 2020, at 11:44, Andrey Petushkov <andrey.petushkov at gmail.com> wrote:
>>>>
>>>> Hi Erik, All,
>>>>
>>>> sometimes ([1], [2]) the message is given that the command line switch
>>>> -XX:-FlightRecorder disables the JFR functionality. The problem is
>>>> that this statement is not true and in fact causes buggy behavior of
>>>> the VM. Namely:
>>>> - on a release VM ran with -XX:-FlightRecorder the call to
>>>> FlightRecorder.getFlightRecorder() succeeds and the recording can be
>>>> successfully created (much like as if no command line flag was given)
>>>> - on a debug version of VM ran with same flag the call to
>>>> FlightRecorder.getFlightRecorder() crashes the VM on assert
>>>> #  Internal Error
>>>> (/home/andrey/ws/jdk/src/hotspot/share/jfr/recorder/jfrRecorder.cpp:236),
>>>> pid=331136, tid=331137
>>>> #  assert(!is_disabled()) failed: invariant
>>>>
>>>> It would be good to clarify the behavior and implement it in the
>>>> consistent way, that is, either:
>>>> - allow -XX:-FlightRecorder to actually disable JFR forever, and throw
>>>> IllegalStateException from FlightRecorder.getFlightRecorder() and
>>>> friends
>>>> - declare -XX:-FlightRecorder as no-op and remove the mentioned assert
>>>> Please note that the answer might be different for pre-13 and 13+
>>>> versions because of flag deprecation
>>>>
>>>> Any opinion on this?
>>>> This might not look like an important thing at all however there were
>>>> quite a number of users (Azul customers) stumbling on unclear behavior
>>>> of the said flag so I thought it's better to have this fixed
>>>>
>>>> Thanks a lot,
>>>> Andrey
>>>>
>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8225312
>>>> [2] jdk/src/hotspot/share/jfr/recorder/jfrRecorder.cpp,
>>>> JfrRecorder::is_disabled()