[JFR: jdk8u-jfr-incubator] RFR: JDK-8238589: Necessary code cleanup in JFR for JDK8u

[Andrey Petushkov]

Hi Mario,

On 12.02.2020 18:59, Mario Torre wrote:
> On Wed, Feb 12, 2020 at 4:37 PM Aleksey Shipilev <shade at redhat.com> wrote:
>> On 2/12/20 1:42 PM, Mario Torre wrote:
>>>> *) In systemDictionary.cpp, this assert seems legit, no need to remove it? It guards from calling
>>>> the branch when jfr_event_handler_proxy is accidentally uninitialized:
>>>>
>>>>  1342       assert(jfr_event_handler_proxy != NULL, "invariant");
>>> But if jfr_event_handler_proxy is null then also class_name needs to
>>> be null for the code to be executed, I don't think this is possible?
>> I don't want these two things to interact. So this is why you need the assert before the branch to
>> verify that we are testing against the sane value:
>>
>>  #if INCLUDE_JFR
>>      assert(jfr_event_handler_proxy != NULL, "invariant");
>>      if (k.is_null() && (class_name == jfr_event_handler_proxy)) {
>>  ...
> I'm a bit afraid that now the assert will be executed no matter what.
> Before it would fall inside an "else" so only execute if k.is_null.
>
> Am I seeing this wrong?
No, you're correct. It should be under k.is_null, otherwise additional
measures are needed
to protect from not executing it during VM init

Andrey
>
> Cheers,
> Mario
>
>>> http://cr.openjdk.java.net/~neugens/8238589/webrev.02/
>> Otherwise looks fine.
>>
>> Stylistically, I don't think you need the comment at #endif of short blocks like:
>>
>>  259 #if INCLUDE_JFR
>>  260   // If requested, return information on which thread held the bias
>>  261   if (biased_locker != NULL) {
>>  262     *biased_locker = biased_thread;
>>  263   }
>>  264 #endif // INCLUDE_JFR
>>
>> ...as it is dead obvious what that #endif belongs to.
>>
>> --
>> Thanks,
>> -Aleksey
>>
>
> --
> Mario Torre
> Associate Manager, Software Engineering
> Red Hat GmbH <https://www.redhat.com>
> 9704 A60C B4BE A8B8 0F30  9205 5D7E 4952 3F65 7898