FW: JDK 10 RFR JDK-8167425: Redundant code in method PerfLongVariant::sample

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

FW: JDK 10 RFR JDK-8167425: Redundant code in method PerfLongVariant::sample

Shafi Ahmad
Hi,

Summary:
It's a very small change to a single file.

void PerfLongVariant::sample() {
 
-  assert(_sample_helper != NULL || _sampled != NULL, "unexpected state");
+  // JJJ - This should not happen.  Maybe the first sample is taken  //  
+ while the _sample_helper is being null'ed out.
+  // assert(_sample_helper != NULL || _sampled != NULL, "unexpected  state");
+ if (_sample_helper == NULL) return;
 
  if (_sample_helper != NULL) {
    *(jlong*)_valuep = _sample_helper->take_sample();
  }
  else if (_sampled != NULL) {
    *(jlong*)_valuep = *_sampled;
  }
}
Above five lines are modified in changeset http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/da91efe96a93#l802.12
Due to the addition of NULL check and return
  if (_sample_helper == NULL) return;
the else-if block becomes redundant and statement 'if (_sample_helper != NULL)' is not needed.

Webrev link: http://cr.openjdk.java.net/~shshahma/8167425/webrev.00/
Jdk10 bug link: https://bugs.openjdk.java.net/browse/JDK-8167425

Testing: jprt.

Thanks,
Shafi
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: JDK 10 RFR JDK-8167425: Redundant code in method PerfLongVariant::sample

Shafi Ahmad
Hi,

May I get the review done for this.

Regards,
Shafi

> -----Original Message-----
> From: Shafi Ahmad
> Sent: Wednesday, March 01, 2017 4:27 PM
> To: [hidden email]
> Subject: FW: JDK 10 RFR JDK-8167425: Redundant code in method
> PerfLongVariant::sample
>
> Hi,
>
> Summary:
> It's a very small change to a single file.
>
> void PerfLongVariant::sample() {
>
> -  assert(_sample_helper != NULL || _sampled != NULL, "unexpected
> state");
> +  // JJJ - This should not happen.  Maybe the first sample is taken  //
> + while the _sample_helper is being null'ed out.
> +  // assert(_sample_helper != NULL || _sampled != NULL, "unexpected
> + state"); if (_sample_helper == NULL) return;
>
>   if (_sample_helper != NULL) {
>     *(jlong*)_valuep = _sample_helper->take_sample();
>   }
>   else if (_sampled != NULL) {
>     *(jlong*)_valuep = *_sampled;
>   }
> }
> Above five lines are modified in changeset
> http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/da91efe96a93#l802.12
> Due to the addition of NULL check and return
>   if (_sample_helper == NULL) return;
> the else-if block becomes redundant and statement 'if (_sample_helper !=
> NULL)' is not needed.
>
> Webrev link: http://cr.openjdk.java.net/~shshahma/8167425/webrev.00/
> Jdk10 bug link: https://bugs.openjdk.java.net/browse/JDK-8167425
>
> Testing: jprt.
>
> Thanks,
> Shafi
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 10 RFR JDK-8167425: Redundant code in method PerfLongVariant::sample

coleen.phillimore

This doesn't look right.

Don't you need to still check for _sampled != NULL?

Can you remove the JJJ in the comment?  I don't know what tests were run
where Jon (aka JJJ in the code) found this, do you?

Thanks,
Coleen


On 3/8/17 10:21 AM, Shafi Ahmad wrote:

> Hi,
>
> May I get the review done for this.
>
> Regards,
> Shafi
>
>> -----Original Message-----
>> From: Shafi Ahmad
>> Sent: Wednesday, March 01, 2017 4:27 PM
>> To: [hidden email]
>> Subject: FW: JDK 10 RFR JDK-8167425: Redundant code in method
>> PerfLongVariant::sample
>>
>> Hi,
>>
>> Summary:
>> It's a very small change to a single file.
>>
>> void PerfLongVariant::sample() {
>>
>> -  assert(_sample_helper != NULL || _sampled != NULL, "unexpected
>> state");
>> +  // JJJ - This should not happen.  Maybe the first sample is taken  //
>> + while the _sample_helper is being null'ed out.
>> +  // assert(_sample_helper != NULL || _sampled != NULL, "unexpected
>> + state"); if (_sample_helper == NULL) return;
>>
>>    if (_sample_helper != NULL) {
>>      *(jlong*)_valuep = _sample_helper->take_sample();
>>    }
>>    else if (_sampled != NULL) {
>>      *(jlong*)_valuep = *_sampled;
>>    }
>> }
>> Above five lines are modified in changeset
>> http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/da91efe96a93#l802.12
>> Due to the addition of NULL check and return
>>    if (_sample_helper == NULL) return;
>> the else-if block becomes redundant and statement 'if (_sample_helper !=
>> NULL)' is not needed.
>>
>> Webrev link: http://cr.openjdk.java.net/~shshahma/8167425/webrev.00/
>> Jdk10 bug link: https://bugs.openjdk.java.net/browse/JDK-8167425
>>
>> Testing: jprt.
>>
>> Thanks,
>> Shafi

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 10 RFR JDK-8167425: Redundant code in method PerfLongVariant::sample

David Holmes
Hi Coleen,

On 9/03/2017 1:42 AM, [hidden email] wrote:
>
> This doesn't look right.
>
> Don't you need to still check for _sampled != NULL?

That use of _sampled was in dead code that is now removed.

     if (_sample_helper == NULL) return;
-   if (_sample_helper != NULL) {
       *(jlong*)_valuep = _sample_helper->take_sample();
-   }
-   else if (_sampled != NULL) {
-     *(jlong*)_valuep = *_sampled;
-   }
   }

This change looks fine to me.

> Can you remove the JJJ in the comment?

I second that :)

Thanks,
David
-----

>  I don't know what tests were run
> where Jon (aka JJJ in the code) found this, do you?
>
> Thanks,
> Coleen
>
>
> On 3/8/17 10:21 AM, Shafi Ahmad wrote:
>> Hi,
>>
>> May I get the review done for this.
>>
>> Regards,
>> Shafi
>>
>>> -----Original Message-----
>>> From: Shafi Ahmad
>>> Sent: Wednesday, March 01, 2017 4:27 PM
>>> To: [hidden email]
>>> Subject: FW: JDK 10 RFR JDK-8167425: Redundant code in method
>>> PerfLongVariant::sample
>>>
>>> Hi,
>>>
>>> Summary:
>>> It's a very small change to a single file.
>>>
>>> void PerfLongVariant::sample() {
>>>
>>> -  assert(_sample_helper != NULL || _sampled != NULL, "unexpected
>>> state");
>>> +  // JJJ - This should not happen.  Maybe the first sample is taken  //
>>> + while the _sample_helper is being null'ed out.
>>> +  // assert(_sample_helper != NULL || _sampled != NULL, "unexpected
>>> + state"); if (_sample_helper == NULL) return;
>>>
>>>    if (_sample_helper != NULL) {
>>>      *(jlong*)_valuep = _sample_helper->take_sample();
>>>    }
>>>    else if (_sampled != NULL) {
>>>      *(jlong*)_valuep = *_sampled;
>>>    }
>>> }
>>> Above five lines are modified in changeset
>>> http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/da91efe96a93#l802.12
>>> Due to the addition of NULL check and return
>>>    if (_sample_helper == NULL) return;
>>> the else-if block becomes redundant and statement 'if (_sample_helper !=
>>> NULL)' is not needed.
>>>
>>> Webrev link: http://cr.openjdk.java.net/~shshahma/8167425/webrev.00/
>>> Jdk10 bug link: https://bugs.openjdk.java.net/browse/JDK-8167425
>>>
>>> Testing: jprt.
>>>
>>> Thanks,
>>> Shafi
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: JDK 10 RFR JDK-8167425: Redundant code in method PerfLongVariant::sample

Shafi Ahmad
Hi David, Coleen,

Thank you for the review.
I will send the updated webrev link.

Regards,
Shafi

