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