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