JDK 10 RFR JDK-8167425: Redundant code in method PerfLongVariant::sample (original) (raw)

Shafi Ahmad shafi.s.ahmad at oracle.com
Thu Mar 9 05:30:51 UTC 2017

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 samplehelper is being null'ed out. 219 // assert(samplehelper != NULL || sampled != NULL, "unexpected state"); 220 if (samplehelper == NULL) return; 221 222 (jlong)valuep = samplehelper->takesample(); 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 (samplehelper == NULL) return; >> - if (samplehelper != NULL) { >> (jlong)valuep = samplehelper->takesample(); >> - } >> - 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(samplehelper != NULL || sampled != NULL, "unexpected >>>>> state"); >>>>> + // JJJ - This should not happen. Maybe the first sample is >>>>> + taken // while the samplehelper is being null'ed out. >>>>> + // assert(samplehelper != NULL || sampled != NULL, >>>>> + "unexpected state"); if (samplehelper == NULL) return; >>>>> >>>>> if (samplehelper != NULL) { >>>>> (jlong)valuep = samplehelper->takesample(); >>>>> } >>>>> 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 (samplehelper == NULL) return; the else-if block becomes >>>>> redundant and statement 'if (samplehelper != 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 >>>

More information about the hotspot-dev mailing list