Re: RFR(s): 8247248: JVM TI might create JNI locals in another thread when using handshakes.
Hi Dan, fixed, thanks! /Robbin On 2020-06-10 21:59, Daniel D. Daugherty wrote: On 6/10/20 9:57 AM, Robbin Ehn wrote: Hi David and Serguei, (Dan feel free to chime in) Honestly I think I'd like to see things reverted to the use of calling_thread as done for the VMOperation previously. We know it is functionally correct and it should also have the same performance profile. Done: http://cr.openjdk.java.net/~rehn/8247248/v2/webrev/ src/hotspot/share/prims/jvmtiEnvBase.hpp No comments. src/hotspot/share/prims/jvmtiEnvBase.cpp No comments. src/hotspot/share/prims/jvmtiEnv.cpp L1248: JavaThread* calling_thread = JavaThread::current(); L1296: JavaThread* calling_thread = JavaThread::current(); nit - please delete extra space before '='. Thumbs up. I like the switch back to use of calling_thread. Thanks! Dan Passes: hotspot jdi/jvmti testing, running mach5. I'll push tomorrow morning if test is ok and you all are happy (+- nits). (and no objection to break the 24h rule) I started this patch with reverting "8242425: JVMTI monitor operations should use Thread-Local Handshakes". And work my way forward. Thanks, Robbin Thanks, David Thanks, Robbin Thanks, David - Issue: https://bugs.openjdk.java.net/browse/JDK-8247248 Local testing of JDI/JVMTI and t1-5. (no real crash so there is nothing to reproduce) Thanks, Robbin
Re: RFR(s): 8247248: JVM TI might create JNI locals in another thread when using handshakes.
Thanks Yasumasa! /Robbin On 2020-06-11 03:36, Yasumasa Suenaga wrote: Hi Robbin, Thanks for catch up this issue. It looks good to me. Yasumasa On 2020/06/10 22:57, Robbin Ehn wrote: Hi David and Serguei, (Dan feel free to chime in) Honestly I think I'd like to see things reverted to the use of calling_thread as done for the VMOperation previously. We know it is functionally correct and it should also have the same performance profile. Done: http://cr.openjdk.java.net/~rehn/8247248/v2/webrev/ Passes: hotspot jdi/jvmti testing, running mach5. I'll push tomorrow morning if test is ok and you all are happy (+- nits). (and no objection to break the 24h rule) I started this patch with reverting "8242425: JVMTI monitor operations should use Thread-Local Handshakes". And work my way forward. Thanks, Robbin Thanks, David Thanks, Robbin Thanks, David - Issue: https://bugs.openjdk.java.net/browse/JDK-8247248 Local testing of JDI/JVMTI and t1-5. (no real crash so there is nothing to reproduce) Thanks, Robbin
Re: RFR(s): 8247248: JVM TI might create JNI locals in another thread when using handshakes.
Thanks David! /Robbin On 2020-06-11 04:01, David Holmes wrote: Looks good! Thanks for making the change. On a positive note I think this would now show that the conversion from VMop to direct handshake was actually much simpler than might have been thought. :) I'm not sure when the repo fork is happening, so am unclear whether this will head to the right repo in time. :) Thanks, David - On 10/06/2020 11:57 pm, Robbin Ehn wrote: Hi David and Serguei, (Dan feel free to chime in) Honestly I think I'd like to see things reverted to the use of calling_thread as done for the VMOperation previously. We know it is functionally correct and it should also have the same performance profile. Done: http://cr.openjdk.java.net/~rehn/8247248/v2/webrev/ Passes: hotspot jdi/jvmti testing, running mach5. I'll push tomorrow morning if test is ok and you all are happy (+- nits). (and no objection to break the 24h rule) I started this patch with reverting "8242425: JVMTI monitor operations should use Thread-Local Handshakes". And work my way forward. Thanks, Robbin Thanks, David Thanks, Robbin Thanks, David - Issue: https://bugs.openjdk.java.net/browse/JDK-8247248 Local testing of JDI/JVMTI and t1-5. (no real crash so there is nothing to reproduce) Thanks, Robbin
Re: RFR JDK-8232222: Set state to 'linked' when an archived class is restored at runtime
Hi Jiangli, I'm sorry for being that late to the party. I had a problem to follow all the details in this email thread discussion. It is hard to notice race issues from simple webrev reading. So, thanks a lot to Ioi and David for catching it. As I get from the review comments this fix is not mature enough and more work and discussions are necessary. I'll try to better track this discussion in the future. Thanks, Serguei On 6/9/20 05:43, coleen.phillim...@oracle.com wrote: (Posting on the right thread and list now...) On 6/9/20 2:26 AM, David Holmes wrote: Hi Jiangli, > http://cr.openjdk.java.net/~jiangli/823/webrev.03/ I'm having trouble keeping track of all the issues, so let me walk through the changes as I see them: - InstanceKlass::restore_unshareable_info For boot loader classes, when no verification is enabled, we mark the class as linked immediately. By doing this in restore_unshareable_info there are no races (as the class is not exposed to anyone yet) and it allows later checks for is_linked to be by-passed (under the assumption that the class and its supertypes truly are in a state that appears linked). However, this doesn't generate the JVM TI class prepare event, and we can't do it here as that would introduce a number of potential issues with JVM TI. I see in the bug report some metrics from HelloWorld, but really this needs to be backed up by a lot more performance measurements to establish this is actually a worthwhile optimisation. - SystemDictionary::define_instance_class This is where we catch up with the JVM TI requirements and immediately after posting the class load event we post the class prepare event. As we have discussed, this earlier posting of the event is observable to a JVMTI agent and although permitted by the specification it is a change in behaviour that might impact existing agents. Ioi has raised an issue about there being a race here with the potential for the event being delivered multiple times. I agree this code is not adequate: 1718 if (k->is_shared() && k->is_linked()) { You only want to fire the event for exactly those classes that you pre-linked, so at a minimum this has to be restricted to boot classes only. Even then as Ioi points out once the class is exported to the SystemDictionary and visibly seen to be loaded, then other threads may race to link it and so have already posted the class prepare event. In normal linking this race is avoided by the use of the init_lock to check the linked state, do the linking and issue the class prepare event, atomically. But your approach cannot do this as it stands, you would need to add an additional flag to track whether the prepare event had already be issued. Thanks to Ioi and David for seeing this race. As I looked at the change, it looked fairly simple and almost straightforward, but very scary how these changes interact in such surprising ways. Without this careful review, these changes cause endless work later on. The area of class loading and our code for doing so has all sorts of subtle details that are hard to reason about. I wish this weren't so and we can have code that we're not afraid of. The CSR is a nice writeup but I didn't see the race from the CSR either. We need to take the opportunity to look at this from the top down in a project like Leyden. There are still some opportunities to speed up class loading in the context of CDS and finding places that we can simplify, but this was alarmingly not simple. I'm grateful to Ioi and David for doing this work, and yours, for thorougly discussing this change. Thanks, Coleen --- So the change as it stands is incomplete, and introduces a behavioural change to JVM TI, and the benefits of it have not been clearly established. The JBS issue states this is a first step towards pre-initialization and other optimisations, and it is certainly a pre-requisite to pre-link before you can pre-initialize, but I don't think pulling out pre-linking as a separate optimisation is really a worthwhile first step. I have grave reservations about the ability to pre-initialize in general and those issues have to be fleshed out in a project like Leyden. Further, as Coleen points out this pre-linking optimisation is incompatible with proposed vtable changes. Additionally, this seems it will be incompatible with changes proposed in Valhalla, as additional link-time actions will be needed that can't be done at the time of restore_unshareable_info. Bottom line for me is that I just don't think this change is worth pursuing as a stand-alone optimisation at this time. Sorry. Cheers, David - On 5/06/2020 8:14 am, Jiangli Zhou wrote: Hi David, On Wed, Jun 3, 2020 at 9:59 PM David Holmes wrote: Ioi pointed out that my proposal was incomplete and that it would need to be more like: if (is_shared() && JvmtiExport::should_post_class_prepare() && !BytecodeVerificationLoca
RFR: 8246196: javax/management/MBeanServer/OldMBeanServerTest fails with AssertionError
Please review change [1] that fixes an intermittent failure of the test when it is runs with -Xcomp. The problem here is that the timespan the test uses to count notifications is not adjusted for "test.timeout.factor" system property. The original issue is reproducible in JDK 11 and on Solaris platform only. However, I think it makes sense to apply this change in JDK 15 to prevent this from possible happening in the future and then backport it to 11. [1] http://cr.openjdk.java.net/~dtitov/8246196/webrev.01/ [2] https://bugs.openjdk.java.net/browse/JDK-8246196 Thank you, Daniil
RFR: 8242328: Update mentions of ThreadMBean to ThreadMXBean
Hi Could you review following fix which change leftovers of ThreadMBean to ThreadMXBean. In the most cases the comments were updated only. webrev: http://cr.openjdk.java.net/~lmesnik/8242328/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8242328 Leonid
Re: RFR: 8242891: vmTestbase/nsk/jvmti/ test should be fixed to fail early if JVMTI function return error
Agree, it would be better to don't try to use data from functions with error code. The new webrev: http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/ I tried to prevent any usage of possibly corrupted data. Mostly strings or allocated data, sometimes method/class id which are used my other JVMTI functions. Leonid On 6/9/20 6:59 PM, serguei.spit...@oracle.com wrote: On 6/9/20 12:58, Leonid Mesnik wrote: Hi On 6/9/20 12:34 PM, serguei.spit...@oracle.com wrote: Hi Leonid, Thank you for taking care about this! It looks good in general. However, I think, a similar return is needed in more cases. One example: http://cr.openjdk.java.net/~lmesnik/8242891/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/Exception/exception001/exception001.cpp.frames.html 99 err = jvmti_env->GetMethodDeclaringClass(method, &cls); 100 if (err != JVMTI_ERROR_NONE) { 101 printf("(GetMethodDeclaringClass#t) unexpected error: %s (%d)\n", 102TranslateError(err), err); 103 result = STATUS_FAILED; 104 return; 105 } 106 err = jvmti_env->GetClassSignature(cls, &ex.t_cls, &generic); 107 if (err != JVMTI_ERROR_NONE) { 108 printf("(GetClassSignature#t) unexpected error: %s (%d)\n", 109TranslateError(err), err); 110 result = STATUS_FAILED; 111 } 112 err = jvmti_env->GetMethodName(method, 113 &ex.t_name, &ex.t_sig, &generic); 114 if (err != JVMTI_ERROR_NONE) { 115 printf("(GetMethodName#t) unexpected error: %s (%d)\n", 116TranslateError(err), err); 117 result = STATUS_FAILED; 118 } 119 ex.t_loc = location; 120 err = jvmti_env->GetMethodDeclaringClass(catch_method, &cls); 121 if (err != JVMTI_ERROR_NONE) { 122 printf("(GetMethodDeclaringClass#c) unexpected error: %s (%d)\n", 123TranslateError(err), err); 124 result = STATUS_FAILED; 125 return; 126 } 127 err = jvmti_env->GetClassSignature(cls, &ex.c_cls, &generic); 128 if (err != JVMTI_ERROR_NONE) { 129 printf("(GetClassSignature#c) unexpected error: %s (%d)\n", 130TranslateError(err), err); 131 result = STATUS_FAILED; 132 } 133 err = jvmti_env->GetMethodName(catch_method, 134 &ex.c_name, &ex.c_sig, &generic); 135 if (err != JVMTI_ERROR_NONE) { 136 printf("(GetMethodName#c) unexpected error: %s (%d)\n", 137TranslateError(err), err); 138 result = STATUS_FAILED; 139 } In the fragment above you added return for JVMTI GetMethodDeclaringClass error. But GetMethodName and GetClassSignature can be also problematic as the returned names are printed below. It seems to be more safe and even simpler to add returns for such cases as well. Otherwise, the code reader is puzzled why there is a return in one failure case and there is no such return in another. It is a good question if we want to fix such places or even fails with first JVMTI failure. (I even started to fix it in the such way but find that existing tests usually don't fail always). I do not suggest to fix all the tests but those which you are already fixing. The difference is that test tries to reuse "cls" in other JVMTI function and going to generate very misleading crash. How it just tries to compare ex and exs values. So test might crash but clearly outside of JVMTI function and with some useful info. So I am not sure if fixing these lines improve test failure handling. If JVMTI functions fail with an error code the results with symbolic strings must be considered invalid. However, they are used later (the values are printed). It is better to bail out in such cases. It should not be a problem to add similar returns in such cases. Or do you think it is important to continue execution for some reason? Assuming that most of existing tests fails early only if going to re-use possible corrupted data I propose to fix this separately. We need to figure out when to fail or to try to finish. Do you suggest it for the updated tests only or for all the tests with such problems? Thanks, Serguei Leonid Thanks, Serguei On 6/1/20 21:33, Leonid Mesnik wrote: Hi Could you please review following fix which stop test execution if JVMTI function returns error. The test fails anyway however using potentially bad data in JVMTI function might cause misleading crash failures. The hs_err will contains the stacktrace not with problem function but with function called with corrupted data. Most of tests already has such behavior but not all. Also I fixed a couple of tests to finish if they haven't managed to suspend thread. I've updated only tests which try to use corrupted data in JVMTI functions after errors. I haven't updated tests which just compare/print values from erroring JVMTI functions. The crash in strcmp/println is not so misle
Re: RFR: 8242328: Update mentions of ThreadMBean to ThreadMXBean
Hi Leonid, On 12/06/2020 7:09 am, Leonid Mesnik wrote: Hi Could you review following fix which change leftovers of ThreadMBean to ThreadMXBean. In the most cases the comments were updated only. webrev: http://cr.openjdk.java.net/~lmesnik/8242328/webrev.00/ Looks good! I find this whole MBean vs MXBean terminology very confusing. :) Thanks, David bug: https://bugs.openjdk.java.net/browse/JDK-8242328 Leonid
Re: RFR: 8246196: javax/management/MBeanServer/OldMBeanServerTest fails with AssertionError
Hi Daniil, On 12/06/2020 5:56 am, Daniil Titov wrote: Please review change [1] that fixes an intermittent failure of the test when it is runs with -Xcomp. The problem here is that the timespan the test uses to count notifications is not adjusted for "test.timeout.factor" system property. The adjustment looks fine. The original issue is reproducible in JDK 11 and on Solaris platform only. However, I think it makes sense to apply this change in JDK 15 to prevent this from possible happening in the future and then backport it to 11. Do you still intend this for 15 or just 16? If 15 then push to jdk15 repo and it will get forward ported to jdk/jdk automatically. Thanks, David [1] http://cr.openjdk.java.net/~dtitov/8246196/webrev.01/ [2] https://bugs.openjdk.java.net/browse/JDK-8246196 Thank you, Daniil
Re: RFR: 8246196: javax/management/MBeanServer/OldMBeanServerTest fails with AssertionError
+1 --alex On 06/11/2020 16:51, David Holmes wrote: Hi Daniil, On 12/06/2020 5:56 am, Daniil Titov wrote: Please review change [1] that fixes an intermittent failure of the test when it is runs with -Xcomp. The problem here is that the timespan the test uses to count notifications is not adjusted for "test.timeout.factor" system property. The adjustment looks fine. The original issue is reproducible in JDK 11 and on Solaris platform only. However, I think it makes sense to apply this change in JDK 15 to prevent this from possible happening in the future and then backport it to 11. Do you still intend this for 15 or just 16? If 15 then push to jdk15 repo and it will get forward ported to jdk/jdk automatically. Thanks, David [1] http://cr.openjdk.java.net/~dtitov/8246196/webrev.01/ [2] https://bugs.openjdk.java.net/browse/JDK-8246196 Thank you, Daniil
Re: RFR: 8242328: Update mentions of ThreadMBean to ThreadMXBean
LGTM -- Igor > On Jun 11, 2020, at 2:09 PM, Leonid Mesnik wrote: > > Hi > > Could you review following fix which change leftovers of ThreadMBean to > ThreadMXBean. In the most cases the comments were updated only. > > webrev: http://cr.openjdk.java.net/~lmesnik/8242328/webrev.00/ > > bug: https://bugs.openjdk.java.net/browse/JDK-8242328 > > Leonid >
Re: RFR: 8242891: vmTestbase/nsk/jvmti/ test should be fixed to fail early if JVMTI function return error
Hi Leonid, It is much better now. Several places still need the same fix. http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetAllThreads/allthr001/allthr001.cpp.frames.html 211 for (i = 0; i < thrInfo[ind].cnt; i++) { 212 for (j = 0, found = 0; j < threadsCount && !found; j++) { 213 err = jvmti->GetThreadInfo(threads[j], &inf); 214 if (err != JVMTI_ERROR_NONE) { 215 printf("Failed to get thread info: %s (%d)\n", 216TranslateError(err), err); 217 result = STATUS_FAILED; 218 } 219 if (printdump == JNI_TRUE) { 220 printf(" >>> %s", inf.name); 221 } 222 found = (inf.name != NULL && 223 strstr(inf.name, thrInfo[ind].thrNames[i]) == inf.name && 224 (ind == 4 || strlen(inf.name) == 225 strlen(thrInfo[ind].thrNames[i]))); 226 } A return is needed after line 217, otherwise the the inf value is used at lines 222-224. http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetBytecodes/bytecodes003/bytecodes003.cpp.frames.html A return is needed for the errors: 363 result = STATUS_FAILED; 372 result = STATUS_FAILED; 384 result = STATUS_FAILED; http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/MethodEntry/mentry001/mentry001.cpp.frames.html A return is needed for the errors: 82 result = STATUS_FAILED; 94 result = STATUS_FAILED; 100 result = STATUS_FAILED; http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/MethodExit/mexit001/mexit001.cpp.frames.html A return is needed for the error: 98 result = STATUS_FAILED; http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/MethodExit/mexit002/mexit002.cpp.frames.html A return is needed for the error: 98 result = STATUS_FAILED; http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses/redefclass019/redefclass019.cpp.frames.html A return is needed for the error: 186 result = STATUS_FAILED; Also, I do not like many uninitialized locals in these tests. But it is for another pass. Otherwise, it looks good. No need for another webrev if you fix the above. I hope, you will update copyright comments before push. Thanks, Serguei On 6/11/20 15:30, Leonid Mesnik wrote: Agree, it would be better to don't try to use data from functions with error code. The new webrev: http://cr.openjdk.java.net/~lmesnik/8242891/webrev.01/ I tried to prevent any usage of possibly corrupted data. Mostly strings or allocated data, sometimes method/class id which are used my other JVMTI functions. Leonid On 6/9/20 6:59 PM, serguei.spit...@oracle.com wrote: On 6/9/20 12:58, Leonid Mesnik wrote: Hi On 6/9/20 12:34 PM, serguei.spit...@oracle.com wrote: Hi Leonid, Thank you for taking care about this! It looks good in general. However, I think, a similar return is needed in more cases. One example: http://cr.openjdk.java.net/~lmesnik/8242891/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/Exception/exception001/exception001.cpp.frames.html 99 err = jvmti_env->GetMethodDeclaringClass(method, &cls); 100 if (err != JVMTI_ERROR_NONE) { 101 printf("(GetMethodDeclaringClass#t) unexpected error: %s (%d)\n", 102TranslateError(err), err); 103 result = STATUS_FAILED; 104 return; 105 } 106 err = jvmti_env->GetClassSignature(cls, &ex.t_cls, &generic); 107 if (err != JVMTI_ERROR_NONE) { 108 printf("(GetClassSignature#t) unexpected error: %s (%d)\n", 109TranslateError(err), err); 110 result = STATUS_FAILED; 111 } 112 err = jvmti_env->GetMethodName(method, 113 &ex.t_name, &ex.t_sig, &generic); 114 if (err != JVMTI_ERROR_NONE) { 115 printf("(GetMethodName#t) unexpected error: %s (%d)\n", 116TranslateError(err), err); 117 result = STATUS_FAILED; 118 } 119 ex.t_loc = location; 120 err = jvmti_env->GetMeth
Re: RFR: 8246196: javax/management/MBeanServer/OldMBeanServerTest fails with AssertionError
+1 Thanks, Serguei On 6/11/20 17:28, Alex Menkov wrote: +1 --alex On 06/11/2020 16:51, David Holmes wrote: Hi Daniil, On 12/06/2020 5:56 am, Daniil Titov wrote: Please review change [1] that fixes an intermittent failure of the test when it is runs with -Xcomp. The problem here is that the timespan the test uses to count notifications is not adjusted for "test.timeout.factor" system property. The adjustment looks fine. The original issue is reproducible in JDK 11 and on Solaris platform only. However, I think it makes sense to apply this change in JDK 15 to prevent this from possible happening in the future and then backport it to 11. Do you still intend this for 15 or just 16? If 15 then push to jdk15 repo and it will get forward ported to jdk/jdk automatically. Thanks, David [1] http://cr.openjdk.java.net/~dtitov/8246196/webrev.01/ [2] https://bugs.openjdk.java.net/browse/JDK-8246196 Thank you, Daniil
Re: RFR: 8242328: Update mentions of ThreadMBean to ThreadMXBean
Hi Leonid, It looks okay to me. > I find this whole MBean vs MXBean terminology very confusing. :) Me too. :) For instance, I see some references to MBeanServer (should they also be replaced with MXBeanServer?): http://cr.openjdk.java.net/~lmesnik/8242328/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/monitoring/CompilationMXBean/comptimemon002/TestDescription.java.udiff.html * The test checks that - * CompilationMBean.isCompilationTimeMonitoringSupported() + * CompilationMXBean.isCompilationTimeMonitoringSupported() * method returns true. The test performs access to management metrics * through default MBeanServer. http://cr.openjdk.java.net/~lmesnik/8242328/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/monitoring/CompilationMXBean/comptimemon003/TestDescription.java.udiff.html * The test checks that - * CompilationMBean.isCompilationTimeMonitoringSupported() + * CompilationMXBean.isCompilationTimeMonitoringSupported() * method returns true. The test performs access to management metrics * through custom MBeanServer (developed and saved in http://cr.openjdk.java.net/~lmesnik/8242328/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/monitoring/CompilationMXBean/comptimemon004/TestDescription.java.udiff.html * DESCRIPTION * The test checks that - * CompilationMBean.isCompilationTimeMonitoringSupported() + * CompilationMXBean.isCompilationTimeMonitoringSupported() * method returns true. The test performs access to management metrics * through default MBeanServer proxy. http://cr.openjdk.java.net/~lmesnik/8242328/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/monitoring/CompilationMXBean/comptimemon005/TestDescription.java.udiff.html * The test checks that - * CompilationMBean.isCompilationTimeMonitoringSupported() + * CompilationMXBean.isCompilationTimeMonitoringSupported() * method returns true. The test performs access to management metrics * through custom MBeanServer proxy (developed and saved in Thanks, Serguei On 6/11/20 16:48, David Holmes wrote: Hi Leonid, On 12/06/2020 7:09 am, Leonid Mesnik wrote: Hi Could you review following fix which change leftovers of ThreadMBean to ThreadMXBean. In the most cases the comments were updated only. webrev: http://cr.openjdk.java.net/~lmesnik/8242328/webrev.00/ Looks good! I find this whole MBean vs MXBean terminology very confusing. :) Thanks, David bug: https://bugs.openjdk.java.net/browse/JDK-8242328 Leonid