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

[Andrey Petushkov]

Hi guys,

On 12.02.2020 18:36, Aleksey Shipilev 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)) {
>  ...
Just please be aware that it's legit to have jfr_event_handler_proxy ==
NULL for a while during VM startup, including
while loading a few bootstrap classes. So if you put this assert
unconditionally you'll crash with it unconditionally :)
(see systemDictionary.cpp lines 1910 and 1912)

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