> -----Original Message-----
> From: David Holmes
> Sent: Thursday, March 09, 2017 7:35 AM
> To: Coleen Phillimore <[hidden email]>; hotspot-
> [hidden email]
> Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
> PerfLongVariant::sample
>
> Hi Coleen,
>
> On 9/03/2017 1:42 AM, [hidden email] wrote:
> >
> > This doesn't look right.
> >
> > Don't you need to still check for _sampled != NULL?
>
> That use of _sampled was in dead code that is now removed.
>
>      if (_sample_helper == NULL) return;
> -   if (_sample_helper != NULL) {
>        *(jlong*)_valuep = _sample_helper->take_sample();
> -   }
> -   else if (_sampled != NULL) {
> -     *(jlong*)_valuep = *_sampled;
> -   }
>    }
>
> This change looks fine to me.
>
> > Can you remove the JJJ in the comment?
>
> I second that :)
>
> Thanks,
> David
> -----
>
> >  I don't know what tests were run
> > where Jon (aka JJJ in the code) found this, do you?
> >
> > Thanks,
> > Coleen
> >
> >
> > On 3/8/17 10:21 AM, Shafi Ahmad wrote:
> >> Hi,
> >>
> >> May I get the review done for this.
> >>
> >> Regards,
> >> Shafi
> >>
> >>> -----Original Message-----
> >>> From: Shafi Ahmad
> >>> Sent: Wednesday, March 01, 2017 4:27 PM
> >>> To: [hidden email]
> >>> Subject: FW: JDK 10 RFR JDK-8167425: Redundant code in method
> >>> PerfLongVariant::sample
> >>>
> >>> Hi,
> >>>
> >>> Summary:
> >>> It's a very small change to a single file.
> >>>
> >>> void PerfLongVariant::sample() {
> >>>
> >>> -  assert(_sample_helper != NULL || _sampled != NULL, "unexpected
> >>> state");
> >>> +  // JJJ - This should not happen.  Maybe the first sample is taken
> >>> + // while the _sample_helper is being null'ed out.
> >>> +  // assert(_sample_helper != NULL || _sampled != NULL, "unexpected
> >>> + state"); if (_sample_helper == NULL) return;
> >>>
> >>>    if (_sample_helper != NULL) {
> >>>      *(jlong*)_valuep = _sample_helper->take_sample();
> >>>    }
> >>>    else if (_sampled != NULL) {
> >>>      *(jlong*)_valuep = *_sampled;
> >>>    }
> >>> }
> >>> Above five lines are modified in changeset
> >>>
> http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/da91efe96a93#l802.1
> >>> 2 Due to the addition of NULL check and return
> >>>    if (_sample_helper == NULL) return; the else-if block becomes
> >>> redundant and statement 'if (_sample_helper != NULL)' is not needed.
> >>>
> >>> Webrev link:
> http://cr.openjdk.java.net/~shshahma/8167425/webrev.00/
> >>> Jdk10 bug link: https://bugs.openjdk.java.net/browse/JDK-8167425
> >>>
> >>> Testing: jprt.
> >>>
> >>> Thanks,
> >>> Shafi
> >
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: JDK 10 RFR JDK-8167425: Redundant code in method PerfLongVariant::sample

Shafi Ahmad
In reply to this post by David Holmes
Hi,

Please find updated webrev link.
http://cr.openjdk.java.net/~shshahma/8167425/webrev.01/

Regards,
Shafi

> -----Original Message-----
> From: David Holmes
> Sent: Thursday, March 09, 2017 7:35 AM
> To: Coleen Phillimore <[hidden email]>; hotspot-
> [hidden email]
> Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
> PerfLongVariant::sample
>
> Hi Coleen,
>
> On 9/03/2017 1:42 AM, [hidden email] wrote:
> >
> > This doesn't look right.
> >
> > Don't you need to still check for _sampled != NULL?
>
> That use of _sampled was in dead code that is now removed.
>
>      if (_sample_helper == NULL) return;
> -   if (_sample_helper != NULL) {
>        *(jlong*)_valuep = _sample_helper->take_sample();
> -   }
> -   else if (_sampled != NULL) {
> -     *(jlong*)_valuep = *_sampled;
> -   }
>    }
>
> This change looks fine to me.
>
> > Can you remove the JJJ in the comment?
>
> I second that :)
>
> Thanks,
> David
> -----
>
> >  I don't know what tests were run
> > where Jon (aka JJJ in the code) found this, do you?
> >
> > Thanks,
> > Coleen
> >
> >
> > On 3/8/17 10:21 AM, Shafi Ahmad wrote:
> >> Hi,
> >>
> >> May I get the review done for this.
> >>
> >> Regards,
> >> Shafi
> >>
> >>> -----Original Message-----
> >>> From: Shafi Ahmad
> >>> Sent: Wednesday, March 01, 2017 4:27 PM
> >>> To: [hidden email]
> >>> Subject: FW: JDK 10 RFR JDK-8167425: Redundant code in method
> >>> PerfLongVariant::sample
> >>>
> >>> Hi,
> >>>
> >>> Summary:
> >>> It's a very small change to a single file.
> >>>
> >>> void PerfLongVariant::sample() {
> >>>
> >>> -  assert(_sample_helper != NULL || _sampled != NULL, "unexpected
> >>> state");
> >>> +  // JJJ - This should not happen.  Maybe the first sample is taken
> >>> + // while the _sample_helper is being null'ed out.
> >>> +  // assert(_sample_helper != NULL || _sampled != NULL, "unexpected
> >>> + state"); if (_sample_helper == NULL) return;
> >>>
> >>>    if (_sample_helper != NULL) {
> >>>      *(jlong*)_valuep = _sample_helper->take_sample();
> >>>    }
> >>>    else if (_sampled != NULL) {
> >>>      *(jlong*)_valuep = *_sampled;
> >>>    }
> >>> }
> >>> Above five lines are modified in changeset
> >>>
> http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/da91efe96a93#l802.1
> >>> 2 Due to the addition of NULL check and return
> >>>    if (_sample_helper == NULL) return; the else-if block becomes
> >>> redundant and statement 'if (_sample_helper != NULL)' is not needed.
> >>>
> >>> Webrev link:
> http://cr.openjdk.java.net/~shshahma/8167425/webrev.00/
> >>> Jdk10 bug link: https://bugs.openjdk.java.net/browse/JDK-8167425
> >>>
> >>> Testing: jprt.
> >>>
> >>> Thanks,
> >>> Shafi
> >
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 10 RFR JDK-8167425: Redundant code in method PerfLongVariant::sample

David Holmes
Hi Shafi,

On 9/03/2017 3:08 PM, Shafi Ahmad wrote:
> Hi,
>
> Please find updated webrev link.
> http://cr.openjdk.java.net/~shshahma/8167425/webrev.01/

The more I read this code block, the comment and the commented-out
assertion:

  215 void PerfLongVariant::sample() {
  216
  217   // This should not happen.  Maybe the first sample is taken
  218   // while the _sample_helper is being null'ed out.
  219   // assert(_sample_helper != NULL || _sampled != NULL,
"unexpected state");
  220   if (_sample_helper == NULL) return;
  221
  222   *(jlong*)_valuep = _sample_helper->take_sample();
  223 }

the less it makes sense to me.  ???

David




