Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
On 5/23/20 6:03 AM, David Holmes wrote: Hi Chris, On 23/05/2020 4:50 am, Chris Plummer wrote: Hi Daniil, There is one reference to "jvmwarningmsg" that occurs before it is declared while all the rest all come after. It probably would make sense to move its declaration up near the top of the file. 92 private static void matchListedProcesses(OutputAnalyzer output) { 93 output.stdoutShouldMatchByLine(JCMD_LIST_REGEX) 94 .stderrShouldBeEmptyIgnoreVMWarnings(); 95 } We probably use this coding pattern all over the place, but I think it just leads to less readable code. Any reason not to use: 92 private static void matchListedProcesses(OutputAnalyzer output) { 93 output.stdoutShouldMatchByLine(JCMD_LIST_REGEX); 94 output.stderrShouldBeEmptyIgnoreVMWarnings(); 95 } I just don't see the point of the chaining, and don't understand why all these OutputAnalyzer methods return the "this" object in the first place. Maybe I'm missing something. They return "this" precisely so that you can chain. The API was designed for a style that allows: output.shouldContain(x).shouldNotContain(y).shouldContain(z) ... to avoid the repetition of "output". Yeah, I get that, but I never did like this pattern. I just don't find it as readable. For one, there's no conveyance of the method return type, not just because of the chaining, but also because the method name does not imply a return type. Chaining like getMethod().getClass().getName() is fine, because there are implied return types in the method names, and they clearly are being called for the purpose of returning a type. But when the return type is there solely for the purpose of chaining, it's not as obvious what is going on. Your example is easier to read because the method names are short, readily identified as related, and you made them all fit on one line with shortened arguments. That's not the case with Daniil's code. I just don't see the argument for saying that: 93 output.stdoutShouldMatchByLine(JCMD_LIST_REGEX) 94 .stderrShouldBeEmptyIgnoreVMWarnings(); Is somehow better than: 93 output.stdoutShouldMatchByLine(JCMD_LIST_REGEX); 94 output.stderrShouldBeEmptyIgnoreVMWarnings(); I don't have to look twice at the second version (or be familiar with the APIs being used) to know what's going on. Chris David - I guess maybe there are cases where the OutputAnalyzer object is not already in a local variable, adding some value to the chaining, but that's not the case here, and I think if it were the case it would be more readable just to stick the OutputAnalyzer object in a local. There one other case of this: 154 private static void matchPerfCounters(OutputAnalyzer output) { 155 output.stdoutShouldMatchByLine(PERF_COUNTER_REGEX,null, 156 PERF_COUNTER_REGEX) 157 .stderrShouldBeEmptyIgnoreVMWarnings(); 158 } I think you can also add spaces after the commas, and probably make the first stdoutShouldMatchByLine() one line. thanks, Chris On 5/21/20 10:06 PM, Daniil Titov wrote: Please review a webrev [1] that reverts the changes done in jdk.test.lib.process.OutputAnalyzer in [3]. Change [3] modified OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() methods to ignore also VM version strings . The current webrev [1] reverts this change and instead makes the tests that expects an empty stderr from launched j-* tools to filter out '-showversion' from test options when forwarding test VM option to j*-tools. Testing: Mach5 tier1-tier5 tests passed successfully. Tier6-tier7 tests are in progress. [1] Webrev: http://cr.openjdk.java.net/~dtitov/8244993/webrev.01 [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8244993 [3] https://bugs.openjdk.java.net/browse/JDK-8242009 Thank you, Daniil
Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
Hi Chris, On 23/05/2020 4:50 am, Chris Plummer wrote: Hi Daniil, There is one reference to "jvmwarningmsg" that occurs before it is declared while all the rest all come after. It probably would make sense to move its declaration up near the top of the file. 92 private static void matchListedProcesses(OutputAnalyzer output) { 93 output.stdoutShouldMatchByLine(JCMD_LIST_REGEX) 94 .stderrShouldBeEmptyIgnoreVMWarnings(); 95 } We probably use this coding pattern all over the place, but I think it just leads to less readable code. Any reason not to use: 92 private static void matchListedProcesses(OutputAnalyzer output) { 93 output.stdoutShouldMatchByLine(JCMD_LIST_REGEX); 94 output.stderrShouldBeEmptyIgnoreVMWarnings(); 95 } I just don't see the point of the chaining, and don't understand why all these OutputAnalyzer methods return the "this" object in the first place. Maybe I'm missing something. They return "this" precisely so that you can chain. The API was designed for a style that allows: output.shouldContain(x).shouldNotContain(y).shouldContain(z) ... to avoid the repetition of "output". David - I guess maybe there are cases where the OutputAnalyzer object is not already in a local variable, adding some value to the chaining, but that's not the case here, and I think if it were the case it would be more readable just to stick the OutputAnalyzer object in a local. There one other case of this: 154 private static void matchPerfCounters(OutputAnalyzer output) { 155 output.stdoutShouldMatchByLine(PERF_COUNTER_REGEX,null, 156 PERF_COUNTER_REGEX) 157 .stderrShouldBeEmptyIgnoreVMWarnings(); 158 } I think you can also add spaces after the commas, and probably make the first stdoutShouldMatchByLine() one line. thanks, Chris On 5/21/20 10:06 PM, Daniil Titov wrote: Please review a webrev [1] that reverts the changes done in jdk.test.lib.process.OutputAnalyzer in [3]. Change [3] modified OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() methods to ignore also VM version strings . The current webrev [1] reverts this change and instead makes the tests that expects an empty stderr from launched j-* tools to filter out '-showversion' from test options when forwarding test VM option to j*-tools. Testing: Mach5 tier1-tier5 tests passed successfully. Tier6-tier7 tests are in progress. [1] Webrev: http://cr.openjdk.java.net/~dtitov/8244993/webrev.01 [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8244993 [3] https://bugs.openjdk.java.net/browse/JDK-8242009 Thank you, Daniil
Re: PING: Re: RFR (XS): 8244571: assert(!_thread->is_pending_jni_exception_check()) failed: Pending JNI Exception Check during class loading
Update looks fine - though I see you already pushed it. David On 22/05/2020 7:32 pm, serguei.spit...@oracle.com wrote: Hi David, The updated webrev is with your comments addressed: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.2/ Thanks, Serguei On 5/22/20 00:43, serguei.spit...@oracle.com wrote: Hi David, Thank you for the comments! On 5/21/20 23:58, David Holmes wrote: Hi Serguei, On 22/05/2020 4:17 pm, serguei.spit...@oracle.com wrote: PING: This is pretty small and easy to review fix. Thanks! Serguei On 5/19/20 09:28, serguei.spit...@oracle.com wrote: Please, review fix for: https://bugs.openjdk.java.net/browse/JDK-8244571 Webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.1/ Summary: There are two places in the native part of test that cause assert and WARNING with the -Xcheck:jni. The assert is because there is no check for pending exception after the call to: jni->CallBooleanMethod(klass, is_hid_mid); Using a JNI ExceptionCheck()after the call fixes the issue. bool res = jni->CallBooleanMethod(klass, is_hid_mid); if (jni->ExceptionCheck()) { LOG0("is_hidden: Exception in jni CallBooleanMethod\n"); } return res; That will fix the pending_jni_exception_check error, but if an exception actually occurs what will be returned? And whatever is returned, the callers of this method don't themselves check for pending exceptions so they will treat it as if the exception didn't occur - at least until we finally return to Java code. Perhaps any exception should result in jni->FatalError as happens with any JVMTI error? You are right, it would be more clean to call jni->FatalError. I was also thinking about it but also worried to get the exception details. The exception can be printed before call to FatalError. The following call to the JVM TI function: err = jvmti->GetClassLoaderClasses(loader, , _classes); produces the warning (with a java level stack trace): WARNING: JNI local refs: 94, exceeds capacity: 32 It is because the GetClassLoaderClasses returns an array of local references to the loader classes. Using a JNI EnsureLocalCapacity() before the JVM TI call also fixes the issue. The warning suggests using 1024 is a bit of overkill. :) What capacity would be more reasonable, 256 or 512? Let's pick 256. This is just a warning, the test is still passing. Thanks! Serguei Cheers, David Testing: Running the test test/hotspot/jtreg/serviceability/jvmti/HiddenClass locally. Will run a mach5 job as well. Thanks, Serguei
Re: RFR: JDK-8244703: "platform encoding not initialized" exceptions with debugger, JNI
On 23/05/2020 01:15, Alex Menkov wrote: size of utf8 string does not depend on sizeof(int). Per RFC each symbol can be encoded by 1..4 byte(s). Maybe Alan can explain this len+len/2+2 value. I don't know without digging into the history. My only reason for pointing it out is that it looked curious as I would have expected len*4 + 1. The lack of parentheses will also force every reader to stop and remind themselves of the precedence rules. I don't want to hold up JDK-8244703, I was spotted it when checking for other usages of utf8FromPlatform. -Alan