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

[David Holmes]

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