> Regards,
> Shafi
>
>> -----Original Message-----
>> From: David Holmes
>> Sent: Thursday, March 09, 2017 7:35 AM
>> To: Coleen Phillimore <[hidden email]>; hotspot-
>> [hidden email]
>> Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
>> PerfLongVariant::sample
>>
>> Hi Coleen,
>>
>> On 9/03/2017 1:42 AM, [hidden email] wrote:
>>>
>>> This doesn't look right.
>>>
>>> Don't you need to still check for _sampled != NULL?
>>
>> That use of _sampled was in dead code that is now removed.
>>
>>      if (_sample_helper == NULL) return;
>> -   if (_sample_helper != NULL) {
>>        *(jlong*)_valuep = _sample_helper->take_sample();
>> -   }
>> -   else if (_sampled != NULL) {
>> -     *(jlong*)_valuep = *_sampled;
>> -   }
>>    }
>>
>> This change looks fine to me.
>>
>>> Can you remove the JJJ in the comment?
>>
>> I second that :)
>>
>> Thanks,
>> David
>> -----
>>
>>>  I don't know what tests were run
>>> where Jon (aka JJJ in the code) found this, do you?
>>>
>>> Thanks,
>>> Coleen
>>>
>>>
>>> On 3/8/17 10:21 AM, Shafi Ahmad wrote:
>>>> Hi,
>>>>
>>>> May I get the review done for this.
>>>>
>>>> Regards,
>>>> Shafi
>>>>
>>>>> -----Original Message-----
>>>>> From: Shafi Ahmad
>>>>> Sent: Wednesday, March 01, 2017 4:27 PM
>>>>> To: [hidden email]
>>>>> Subject: FW: JDK 10 RFR JDK-8167425: Redundant code in method
>>>>> PerfLongVariant::sample
>>>>>
>>>>> Hi,
>>>>>
>>>>> Summary:
>>>>> It's a very small change to a single file.
>>>>>
>>>>> void PerfLongVariant::sample() {
>>>>>
>>>>> -  assert(_sample_helper != NULL || _sampled != NULL, "unexpected
>>>>> state");
>>>>> +  // JJJ - This should not happen.  Maybe the first sample is taken
>>>>> + // while the _sample_helper is being null'ed out.
>>>>> +  // assert(_sample_helper != NULL || _sampled != NULL, "unexpected
>>>>> + state"); if (_sample_helper == NULL) return;
>>>>>
>>>>>    if (_sample_helper != NULL) {
>>>>>      *(jlong*)_valuep = _sample_helper->take_sample();
>>>>>    }
>>>>>    else if (_sampled != NULL) {
>>>>>      *(jlong*)_valuep = *_sampled;
>>>>>    }
>>>>> }
>>>>> Above five lines are modified in changeset
>>>>>
>> http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/da91efe96a93#l802.1
>>>>> 2 Due to the addition of NULL check and return
>>>>>    if (_sample_helper == NULL) return; the else-if block becomes
>>>>> redundant and statement 'if (_sample_helper != NULL)' is not needed.
>>>>>
>>>>> Webrev link:
>> http://cr.openjdk.java.net/~shshahma/8167425/webrev.00/
>>>>> Jdk10 bug link: https://bugs.openjdk.java.net/browse/JDK-8167425
>>>>>
>>>>> Testing: jprt.
>>>>>
>>>>> Thanks,
>>>>> Shafi
>>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: JDK 10 RFR JDK-8167425: Redundant code in method PerfLongVariant::sample

Shafi Ahmad
Hi David,

Yes, you are right.

The comments are not at all needed because of
if (_sample_helper == NULL) return;

Regards,
Shafi

> -----Original Message-----
> From: David Holmes
> Sent: Thursday, March 09, 2017 10:49 AM
> To: Shafi Ahmad <[hidden email]>; Coleen Phillimore
> <[hidden email]>; [hidden email]
> Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
> PerfLongVariant::sample
>
> Hi Shafi,
>
> On 9/03/2017 3:08 PM, Shafi Ahmad wrote:
> > Hi,
> >
> > Please find updated webrev link.
> > http://cr.openjdk.java.net/~shshahma/8167425/webrev.01/
>
> The more I read this code block, the comment and the commented-out
> assertion:
>
>   215 void PerfLongVariant::sample() {
>   216
>   217   // This should not happen.  Maybe the first sample is taken
>   218   // while the _sample_helper is being null'ed out.
>   219   // assert(_sample_helper != NULL || _sampled != NULL,
> "unexpected state");
>   220   if (_sample_helper == NULL) return;
>   221
>   222   *(jlong*)_valuep = _sample_helper->take_sample();
>   223 }
>
> the less it makes sense to me.  ???
>
> David
>
>
>
>
> > Regards,
> > Shafi
> >
> >> -----Original Message-----
> >> From: David Holmes
> >> Sent: Thursday, March 09, 2017 7:35 AM
> >> To: Coleen Phillimore <[hidden email]>; hotspot-
> >> [hidden email]
> >> Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
> >> PerfLongVariant::sample
> >>
> >> Hi Coleen,
> >>
> >> On 9/03/2017 1:42 AM, [hidden email] wrote:
> >>>
> >>> This doesn't look right.
> >>>
> >>> Don't you need to still check for _sampled != NULL?
> >>
> >> That use of _sampled was in dead code that is now removed.
> >>
> >>      if (_sample_helper == NULL) return;
> >> -   if (_sample_helper != NULL) {
> >>        *(jlong*)_valuep = _sample_helper->take_sample();
> >> -   }
> >> -   else if (_sampled != NULL) {
> >> -     *(jlong*)_valuep = *_sampled;
> >> -   }
> >>    }
> >>
> >> This change looks fine to me.
> >>
> >>> Can you remove the JJJ in the comment?
> >>
> >> I second that :)
> >>
> >> Thanks,
> >> David
> >> -----
> >>
> >>>  I don't know what tests were run
> >>> where Jon (aka JJJ in the code) found this, do you?
> >>>
> >>> Thanks,
> >>> Coleen
> >>>
> >>>
> >>> On 3/8/17 10:21 AM, Shafi Ahmad wrote:
> >>>> Hi,
> >>>>
> >>>> May I get the review done for this.
> >>>>
> >>>> Regards,
> >>>> Shafi
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Shafi Ahmad
> >>>>> Sent: Wednesday, March 01, 2017 4:27 PM
> >>>>> To: [hidden email]
> >>>>> Subject: FW: JDK 10 RFR JDK-8167425: Redundant code in method
> >>>>> PerfLongVariant::sample
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> Summary:
> >>>>> It's a very small change to a single file.
> >>>>>
> >>>>> void PerfLongVariant::sample() {
> >>>>>
> >>>>> -  assert(_sample_helper != NULL || _sampled != NULL, "unexpected
> >>>>> state");
> >>>>> +  // JJJ - This should not happen.  Maybe the first sample is
> >>>>> + taken // while the _sample_helper is being null'ed out.
> >>>>> +  // assert(_sample_helper != NULL || _sampled != NULL,
> >>>>> + "unexpected state"); if (_sample_helper == NULL) return;
> >>>>>
> >>>>>    if (_sample_helper != NULL) {
> >>>>>      *(jlong*)_valuep = _sample_helper->take_sample();
> >>>>>    }
> >>>>>    else if (_sampled != NULL) {
> >>>>>      *(jlong*)_valuep = *_sampled;
> >>>>>    }
> >>>>> }
> >>>>> Above five lines are modified in changeset
> >>>>>
> >> http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/da91efe96a93#l802.1
> >>>>> 2 Due to the addition of NULL check and return
> >>>>>    if (_sample_helper == NULL) return; the else-if block becomes
> >>>>> redundant and statement 'if (_sample_helper != NULL)' is not
> needed.
> >>>>>
> >>>>> Webrev link:
> >> http://cr.openjdk.java.net/~shshahma/8167425/webrev.00/
> >>>>> Jdk10 bug link: https://bugs.openjdk.java.net/browse/JDK-8167425
> >>>>>
> >>>>> Testing: jprt.
> >>>>>
> >>>>> Thanks,
> >>>>> Shafi
> >>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: JDK 10 RFR JDK-8167425: Redundant code in method PerfLongVariant::sample

Shafi Ahmad
Hi All,

Let me know if I have to remove the comment completely.

Regards,
Shafi

