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

[Mario Torre]

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?

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