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

[coleen.phillimore at oracle.com]

On 3/9/17 6:54 AM, David Holmes wrote:
> 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.

I don't either.   I think the version without comments is probably the 
least confusing and looks correct.  If you change it to this, I don't 
need to see a webrev again.

thanks,
Coleen

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