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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Thu Mar 9 14:09:35 UTC 2017

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 (samplehelper != NULL) { (jlong)valuep = samplehelper->takesample(); } } 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 (samplehelper == 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