> -----Original Message-----
> From: Shafi Ahmad
> Sent: Thursday, March 09, 2017 11:01 AM
> To: David Holmes <[hidden email]>; Coleen Phillimore
> <[hidden email]>; [hidden email]
> Subject: RE: JDK 10 RFR JDK-8167425: Redundant code in method
> PerfLongVariant::sample
>
> Hi David,
>
> Yes, you are right.
>
> The comments are not at all needed because of if (_sample_helper == NULL)
> return;
>
> Regards,
> Shafi
> > -----Original Message-----
> > From: David Holmes
> > Sent: Thursday, March 09, 2017 10:49 AM
> > To: Shafi Ahmad <[hidden email]>; Coleen Phillimore
> > <[hidden email]>; [hidden email]
> > Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
> > PerfLongVariant::sample
> >
> > Hi Shafi,
> >
> > On 9/03/2017 3:08 PM, Shafi Ahmad wrote:
> > > Hi,
> > >
> > > Please find updated webrev link.
> > > http://cr.openjdk.java.net/~shshahma/8167425/webrev.01/
> >
> > The more I read this code block, the comment and the commented-out
> > assertion:
> >
> >   215 void PerfLongVariant::sample() {
> >   216
> >   217   // This should not happen.  Maybe the first sample is taken
> >   218   // while the _sample_helper is being null'ed out.
> >   219   // assert(_sample_helper != NULL || _sampled != NULL,
> > "unexpected state");
> >   220   if (_sample_helper == NULL) return;
> >   221
> >   222   *(jlong*)_valuep = _sample_helper->take_sample();
> >   223 }
> >
> > the less it makes sense to me.  ???
> >
> > David
> >
> >
> >
> >
> > > Regards,
> > > Shafi
> > >
> > >> -----Original Message-----
> > >> From: David Holmes
> > >> Sent: Thursday, March 09, 2017 7:35 AM
> > >> To: Coleen Phillimore <[hidden email]>; hotspot-
> > >> [hidden email]
> > >> Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
> > >> PerfLongVariant::sample
> > >>
> > >> Hi Coleen,
> > >>
> > >> On 9/03/2017 1:42 AM, [hidden email] wrote:
> > >>>
> > >>> This doesn't look right.
> > >>>
> > >>> Don't you need to still check for _sampled != NULL?
> > >>
> > >> That use of _sampled was in dead code that is now removed.
> > >>
> > >>      if (_sample_helper == NULL) return;
> > >> -   if (_sample_helper != NULL) {
> > >>        *(jlong*)_valuep = _sample_helper->take_sample();
> > >> -   }
> > >> -   else if (_sampled != NULL) {
> > >> -     *(jlong*)_valuep = *_sampled;
> > >> -   }
> > >>    }
> > >>
> > >> This change looks fine to me.
> > >>
> > >>> Can you remove the JJJ in the comment?
> > >>
> > >> I second that :)
> > >>
> > >> Thanks,
> > >> David
> > >> -----
> > >>
> > >>>  I don't know what tests were run
> > >>> where Jon (aka JJJ in the code) found this, do you?
> > >>>
> > >>> Thanks,
> > >>> Coleen
> > >>>
> > >>>
> > >>> On 3/8/17 10:21 AM, Shafi Ahmad wrote:
> > >>>> Hi,
> > >>>>
> > >>>> May I get the review done for this.
> > >>>>
> > >>>> Regards,
> > >>>> Shafi
> > >>>>
> > >>>>> -----Original Message-----
> > >>>>> From: Shafi Ahmad
> > >>>>> Sent: Wednesday, March 01, 2017 4:27 PM
> > >>>>> To: [hidden email]
> > >>>>> Subject: FW: JDK 10 RFR JDK-8167425: Redundant code in method
> > >>>>> PerfLongVariant::sample
> > >>>>>
> > >>>>> Hi,
> > >>>>>
> > >>>>> Summary:
> > >>>>> It's a very small change to a single file.
> > >>>>>
> > >>>>> void PerfLongVariant::sample() {
> > >>>>>
> > >>>>> -  assert(_sample_helper != NULL || _sampled != NULL,
> > >>>>> "unexpected state");
> > >>>>> +  // JJJ - This should not happen.  Maybe the first sample is
> > >>>>> + taken // while the _sample_helper is being null'ed out.
> > >>>>> +  // assert(_sample_helper != NULL || _sampled != NULL,
> > >>>>> + "unexpected state"); if (_sample_helper == NULL) return;
> > >>>>>
> > >>>>>    if (_sample_helper != NULL) {
> > >>>>>      *(jlong*)_valuep = _sample_helper->take_sample();
> > >>>>>    }
> > >>>>>    else if (_sampled != NULL) {
> > >>>>>      *(jlong*)_valuep = *_sampled;
> > >>>>>    }
> > >>>>> }
> > >>>>> Above five lines are modified in changeset
> > >>>>>
> > >> http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/da91efe96a93#l802.
> > >> 1
> > >>>>> 2 Due to the addition of NULL check and return
> > >>>>>    if (_sample_helper == NULL) return; the else-if block becomes
> > >>>>> redundant and statement 'if (_sample_helper != NULL)' is not
> > needed.
> > >>>>>
> > >>>>> Webrev link:
> > >> http://cr.openjdk.java.net/~shshahma/8167425/webrev.00/
> > >>>>> Jdk10 bug link: https://bugs.openjdk.java.net/browse/JDK-8167425
> > >>>>>
> > >>>>> Testing: jprt.
> > >>>>>
> > >>>>> Thanks,
> > >>>>> Shafi
> > >>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 10 RFR JDK-8167425: Redundant code in method PerfLongVariant::sample

David Holmes
Hi Shafi,

On 9/03/2017 9:21 PM, Shafi Ahmad wrote:
> Hi All,
>
> Let me know if I have to remove the comment completely.

I don't think the comment makes much sense, so assuming the code is
logically correct I would remove it. I'd then suggest simplifying the
method to just:

void PerfLongVariant::sample() {
   if (_sample_helper != NULL) {
     *(jlong*)_valuep = _sample_helper->take_sample();
   }
}

But I have to say that I don't really understand what this code is doing.

David

> Regards,
> Shafi
>
>> -----Original Message-----
>> From: Shafi Ahmad
>> Sent: Thursday, March 09, 2017 11:01 AM
>> To: David Holmes <[hidden email]>; Coleen Phillimore
>> <[hidden email]>; [hidden email]
>> Subject: RE: JDK 10 RFR JDK-8167425: Redundant code in method
>> PerfLongVariant::sample
>>
>> Hi David,
>>
>> Yes, you are right.
>>
>> The comments are not at all needed because of if (_sample_helper == NULL)
>> return;
>>
>> Regards,
>> Shafi
>>> -----Original Message-----
>>> From: David Holmes
>>> Sent: Thursday, March 09, 2017 10:49 AM
>>> To: Shafi Ahmad <[hidden email]>; Coleen Phillimore
>>> <[hidden email]>; [hidden email]
>>> Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
>>> PerfLongVariant::sample
>>>
>>> Hi Shafi,
>>>
>>> On 9/03/2017 3:08 PM, Shafi Ahmad wrote:
>>>> Hi,
>>>>
>>>> Please find updated webrev link.
>>>> http://cr.openjdk.java.net/~shshahma/8167425/webrev.01/
>>>
>>> The more I read this code block, the comment and the commented-out
>>> assertion:
>>>
>>>   215 void PerfLongVariant::sample() {
>>>   216
>>>   217   // This should not happen.  Maybe the first sample is taken
>>>   218   // while the _sample_helper is being null'ed out.
>>>   219   // assert(_sample_helper != NULL || _sampled != NULL,
>>> "unexpected state");
>>>   220   if (_sample_helper == NULL) return;
>>>   221
>>>   222   *(jlong*)_valuep = _sample_helper->take_sample();
>>>   223 }
>>>
>>> the less it makes sense to me.  ???
>>>
>>> David
>>>
>>>
>>>
>>>
>>>> Regards,
>>>> Shafi
>>>>
>>>>> -----Original Message-----
>>>>> From: David Holmes
>>>>> Sent: Thursday, March 09, 2017 7:35 AM
>>>>> To: Coleen Phillimore <[hidden email]>; hotspot-
>>>>> [hidden email]
>>>>> Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
>>>>> PerfLongVariant::sample
>>>>>
>>>>> Hi Coleen,
>>>>>
>>>>> On 9/03/2017 1:42 AM, [hidden email] wrote:
>>>>>>
>>>>>> This doesn't look right.
>>>>>>
>>>>>> Don't you need to still check for _sampled != NULL?
>>>>>
>>>>> That use of _sampled was in dead code that is now removed.
>>>>>
>>>>>      if (_sample_helper == NULL) return;
>>>>> -   if (_sample_helper != NULL) {
>>>>>        *(jlong*)_valuep = _sample_helper->take_sample();
>>>>> -   }
>>>>> -   else if (_sampled != NULL) {
>>>>> -     *(jlong*)_valuep = *_sampled;
>>>>> -   }
>>>>>    }
>>>>>
>>>>> This change looks fine to me.
>>>>>
>>>>>> Can you remove the JJJ in the comment?
>>>>>
>>>>> I second that :)
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>>  I don't know what tests were run
>>>>>> where Jon (aka JJJ in the code) found this, do you?
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>>>
>>>>>>
>>>>>> On 3/8/17 10:21 AM, Shafi Ahmad wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> May I get the review done for this.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Shafi
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Shafi Ahmad
>>>>>>>> Sent: Wednesday, March 01, 2017 4:27 PM
>>>>>>>> To: [hidden email]
>>>>>>>> Subject: FW: JDK 10 RFR JDK-8167425: Redundant code in method
>>>>>>>> PerfLongVariant::sample
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Summary:
>>>>>>>> It's a very small change to a single file.
>>>>>>>>
>>>>>>>> void PerfLongVariant::sample() {
>>>>>>>>
>>>>>>>> -  assert(_sample_helper != NULL || _sampled != NULL,
>>>>>>>> "unexpected state");
>>>>>>>> +  // JJJ - This should not happen.  Maybe the first sample is
>>>>>>>> + taken // while the _sample_helper is being null'ed out.
>>>>>>>> +  // assert(_sample_helper != NULL || _sampled != NULL,
>>>>>>>> + "unexpected state"); if (_sample_helper == NULL) return;
>>>>>>>>
>>>>>>>>    if (_sample_helper != NULL) {
>>>>>>>>      *(jlong*)_valuep = _sample_helper->take_sample();
>>>>>>>>    }
>>>>>>>>    else if (_sampled != NULL) {
>>>>>>>>      *(jlong*)_valuep = *_sampled;
>>>>>>>>    }
>>>>>>>> }
>>>>>>>> Above five lines are modified in changeset
>>>>>>>>
>>>>> http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/da91efe96a93#l802.
>>>>> 1
>>>>>>>> 2 Due to the addition of NULL check and return
>>>>>>>>    if (_sample_helper == NULL) return; the else-if block becomes
>>>>>>>> redundant and statement 'if (_sample_helper != NULL)' is not
>>> needed.
>>>>>>>>
>>>>>>>> Webrev link:
>>>>> http://cr.openjdk.java.net/~shshahma/8167425/webrev.00/
>>>>>>>> Jdk10 bug link: https://bugs.openjdk.java.net/browse/JDK-8167425
>>>>>>>>
>>>>>>>> Testing: jprt.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Shafi
>>>>>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 10 RFR JDK-8167425: Redundant code in method PerfLongVariant::sample

