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

[David Holmes]

Hi Shafi,

On 9/03/2017 9:21 PM, Shafi Ahmad wrote:
> Hi All,
>
> Let me know if I have to remove the comment completely.

I don't think the comment makes much sense, so assuming the code is 
logically correct I would remove it. I'd then suggest simplifying the 
method to just:

void PerfLongVariant::sample() {
   if (_sample_helper != NULL) {
     *(jlong*)_valuep = _sample_helper->take_sample();
   }
}

But I have to say that I don't really understand what this code is doing.

David

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