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

[serguei.spitsyn at oracle.com]

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