coleen.phillimore
In reply to this post by David Holmes


On 3/8/17 9:05 PM, David Holmes wrote:
> Hi Coleen,
>
> On 9/03/2017 1:42 AM, [hidden email] wrote:
>>
>> This doesn't look right.
>>
>> Don't you need to still check for _sampled != NULL?
>
> That use of _sampled was in dead code that is now removed.

I see it all now.
Coleen

>
>     if (_sample_helper == NULL) return;
> -   if (_sample_helper != NULL) {
>       *(jlong*)_valuep = _sample_helper->take_sample();
> -   }
> -   else if (_sampled != NULL) {
> -     *(jlong*)_valuep = *_sampled;
> -   }
>   }
>
> This change looks fine to me.
>
>> Can you remove the JJJ in the comment?
>
> I second that :)
>
> Thanks,
> David
> -----
>
>>  I don't know what tests were run
>> where Jon (aka JJJ in the code) found this, do you?
>>
>> Thanks,
>> Coleen
>>
>>
>> On 3/8/17 10:21 AM, Shafi Ahmad wrote:
>>> Hi,
>>>
>>> May I get the review done for this.
>>>
>>> Regards,
>>> Shafi
>>>
>>>> -----Original Message-----
>>>> From: Shafi Ahmad
>>>> Sent: Wednesday, March 01, 2017 4:27 PM
>>>> To: [hidden email]
>>>> Subject: FW: JDK 10 RFR JDK-8167425: Redundant code in method
>>>> PerfLongVariant::sample
>>>>
>>>> Hi,
>>>>
>>>> Summary:
>>>> It's a very small change to a single file.
>>>>
>>>> void PerfLongVariant::sample() {
>>>>
>>>> -  assert(_sample_helper != NULL || _sampled != NULL, "unexpected
>>>> state");
>>>> +  // JJJ - This should not happen.  Maybe the first sample is
>>>> taken  //
>>>> + while the _sample_helper is being null'ed out.
>>>> +  // assert(_sample_helper != NULL || _sampled != NULL, "unexpected
>>>> + state"); if (_sample_helper == NULL) return;
>>>>
>>>>    if (_sample_helper != NULL) {
>>>>      *(jlong*)_valuep = _sample_helper->take_sample();
>>>>    }
>>>>    else if (_sampled != NULL) {
>>>>      *(jlong*)_valuep = *_sampled;
>>>>    }
>>>> }
>>>> Above five lines are modified in changeset
>>>> http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/da91efe96a93#l802.12
>>>> Due to the addition of NULL check and return
>>>>    if (_sample_helper == NULL) return;
>>>> the else-if block becomes redundant and statement 'if
>>>> (_sample_helper !=
>>>> NULL)' is not needed.
>>>>
>>>> Webrev link: http://cr.openjdk.java.net/~shshahma/8167425/webrev.00/
>>>> Jdk10 bug link: https://bugs.openjdk.java.net/browse/JDK-8167425
>>>>
>>>> Testing: jprt.
>>>>
>>>> Thanks,
>>>> Shafi
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 10 RFR JDK-8167425: Redundant code in method PerfLongVariant::sample

coleen.phillimore
In reply to this post by David Holmes


On 3/9/17 6:54 AM, David Holmes wrote:

> Hi Shafi,
>
> On 9/03/2017 9:21 PM, Shafi Ahmad wrote:
>> Hi All,
>>
>> Let me know if I have to remove the comment completely.
>
> I don't think the comment makes much sense, so assuming the code is
> logically correct I would remove it. I'd then suggest simplifying the
> method to just:
>
> void PerfLongVariant::sample() {
>   if (_sample_helper != NULL) {
>     *(jlong*)_valuep = _sample_helper->take_sample();
>   }
> }
>
> But I have to say that I don't really understand what this code is doing.

I don't either.   I think the version without comments is probably the
least confusing and looks correct.  If you change it to this, I don't
need to see a webrev again.

thanks,
Coleen

>
> David
>
>> Regards,
>> Shafi
>>
>>> -----Original Message-----
>>> From: Shafi Ahmad
>>> Sent: Thursday, March 09, 2017 11:01 AM
>>> To: David Holmes <[hidden email]>; Coleen Phillimore
>>> <[hidden email]>; [hidden email]
>>> Subject: RE: JDK 10 RFR JDK-8167425: Redundant code in method
>>> PerfLongVariant::sample
>>>
>>> Hi David,
>>>
>>> Yes, you are right.
>>>
>>> The comments are not at all needed because of if (_sample_helper ==
>>> NULL)
>>> return;
>>>
>>> Regards,
>>> Shafi
>>>> -----Original Message-----
>>>> From: David Holmes
>>>> Sent: Thursday, March 09, 2017 10:49 AM
>>>> To: Shafi Ahmad <[hidden email]>; Coleen Phillimore
>>>> <[hidden email]>; [hidden email]
>>>> Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
>>>> PerfLongVariant::sample
>>>>
>>>> Hi Shafi,
>>>>
>>>> On 9/03/2017 3:08 PM, Shafi Ahmad wrote:
>>>>> Hi,
>>>>>
>>>>> Please find updated webrev link.
>>>>> http://cr.openjdk.java.net/~shshahma/8167425/webrev.01/
>>>>
>>>> The more I read this code block, the comment and the commented-out
>>>> assertion:
>>>>
>>>>   215 void PerfLongVariant::sample() {
>>>>   216
>>>>   217   // This should not happen.  Maybe the first sample is taken
>>>>   218   // while the _sample_helper is being null'ed out.
>>>>   219   // assert(_sample_helper != NULL || _sampled != NULL,
>>>> "unexpected state");
>>>>   220   if (_sample_helper == NULL) return;
>>>>   221
>>>>   222   *(jlong*)_valuep = _sample_helper->take_sample();
>>>>   223 }
>>>>
>>>> the less it makes sense to me.  ???
>>>>
>>>> David
>>>>
>>>>
>>>>
>>>>
>>>>> Regards,
>>>>> Shafi
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: David Holmes
>>>>>> Sent: Thursday, March 09, 2017 7:35 AM
>>>>>> To: Coleen Phillimore <[hidden email]>; hotspot-
>>>>>> [hidden email]
>>>>>> Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
>>>>>> PerfLongVariant::sample
>>>>>>
>>>>>> Hi Coleen,
>>>>>>
>>>>>> On 9/03/2017 1:42 AM, [hidden email] wrote:
>>>>>>>
>>>>>>> This doesn't look right.
>>>>>>>
>>>>>>> Don't you need to still check for _sampled != NULL?
>>>>>>
>>>>>> That use of _sampled was in dead code that is now removed.
>>>>>>
>>>>>>      if (_sample_helper == NULL) return;
>>>>>> -   if (_sample_helper != NULL) {
>>>>>>        *(jlong*)_valuep = _sample_helper->take_sample();
>>>>>> -   }
>>>>>> -   else if (_sampled != NULL) {
>>>>>> -     *(jlong*)_valuep = *_sampled;
>>>>>> -   }
>>>>>>    }
>>>>>>
>>>>>> This change looks fine to me.
>>>>>>
>>>>>>> Can you remove the JJJ in the comment?
>>>>>>
>>>>>> I second that :)
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>>>  I don't know what tests were run
>>>>>>> where Jon (aka JJJ in the code) found this, do you?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Coleen
>>>>>>>
>>>>>>>
>>>>>>> On 3/8/17 10:21 AM, Shafi Ahmad wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> May I get the review done for this.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Shafi
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Shafi Ahmad
>>>>>>>>> Sent: Wednesday, March 01, 2017 4:27 PM
>>>>>>>>> To: [hidden email]
>>>>>>>>> Subject: FW: JDK 10 RFR JDK-8167425: Redundant code in method
>>>>>>>>> PerfLongVariant::sample
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Summary:
>>>>>>>>> It's a very small change to a single file.
>>>>>>>>>
>>>>>>>>> void PerfLongVariant::sample() {
>>>>>>>>>
>>>>>>>>> -  assert(_sample_helper != NULL || _sampled != NULL,
>>>>>>>>> "unexpected state");
>>>>>>>>> +  // JJJ - This should not happen.  Maybe the first sample is
>>>>>>>>> + taken // while the _sample_helper is being null'ed out.
>>>>>>>>> +  // assert(_sample_helper != NULL || _sampled != NULL,
>>>>>>>>> + "unexpected state"); if (_sample_helper == NULL) return;
>>>>>>>>>
>>>>>>>>>    if (_sample_helper != NULL) {
>>>>>>>>>      *(jlong*)_valuep = _sample_helper->take_sample();
>>>>>>>>>    }
>>>>>>>>>    else if (_sampled != NULL) {
>>>>>>>>>      *(jlong*)_valuep = *_sampled;
>>>>>>>>>    }
>>>>>>>>> }
>>>>>>>>> Above five lines are modified in changeset
>>>>>>>>>
>>>>>> http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/da91efe96a93#l802.
>>>>>> 1
>>>>>>>>> 2 Due to the addition of NULL check and return
>>>>>>>>>    if (_sample_helper == NULL) return; the else-if block becomes
>>>>>>>>> redundant and statement 'if (_sample_helper != NULL)' is not
>>>> needed.
>>>>>>>>>
>>>>>>>>> Webrev link:
>>>>>> http://cr.openjdk.java.net/~shshahma/8167425/webrev.00/
>>>>>>>>> Jdk10 bug link: https://bugs.openjdk.java.net/browse/JDK-8167425
>>>>>>>>>
>>>>>>>>> Testing: jprt.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Shafi
>>>>>>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 10 RFR JDK-8167425: Redundant code in method PerfLongVariant::sample

