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

[David Holmes]

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