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 <shafi.s.ahmad at oracle.com>; David Holmes
> <david.holmes at oracle.com>; Coleen Phillimore
> <coleen.phillimore at oracle.com>; hotspot-dev at openjdk.java.net
> 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 <shafi.s.ahmad at oracle.com>; Coleen Phillimore
> >> <coleen.phillimore at oracle.com>; hotspot-dev at openjdk.java.net
> >> 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 <coleen.phillimore at oracle.com>; hotspot-
> >>>> dev at openjdk.java.net
> >>>> Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method
> >>>> PerfLongVariant::sample
> >>>>
> >>>> Hi Coleen,
> >>>>
> >>>> On 9/03/2017 1:42 AM, coleen.phillimore at oracle.com 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: hotspot-dev at openjdk.java.net
> >>>>>>> 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
>