serguei.spitsyn@oracle.com
In reply to this post by Shafi Ahmad
Hi Shafi,

I agree the comment is not needed.
Otherwise, it looks good to me.

Thanks,
Serguei


On 3/8/17 21:30, Shafi Ahmad wrote:

> Hi David,
>
> Yes, you are right.
>
> The comments are not at all needed because of
> if (_sample_helper == NULL) return;
>
> Regards,
> Shafi
>> -----Original Message-----
>> From: David Holmes
>> Sent: Thursday, March 09, 2017 10:49 AM
>> To: Shafi Ahmad <[hidden email]>; Coleen Phillimore
>> <[hidden email]>; [hidden email]
>> Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
>> PerfLongVariant::sample
>>
>> Hi Shafi,
>>
>> On 9/03/2017 3:08 PM, Shafi Ahmad wrote:
>>> Hi,
>>>
>>> Please find updated webrev link.
>>> http://cr.openjdk.java.net/~shshahma/8167425/webrev.01/
>> The more I read this code block, the comment and the commented-out
>> assertion:
>>
>>    215 void PerfLongVariant::sample() {
>>    216
>>    217   // This should not happen.  Maybe the first sample is taken
>>    218   // while the _sample_helper is being null'ed out.
>>    219   // assert(_sample_helper != NULL || _sampled != NULL,
>> "unexpected state");
>>    220   if (_sample_helper == NULL) return;
>>    221
>>    222   *(jlong*)_valuep = _sample_helper->take_sample();
>>    223 }
>>
>> the less it makes sense to me.  ???
>>
>> David
>>
>>
>>
>>
>>> Regards,
>>> Shafi
>>>
>>>> -----Original Message-----
>>>> From: David Holmes
>>>> Sent: Thursday, March 09, 2017 7:35 AM
>>>> To: Coleen Phillimore <[hidden email]>; hotspot-
>>>> [hidden email]
>>>> Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
>>>> PerfLongVariant::sample
>>>>
>>>> Hi Coleen,
>>>>
>>>> On 9/03/2017 1:42 AM, [hidden email] wrote:
>>>>> This doesn't look right.
>>>>>
>>>>> Don't you need to still check for _sampled != NULL?
>>>> That use of _sampled was in dead code that is now removed.
>>>>
>>>>       if (_sample_helper == NULL) return;
>>>> -   if (_sample_helper != NULL) {
>>>>         *(jlong*)_valuep = _sample_helper->take_sample();
>>>> -   }
>>>> -   else if (_sampled != NULL) {
>>>> -     *(jlong*)_valuep = *_sampled;
>>>> -   }
>>>>     }
>>>>
>>>> This change looks fine to me.
>>>>
>>>>> Can you remove the JJJ in the comment?
>>>> I second that :)
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>>   I don't know what tests were run
>>>>> where Jon (aka JJJ in the code) found this, do you?
>>>>>
>>>>> Thanks,
>>>>> Coleen
>>>>>
>>>>>
>>>>> On 3/8/17 10:21 AM, Shafi Ahmad wrote:
>>>>>> Hi,
>>>>>>
>>>>>> May I get the review done for this.
>>>>>>
>>>>>> Regards,
>>>>>> Shafi
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Shafi Ahmad
>>>>>>> Sent: Wednesday, March 01, 2017 4:27 PM
>>>>>>> To: [hidden email]
>>>>>>> Subject: FW: JDK 10 RFR JDK-8167425: Redundant code in method
>>>>>>> PerfLongVariant::sample
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> Summary:
>>>>>>> It's a very small change to a single file.
>>>>>>>
>>>>>>> void PerfLongVariant::sample() {
>>>>>>>
>>>>>>> -  assert(_sample_helper != NULL || _sampled != NULL, "unexpected
>>>>>>> state");
>>>>>>> +  // JJJ - This should not happen.  Maybe the first sample is
>>>>>>> + taken // while the _sample_helper is being null'ed out.
>>>>>>> +  // assert(_sample_helper != NULL || _sampled != NULL,
>>>>>>> + "unexpected state"); if (_sample_helper == NULL) return;
>>>>>>>
>>>>>>>     if (_sample_helper != NULL) {
>>>>>>>       *(jlong*)_valuep = _sample_helper->take_sample();
>>>>>>>     }
>>>>>>>     else if (_sampled != NULL) {
>>>>>>>       *(jlong*)_valuep = *_sampled;
>>>>>>>     }
>>>>>>> }
>>>>>>> Above five lines are modified in changeset
>>>>>>>
>>>> http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/da91efe96a93#l802.1
>>>>>>> 2 Due to the addition of NULL check and return
>>>>>>>     if (_sample_helper == NULL) return; the else-if block becomes
>>>>>>> redundant and statement 'if (_sample_helper != NULL)' is not
>> needed.
>>>>>>> Webrev link:
>>>> http://cr.openjdk.java.net/~shshahma/8167425/webrev.00/
>>>>>>> Jdk10 bug link: https://bugs.openjdk.java.net/browse/JDK-8167425
>>>>>>>
>>>>>>> Testing: jprt.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Shafi

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: JDK 10 RFR JDK-8167425: Redundant code in method PerfLongVariant::sample

Shafi Ahmad
Hi,

Thank you Coleen, David and Serguei  for the review.

Please find updated webrev
http://cr.openjdk.java.net/~shshahma/8167425/webrev.02/

Regards,
Shafi

