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

[serguei.spitsyn at oracle.com]

+1

Thanks,
Serguei


On 3/10/17 03:56, David Holmes wrote:
> 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
>>>