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