> -----Original Message-----
> From: Serguei Spitsyn
> Sent: Friday, March 10, 2017 4:29 PM
> To: Shafi Ahmad <[hidden email]>; David Holmes
> <[hidden email]>; Coleen Phillimore
> <[hidden email]>; [hidden email]
> Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
> PerfLongVariant::sample
>
> Hi Shafi,
>
> I agree the comment is not needed.
> Otherwise, it looks good to me.
>
> Thanks,
> Serguei
>
>
> On 3/8/17 21:30, Shafi Ahmad wrote:
> > Hi David,
> >
> > Yes, you are right.
> >
> > The comments are not at all needed because of if (_sample_helper ==
> > NULL) return;
> >
> > Regards,
> > Shafi
> >> -----Original Message-----
> >> From: David Holmes
> >> Sent: Thursday, March 09, 2017 10:49 AM
> >> To: Shafi Ahmad <[hidden email]>; Coleen Phillimore
> >> <[hidden email]>; [hidden email]
> >> Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
> >> PerfLongVariant::sample
> >>
> >> Hi Shafi,
> >>
> >> On 9/03/2017 3:08 PM, Shafi Ahmad wrote:
> >>> Hi,
> >>>
> >>> Please find updated webrev link.
> >>> http://cr.openjdk.java.net/~shshahma/8167425/webrev.01/
> >> The more I read this code block, the comment and the commented-out
> >> assertion:
> >>
> >>    215 void PerfLongVariant::sample() {
> >>    216
> >>    217   // This should not happen.  Maybe the first sample is taken
> >>    218   // while the _sample_helper is being null'ed out.
> >>    219   // assert(_sample_helper != NULL || _sampled != NULL,
> >> "unexpected state");
> >>    220   if (_sample_helper == NULL) return;
> >>    221
> >>    222   *(jlong*)_valuep = _sample_helper->take_sample();
> >>    223 }
> >>
> >> the less it makes sense to me.  ???
> >>
> >> David
> >>
> >>
> >>
> >>
> >>> Regards,
> >>> Shafi
> >>>
> >>>> -----Original Message-----
> >>>> From: David Holmes
> >>>> Sent: Thursday, March 09, 2017 7:35 AM
> >>>> To: Coleen Phillimore <[hidden email]>; hotspot-
> >>>> [hidden email]
> >>>> Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
> >>>> PerfLongVariant::sample
> >>>>
> >>>> Hi Coleen,
> >>>>
> >>>> On 9/03/2017 1:42 AM, [hidden email] wrote:
> >>>>> This doesn't look right.
> >>>>>
> >>>>> Don't you need to still check for _sampled != NULL?
> >>>> That use of _sampled was in dead code that is now removed.
> >>>>
> >>>>       if (_sample_helper == NULL) return;
> >>>> -   if (_sample_helper != NULL) {
> >>>>         *(jlong*)_valuep = _sample_helper->take_sample();
> >>>> -   }
> >>>> -   else if (_sampled != NULL) {
> >>>> -     *(jlong*)_valuep = *_sampled;
> >>>> -   }
> >>>>     }
> >>>>
> >>>> This change looks fine to me.
> >>>>
> >>>>> Can you remove the JJJ in the comment?
> >>>> I second that :)
> >>>>
> >>>> Thanks,
> >>>> David
> >>>> -----
> >>>>
> >>>>>   I don't know what tests were run where Jon (aka JJJ in the code)
> >>>>> found this, do you?
> >>>>>
> >>>>> Thanks,
> >>>>> Coleen
> >>>>>
> >>>>>
> >>>>> On 3/8/17 10:21 AM, Shafi Ahmad wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> May I get the review done for this.
> >>>>>>
> >>>>>> Regards,
> >>>>>> Shafi
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: Shafi Ahmad
> >>>>>>> Sent: Wednesday, March 01, 2017 4:27 PM
> >>>>>>> To: [hidden email]
> >>>>>>> Subject: FW: JDK 10 RFR JDK-8167425: Redundant code in method
> >>>>>>> PerfLongVariant::sample
> >>>>>>>
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> Summary:
> >>>>>>> It's a very small change to a single file.
> >>>>>>>
> >>>>>>> void PerfLongVariant::sample() {
> >>>>>>>
> >>>>>>> -  assert(_sample_helper != NULL || _sampled != NULL,
> >>>>>>> "unexpected state");
> >>>>>>> +  // JJJ - This should not happen.  Maybe the first sample is
> >>>>>>> + taken // while the _sample_helper is being null'ed out.
> >>>>>>> +  // assert(_sample_helper != NULL || _sampled != NULL,
> >>>>>>> + "unexpected state"); if (_sample_helper == NULL) return;
> >>>>>>>
> >>>>>>>     if (_sample_helper != NULL) {
> >>>>>>>       *(jlong*)_valuep = _sample_helper->take_sample();
> >>>>>>>     }
> >>>>>>>     else if (_sampled != NULL) {
> >>>>>>>       *(jlong*)_valuep = *_sampled;
> >>>>>>>     }
> >>>>>>> }
> >>>>>>> Above five lines are modified in changeset
> >>>>>>>
> >>>>
> http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/da91efe96a93#l802.
> >>>> 1
> >>>>>>> 2 Due to the addition of NULL check and return
> >>>>>>>     if (_sample_helper == NULL) return; the else-if block
> >>>>>>> becomes redundant and statement 'if (_sample_helper != NULL)' is
> >>>>>>> not
> >> needed.
> >>>>>>> Webrev link:
> >>>> http://cr.openjdk.java.net/~shshahma/8167425/webrev.00/
> >>>>>>> Jdk10 bug link: https://bugs.openjdk.java.net/browse/JDK-8167425
> >>>>>>>
> >>>>>>> Testing: jprt.
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>> Shafi
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 10 RFR JDK-8167425: Redundant code in method PerfLongVariant::sample

David Holmes
Hi Shafi,

Stylistically this looks very awkward, please rewrite as:

void PerfLongVariant::sample() {
   if (_sample_helper != NULL) {
     *(jlong*)_valuep = _sample_helper->take_sample();
   }
}

No need for further webrevs.

Thanks,
David

On 10/03/2017 9:13 PM, Shafi Ahmad wrote:

> Hi,
>
> Thank you Coleen, David and Serguei  for the review.
>
> Please find updated webrev
> http://cr.openjdk.java.net/~shshahma/8167425/webrev.02/
>
> Regards,
> Shafi
>
>> -----Original Message-----
>> From: Serguei Spitsyn
>> Sent: Friday, March 10, 2017 4:29 PM
>> To: Shafi Ahmad <[hidden email]>; David Holmes
>> <[hidden email]>; Coleen Phillimore
>> <[hidden email]>; [hidden email]
>> Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
>> PerfLongVariant::sample
>>
>> Hi Shafi,
>>
>> I agree the comment is not needed.
>> Otherwise, it looks good to me.
>>
>> Thanks,
>> Serguei
>>
>>
>> On 3/8/17 21:30, Shafi Ahmad wrote:
>>> Hi David,
>>>
>>> Yes, you are right.
>>>
>>> The comments are not at all needed because of if (_sample_helper ==
>>> NULL) return;
>>>
>>> Regards,
>>> Shafi
>>>> -----Original Message-----
>>>> From: David Holmes
>>>> Sent: Thursday, March 09, 2017 10:49 AM
>>>> To: Shafi Ahmad <[hidden email]>; Coleen Phillimore
>>>> <[hidden email]>; [hidden email]
>>>> Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
>>>> PerfLongVariant::sample
>>>>
>>>> Hi Shafi,
>>>>
>>>> On 9/03/2017 3:08 PM, Shafi Ahmad wrote:
>>>>> Hi,
>>>>>
>>>>> Please find updated webrev link.
>>>>> http://cr.openjdk.java.net/~shshahma/8167425/webrev.01/
>>>> The more I read this code block, the comment and the commented-out
>>>> assertion:
>>>>
>>>>    215 void PerfLongVariant::sample() {
>>>>    216
>>>>    217   // This should not happen.  Maybe the first sample is taken
>>>>    218   // while the _sample_helper is being null'ed out.
>>>>    219   // assert(_sample_helper != NULL || _sampled != NULL,
>>>> "unexpected state");
>>>>    220   if (_sample_helper == NULL) return;
>>>>    221
>>>>    222   *(jlong*)_valuep = _sample_helper->take_sample();
>>>>    223 }
>>>>
>>>> the less it makes sense to me.  ???
>>>>
>>>> David
>>>>
>>>>
>>>>
>>>>
>>>>> Regards,
>>>>> Shafi
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: David Holmes
>>>>>> Sent: Thursday, March 09, 2017 7:35 AM
>>>>>> To: Coleen Phillimore <[hidden email]>; hotspot-
>>>>>> [hidden email]
>>>>>> Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
>>>>>> PerfLongVariant::sample
>>>>>>
>>>>>> Hi Coleen,
>>>>>>
>>>>>> On 9/03/2017 1:42 AM, [hidden email] wrote:
>>>>>>> This doesn't look right.
>>>>>>>
>>>>>>> Don't you need to still check for _sampled != NULL?
>>>>>> That use of _sampled was in dead code that is now removed.
>>>>>>
>>>>>>       if (_sample_helper == NULL) return;
>>>>>> -   if (_sample_helper != NULL) {
>>>>>>         *(jlong*)_valuep = _sample_helper->take_sample();
>>>>>> -   }
>>>>>> -   else if (_sampled != NULL) {
>>>>>> -     *(jlong*)_valuep = *_sampled;
>>>>>> -   }
>>>>>>     }
>>>>>>
>>>>>> This change looks fine to me.
>>>>>>
>>>>>>> Can you remove the JJJ in the comment?
>>>>>> I second that :)
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>>>   I don't know what tests were run where Jon (aka JJJ in the code)
>>>>>>> found this, do you?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Coleen
>>>>>>>
>>>>>>>
>>>>>>> On 3/8/17 10:21 AM, Shafi Ahmad wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> May I get the review done for this.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Shafi
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Shafi Ahmad
>>>>>>>>> Sent: Wednesday, March 01, 2017 4:27 PM
>>>>>>>>> To: [hidden email]
>>>>>>>>> Subject: FW: JDK 10 RFR JDK-8167425: Redundant code in method
>>>>>>>>> PerfLongVariant::sample
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Summary:
>>>>>>>>> It's a very small change to a single file.
>>>>>>>>>
>>>>>>>>> void PerfLongVariant::sample() {
>>>>>>>>>
>>>>>>>>> -  assert(_sample_helper != NULL || _sampled != NULL,
>>>>>>>>> "unexpected state");
>>>>>>>>> +  // JJJ - This should not happen.  Maybe the first sample is
>>>>>>>>> + taken // while the _sample_helper is being null'ed out.
>>>>>>>>> +  // assert(_sample_helper != NULL || _sampled != NULL,
>>>>>>>>> + "unexpected state"); if (_sample_helper == NULL) return;
>>>>>>>>>
>>>>>>>>>     if (_sample_helper != NULL) {
>>>>>>>>>       *(jlong*)_valuep = _sample_helper->take_sample();
>>>>>>>>>     }
>>>>>>>>>     else if (_sampled != NULL) {
>>>>>>>>>       *(jlong*)_valuep = *_sampled;
>>>>>>>>>     }
>>>>>>>>> }
>>>>>>>>> Above five lines are modified in changeset
>>>>>>>>>
>>>>>>
>> http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/da91efe96a93#l802.
>>>>>> 1
>>>>>>>>> 2 Due to the addition of NULL check and return
>>>>>>>>>     if (_sample_helper == NULL) return; the else-if block
>>>>>>>>> becomes redundant and statement 'if (_sample_helper != NULL)' is
>>>>>>>>> not
>>>> needed.
>>>>>>>>> Webrev link:
>>>>>> http://cr.openjdk.java.net/~shshahma/8167425/webrev.00/
>>>>>>>>> Jdk10 bug link: https://bugs.openjdk.java.net/browse/JDK-8167425
>>>>>>>>>
>>>>>>>>> Testing: jprt.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Shafi
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 10 RFR JDK-8167425: Redundant code in method PerfLongVariant::sample

