Re: PING: Re: RFR (XS): 8244571: assert(!_thread->is_pending_jni_exception_check()) failed: Pending JNI Exception Check during class loading
Hi David,
Sorry for pushing it before your final approval.
I thought you were okay with the update as it was implementation of your
suggestion.
Thanks,
Serguei
On 5/23/20 05:49, David Holmes wrote:
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
RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath
Please, review a fix for: https://bugs.openjdk.java.net/browse/JDK-8234882 CSR draft (one CSR reviewer is needed before finalizing it): https://bugs.openjdk.java.net/browse/JDK-8245853 Webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/src/ Updated JVM TI StopThread spec: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread Summary: The JVM TI StopThread method mirrored the functionality of the java.lang.Thread::stop(Throwable t) method, in that it allows any exception type to be installed as an asynchronous exception in the target thread. However, the java.lang.Thread::stop(Throwable t) method was inherently unsafe and in Java 8 (under JDK-7059085) it was "retired" so that it always threw UnsupportedOperationException. The updated JVM TI StopThread spec disallows an arbitrary Throwable from being passed, and instead restricts the argument to being an instance of ThreadDeath, thus mirroring the (deprecated but still functional) java.lang.Thread::stop() method. The error JVMTI_ERROR_INVALID_OBJECT is returned if the exception argument is not an instance of ThreadDeath. Also, I will file similar RFE and CSR on the JDI and JDWP spec. Testing: Built docs and checked the doc has been generated as expected. Will run the nsk.jvmti tests locally. Will submit hs-tiers1-3 to make sure there are no regressions in the JVM TI and JDI tests. Thanks, Serguei
Re: RFR(S): 8244668: Remove SA's javascript support
Looks good to me. -Sundar On 27/05/20 5:01 am, Chris Plummer wrote: Hello, Please review the following changes to fully remove javascript support from SA. It's been non-functional since JDK 9 and there are no plans to support it anymore. https://bugs.openjdk.java.net/browse/JDK-8244668 http://cr.openjdk.java.net/~cjplummer/8244668/webrev.00/index.html If anyone thinks a CSR is needed for this, please let me know. There's no spec involved. The support was always pretty ad hoc, with some supporting documentation in the source tree in jsdb.html [1], but which doesn't appear itself to have ever been released. There was one menu item in the hsdb GUI that used javascript (and as a result caused an exception). I was having a little trouble deciphering what it actually does, but it appears to allow you to write some javascript to produce (and filter) a list of objects, and then display the list of objects in a window where you could inspect them further. Since there is no way to do something similar in java without allowing the user to provide hsdb extensions in the form of .class files (or support writing and compiling java classes from within hsdb), I just removed the hsdb feature by removing the menu item and supporting code. thanks, Chris [1] https://hg.openjdk.java.net/jdk/jdk/raw-file/a7e42c260029/src/jdk.hotspot.agent/doc/jsdb.html
Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
Hi Daniil,
Looks good.
thanks,
Chris
On 5/26/20 10:46 AM, Daniil Titov wrote:
Hi Chris and David,
Please review a new version of the fix [1] with the changes Chris suggested.
[1] Webrev: http://cr.openjdk.java.net/~dtitov/8244993/webrev.02
[2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8244993
Thank you,
Daniil
On 5/22/20, 11: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. 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
>
>
RFR(S): 8244668: Remove SA's javascript support
Hello, Please review the following changes to fully remove javascript support from SA. It's been non-functional since JDK 9 and there are no plans to support it anymore. https://bugs.openjdk.java.net/browse/JDK-8244668 http://cr.openjdk.java.net/~cjplummer/8244668/webrev.00/index.html If anyone thinks a CSR is needed for this, please let me know. There's no spec involved. The support was always pretty ad hoc, with some supporting documentation in the source tree in jsdb.html [1], but which doesn't appear itself to have ever been released. There was one menu item in the hsdb GUI that used javascript (and as a result caused an exception). I was having a little trouble deciphering what it actually does, but it appears to allow you to write some javascript to produce (and filter) a list of objects, and then display the list of objects in a window where you could inspect them further. Since there is no way to do something similar in java without allowing the user to provide hsdb extensions in the form of .class files (or support writing and compiling java classes from within hsdb), I just removed the hsdb feature by removing the menu item and supporting code. thanks, Chris [1] https://hg.openjdk.java.net/jdk/jdk/raw-file/a7e42c260029/src/jdk.hotspot.agent/doc/jsdb.html
RFR(T): 8244622: Remove SA's memory/FreeChunk.java. It's no longer used.
Hello, Please review the following trivial change to remove FreeChunk.java: https://bugs.openjdk.java.net/browse/JDK-8244622 http://cr.openjdk.java.net/~cjplummer/8244622/webrev.00/index.html Tested with tier1 and also running all SA tests. thanks, Chris
Re: RFR: JDK-8244703: "platform encoding not initialized" exceptions with debugger, JNI
updated webrev (with fixing utf8 buffer size in transport.c): http://cr.openjdk.java.net/~amenkov/jdk15/jdwp_javalib_dep/webrev.2/ On 05/23/2020 01:54, Alan Bateman wrote: 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. Not a problem, I think it would be good to fix this right now. --alex -Alan
Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
Hi Chris,
I like the pattern and use it quite often.
And the main reason not to avoid repetition of the object, but to avoid
stupid copy-paste errors like
MyObject obj1 = new MyObject();
obj1.method1();
obj1.method2();
MyObject obj2 = new MyObject();
obj2.method1();
obj1.method2(); <== I fixed a lot of such error
With this pattern you just do
MyObject obj1 = new MyObject()
.method1()
.method2();
MyObject obj2 = new MyObject()
.method1()
.method2();
Just my 2 cents..
--alex
On 05/23/2020 10:06, Chris Plummer wrote:
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(XS): 8245521: Remove STACK_BIAS
David/Matthias/Vladimir, thanks for the reviews! Change pushed. Cheers, Mikael > On May 26, 2020, at 12:01 PM, Vladimir Kozlov > wrote: > > On 5/21/20 9:11 PM, David Holmes wrote: >> Hi Mikael, >> Looks good. > > +1 > >> I assume the change to GraalHotSpotVMConfig.java is to allow it to work with >> older VMs? > > Yes. stackBias will be set to 0 if STACK_BIAS is not present. Otherwise it > will be set to STACK_BIAS value. > > Thanks, > Vladimir > >> Thanks, >> David >> On 22/05/2020 1:36 pm, Mikael Vidstedt wrote: >>> >>> Please review this small change which removes the STACK_BIAS constant and >>> its uses: >>> >>> JBS: https://bugs.openjdk.java.net/browse/JDK-8245521 >>> webrev: >>> http://cr.openjdk.java.net/~mikael/webrevs/8245521/webrev.00/open/webrev/ >>> >>> Background (from JBS): >>> >>> With Solaris/SPARC removed the STACK_BIAS definition in >>> src/hotspot/share/utilities/globalDefinitions.hpp is now always 0 and can >>> be removed. >>> >>> >>> Testing: >>> >>> Tier1 >>> >>> Cheers, >>> Mikael >>>
Re: RFR(XS): 8245521: Remove STACK_BIAS
On 5/21/20 9:11 PM, David Holmes wrote: Hi Mikael, Looks good. +1 I assume the change to GraalHotSpotVMConfig.java is to allow it to work with older VMs? Yes. stackBias will be set to 0 if STACK_BIAS is not present. Otherwise it will be set to STACK_BIAS value. Thanks, Vladimir Thanks, David On 22/05/2020 1:36 pm, Mikael Vidstedt wrote: Please review this small change which removes the STACK_BIAS constant and its uses: JBS: https://bugs.openjdk.java.net/browse/JDK-8245521 webrev: http://cr.openjdk.java.net/~mikael/webrevs/8245521/webrev.00/open/webrev/ Background (from JBS): With Solaris/SPARC removed the STACK_BIAS definition in src/hotspot/share/utilities/globalDefinitions.hpp is now always 0 and can be removed. Testing: Tier1 Cheers, Mikael
Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
Hi Chris and David,
Please review a new version of the fix [1] with the changes Chris suggested.
[1] Webrev: http://cr.openjdk.java.net/~dtitov/8244993/webrev.02
[2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8244993
Thank you,
Daniil
On 5/22/20, 11: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. 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(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant
Hi Vladimir, > > Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ > Not an expert in JVMTI code base, so can't comment on the actual changes. > From JIT-compilers perspective it looks good. I put out webrev.1 a while ago [1]: Webrev:http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1/ Webrev(delta): http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1.inc/ You originally suggested to use a handshake to switch a thread into interpreter mode [2]. I'm using a direct handshake now, because I think it is the best fit. May I ask if webrev.1 still looks good to you from JIT-compilers perspective? Can I list you as (partial) Reviewer? Thanks, Richard. [1] http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-April/031245.html [2] http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-January/030340.html -Original Message- From: Vladimir Ivanov Sent: Freitag, 7. Februar 2020 09:19 To: Reingruber, Richard ; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net Subject: Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant > Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ Not an expert in JVMTI code base, so can't comment on the actual changes. From JIT-compilers perspective it looks good. Best regards, Vladimir Ivanov > Bug:https://bugs.openjdk.java.net/browse/JDK-8238585 > > The change avoids making all compiled methods on stack not_entrant when > switching a java thread to > interpreter only execution for jvmti purposes. It is sufficient to deoptimize > the compiled frames on stack. > > Additionally a handshake is used instead of a vm operation to walk the stack > and do the deoptimizations. > > Testing: JCK and JTREG tests, also in Xcomp mode with fastdebug and release > builds on all platforms. > > Thanks, Richard. > > See also my question if anyone knows a reason for making the compiled methods > not_entrant: > http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-January/030339.html >
Re: RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
On 5/25/20 23:39,
serguei.spit...@oracle.com wrote:
Please, review a fix for:
https://bugs.openjdk.java.net/browse/JDK-8245126
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.1/
Summary:
The Kitchensink stress test with the Instrumentation module
enabled does
a lot of class retransformations in parallel with all other
stressing.
It provokes the assert at the compiled code installation time:
assert(!method->is_old()) failed: Should not be installing
old methods
The problem is that the CompileBroker::invoke_compiler_on_method
in C2 version
(non-JVMCI tiered compilation) is missing the check that exists
in the JVMCI
part of implementation:
2148 // Skip redefined methods
2149 if (target_handle->is_old()) {
2150 failure_reason = "redefined method";
2151 retry_message = "not retryable";
2152 compilable = ciEnv::MethodCompilable_never;
2153 } else {
. . .
2168 }
The fix is to add this check.
Sorry, forgot to explain one thing.
Compiler code has a special mechanism to ensure the JVMTI class
redefinition did
not happen while the method was compiled, so all the assumptions
remain correct.
2190 // Cache Jvmti state
2191 ci_env.cache_jvmti_state();
Part of this is a check that the value of
JvmtiExport::redefinition_count() is
cached in ciEnv variable: _jvmti_redefinition_count.
The JvmtiExport::redefinition_count() value change means a class
redefinition
happened which also implies some of methods may become old.
However, the method being compiled can be already old at the point
where the
redefinition counter is cached, so the redefinition counter check
does not help much.
Thanks,
Serguei
Testing:
Ran Kitchensink test with the Instrumentation module enabled in mach5
multiple times for 100 times. Without the fix the test normally fails
a couple of times in 200 runs. It does not fail with the fix anymore.
Will also submit hs tiers1-5.
Thanks,
Serguei
RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
Please, review a fix for:
https://bugs.openjdk.java.net/browse/JDK-8245126
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.1/
Summary:
The Kitchensink stress test with the Instrumentation module
enabled does
a lot of class retransformations in parallel with all other
stressing.
It provokes the assert at the compiled code installation time:
assert(!method->is_old()) failed: Should not be installing
old methods
The problem is that the
CompileBroker::invoke_compiler_on_method in C2 version
(non-JVMCI tiered compilation) is missing the
check that exists in the JVMCI
part of implementation:
2148 // Skip redefined methods
2149 if (target_handle->is_old()) {
2150 failure_reason = "redefined method";
2151 retry_message = "not retryable";
2152 compilable = ciEnv::MethodCompilable_never;
2153 } else {
. . .
2168 }
The fix is to add this check.
Testing:
Ran Kitchensink test with the Instrumentation module enabled in mach5
multiple times for 100 times. Without the fix the test normally fails
a couple of times in 200 runs. It does not fail with the fix anymore.
Will also submit hs tiers1-5.
Thanks,
Serguei
