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: 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], );
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, );
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, _cls, );
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 _name, _sig, );
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, );
121
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: 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: 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: 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: 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, );
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, _cls, );
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 _name, _sig, );
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, );
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, _cls, );
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 _name, _sig, );
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 misleading and might be point to real issue.
webrev:
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
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
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() &&
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(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.
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