serguei.spitsyn@oracle.com
+1

Thanks,
Serguei


On 3/10/17 03:56, David Holmes wrote:

> Hi Shafi,
>
> Stylistically this looks very awkward, please rewrite as:
>
> void PerfLongVariant::sample() {
>   if (_sample_helper != NULL) {
>     *(jlong*)_valuep = _sample_helper->take_sample();
>   }
> }
>
> No need for further webrevs.
>
> Thanks,
> David
>
> On 10/03/2017 9:13 PM, Shafi Ahmad wrote:
>> Hi,
>>
>> Thank you Coleen, David and Serguei  for the review.
>>
>> Please find updated webrev
>> http://cr.openjdk.java.net/~shshahma/8167425/webrev.02/
>>
>> Regards,
>> Shafi
>>
>>> -----Original Message-----
>>> From: Serguei Spitsyn
>>> Sent: Friday, March 10, 2017 4:29 PM
>>> To: Shafi Ahmad <[hidden email]>; David Holmes
>>> <[hidden email]>; Coleen Phillimore
>>> <[hidden email]>; [hidden email]
>>> Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
>>> PerfLongVariant::sample
>>>
>>> Hi Shafi,
>>>
>>> I agree the comment is not needed.
>>> Otherwise, it looks good to me.
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 3/8/17 21:30, Shafi Ahmad wrote:
>>>> Hi David,
>>>>
>>>> Yes, you are right.
>>>>
>>>> The comments are not at all needed because of if (_sample_helper ==
>>>> NULL) return;
>>>>
>>>> Regards,
>>>> Shafi
>>>>> -----Original Message-----
>>>>> From: David Holmes
>>>>> Sent: Thursday, March 09, 2017 10:49 AM
>>>>> To: Shafi Ahmad <[hidden email]>; Coleen Phillimore
>>>>> <[hidden email]>; [hidden email]
>>>>> Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
>>>>> PerfLongVariant::sample
>>>>>
>>>>> Hi Shafi,
>>>>>
>>>>> On 9/03/2017 3:08 PM, Shafi Ahmad wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please find updated webrev link.
>>>>>> http://cr.openjdk.java.net/~shshahma/8167425/webrev.01/
>>>>> The more I read this code block, the comment and the commented-out
>>>>> assertion:
>>>>>
>>>>>    215 void PerfLongVariant::sample() {
>>>>>    216
>>>>>    217   // This should not happen.  Maybe the first sample is taken
>>>>>    218   // while the _sample_helper is being null'ed out.
>>>>>    219   // assert(_sample_helper != NULL || _sampled != NULL,
>>>>> "unexpected state");
>>>>>    220   if (_sample_helper == NULL) return;
>>>>>    221
>>>>>    222   *(jlong*)_valuep = _sample_helper->take_sample();
>>>>>    223 }
>>>>>
>>>>> the less it makes sense to me.  ???
>>>>>
>>>>> David
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>> Regards,
>>>>>> Shafi
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: David Holmes
>>>>>>> Sent: Thursday, March 09, 2017 7:35 AM
>>>>>>> To: Coleen Phillimore <[hidden email]>; hotspot-
>>>>>>> [hidden email]
>>>>>>> Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
>>>>>>> PerfLongVariant::sample
>>>>>>>
>>>>>>> Hi Coleen,
>>>>>>>
>>>>>>> On 9/03/2017 1:42 AM, [hidden email] wrote:
>>>>>>>> This doesn't look right.
>>>>>>>>
>>>>>>>> Don't you need to still check for _sampled != NULL?
>>>>>>> That use of _sampled was in dead code that is now removed.
>>>>>>>
>>>>>>>       if (_sample_helper == NULL) return;
>>>>>>> -   if (_sample_helper != NULL) {
>>>>>>>         *(jlong*)_valuep = _sample_helper->take_sample();
>>>>>>> -   }
>>>>>>> -   else if (_sampled != NULL) {
>>>>>>> -     *(jlong*)_valuep = *_sampled;
>>>>>>> -   }
>>>>>>>     }
>>>>>>>
>>>>>>> This change looks fine to me.
>>>>>>>
>>>>>>>> Can you remove the JJJ in the comment?
>>>>>>> I second that :)
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>>>   I don't know what tests were run where Jon (aka JJJ in the code)
>>>>>>>> found this, do you?
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Coleen
>>>>>>>>
>>>>>>>>
>>>>>>>> On 3/8/17 10:21 AM, Shafi Ahmad wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> May I get the review done for this.
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Shafi
>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Shafi Ahmad
>>>>>>>>>> Sent: Wednesday, March 01, 2017 4:27 PM
>>>>>>>>>> To: [hidden email]
>>>>>>>>>> Subject: FW: JDK 10 RFR JDK-8167425: Redundant code in method
>>>>>>>>>> PerfLongVariant::sample
>>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> Summary:
>>>>>>>>>> It's a very small change to a single file.
>>>>>>>>>>
>>>>>>>>>> void PerfLongVariant::sample() {
>>>>>>>>>>
>>>>>>>>>> -  assert(_sample_helper != NULL || _sampled != NULL,
>>>>>>>>>> "unexpected state");
>>>>>>>>>> +  // JJJ - This should not happen.  Maybe the first sample is
>>>>>>>>>> + taken // while the _sample_helper is being null'ed out.
>>>>>>>>>> +  // assert(_sample_helper != NULL || _sampled != NULL,
>>>>>>>>>> + "unexpected state"); if (_sample_helper == NULL) return;
>>>>>>>>>>
>>>>>>>>>>     if (_sample_helper != NULL) {
>>>>>>>>>>       *(jlong*)_valuep = _sample_helper->take_sample();
>>>>>>>>>>     }
>>>>>>>>>>     else if (_sampled != NULL) {
>>>>>>>>>>       *(jlong*)_valuep = *_sampled;
>>>>>>>>>>     }
>>>>>>>>>> }
>>>>>>>>>> Above five lines are modified in changeset
>>>>>>>>>>
>>>>>>>
>>> http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/da91efe96a93#l802.
>>>>>>> 1
>>>>>>>>>> 2 Due to the addition of NULL check and return
>>>>>>>>>>     if (_sample_helper == NULL) return; the else-if block
>>>>>>>>>> becomes redundant and statement 'if (_sample_helper != NULL)' is
>>>>>>>>>> not
>>>>> needed.
>>>>>>>>>> Webrev link:
>>>>>>> http://cr.openjdk.java.net/~shshahma/8167425/webrev.00/
>>>>>>>>>> Jdk10 bug link: https://bugs.openjdk.java.net/browse/JDK-8167425
>>>>>>>>>>
>>>>>>>>>> Testing: jprt.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Shafi
>>>

Loading...