Re: RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
Hi Serguei Thanks for fixing this. I don't have official reviewer status but the changes look good to me. As we've already discussed, this does not fix JDK-8245128, unfortunately. Best regards, Christian On 04.06.20 01:05, serguei.spit...@oracle.com wrote: Hi Dean, Thank you a lot for the review! I hope, Christian will have a chance to look at it. Thanks, Serguei On 6/3/20 14:56, Dean Long wrote: Hi Serguei, I like the latest changes so that JVMCI matches C2. Please get another review because this is not a trivial change. dl On 6/3/20 10:06 AM, serguei.spit...@oracle.com wrote: Hi Dean, The updated webrev is: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.3/ Probably, the JVMCI part can be simplified. Only the compile_state line has to be moved up: + JVMCICompileState compile_state(task); // Skip redefined methods - if (target_handle->is_old()) { + if (compile_state.target_method_is_old()) { failure_reason = "redefined method"; retry_message = "not retryable"; compilable = ciEnv::MethodCompilable_never; } else { - JVMCICompileState compile_state(task); Fixes in the jvmciEnv.?pp are not really needed Please, let me know what do you think. This version does not fail at all (in 300 runs for both C2 and JVMCI). It seems, other two issues disappeared as well: This was seen with the C2: https://bugs.openjdk.java.net/browse/JDK-8245128 This was seen with the JVMCI: https://bugs.openjdk.java.net/browse/JDK-8245446 Thanks, Serguei On 6/1/20 23:40, serguei.spit...@oracle.com wrote: Hi Dean, Thank you for the reply. The problem is I do not fully understand your suggestion, especially the part about caching the method,is_old() value in the cache_jvmti_state(). This is a preliminary webrev where I tried to implement your suggestion: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.2/ This variant is failing in half of test runs for both C1/C2 and JVMCI. I think, the root cause is a safepoint in a ThreadInVMfromNative desctructor. Here: 232 void ciEnv::cache_jvmti_state() { 233 VM_ENTRY_MARK; Then we check for the target_method_is_old() value which is not up-to-date any more. I feel, it was correct and more simple before introducing this approach. Probably, I'm missing something here. I also have a question about the update fragment: 1696 { 1697 // Must switch to native to allocate ci_env 1698 ThreadToNativeFromVM ttn(thread); 1699 ciEnv ci_env((CompileTask*)NULL); 1700 1701 // Switch back to VM state to do compiler initialization 1702 ThreadInVMfromNative tv(thread); 1703 ResetNoHandleMark rnhm; 1704 1705 // Perform per-thread and global initializations 1706 comp->initialize(); 1707 } Can we remove the ciEnv object initialization above with the state transitions? Or it has some side effects? Please, let me know what you think. Thanks, Serguei On 6/1/20 15:10, Dean Long wrote: On 5/31/20 11:16 PM, serguei.spit...@oracle.com wrote: Hi Dean, To check the is_old as you suggest the target method has to be passed to the cache_jvmti_state() as argument. Is it what you are suggesting? I believe you can use use _task->method()->is_old(), as the ciEnv already has the task. Just want to make sure I understand you correctly. The cache_jvmti_state() and cache_dtrace_flags() are called in the CompileBroker::init_compiler_runtime() for a ciEnv with the NULL CompileTask which looks unnecessary (or I don't understand it): bool CompileBroker::init_compiler_runtime() { CompilerThread* thread = CompilerThread::current(); . . . ciEnv ci_env((CompileTask*)NULL); // Cache Jvmti state ci_env.cache_jvmti_state(); // Cache DTrace flags ci_env.cache_dtrace_flags(); These calls look unnecessary to me, as the ci_env will cache these again before compiling a method. I suggest removing these calls. We should make sure the cache fields are initialized to sane values in the ciEnv ctor. The JVMCI has a separate implementation for ciEnv which is jvmciEnv and its own set of cache_jvmti_state() and jvmti_state_changed() functions. Both are not called in the JVMCI case. So, these checks look as broken in JVMCI now. JVMCI is in better shape, because it doesn't transition out of _thread_in_vm state, but yes it needs similar changes. Not sure, I have enough compiler knowledge to fix this at this stage of release. Would it better to file a separate hotspot/compiler RFE targeted to 16? It can be assigned to me if it helps. This is a P3 so I believe we have time to fix it for 15. Please go ahead and let's see if we can get it in. I can help with the JVMCI changes if they are not straightforward. dl Thanks, Serguei On 5/28/20 10:54, Dean Long wrote: Sure, you could just have cache_jvmti_state() return a boolean to bail out immediately for is_old. dl On 5/28/20 7:23 AM, serguei.spit...@oracle.com wrote: Hi Dean, Thank you for looking at
Re: RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
Thank you a lot for review, Christian! Serguei On 6/8/20 00:34, Christian Hagedorn wrote: Hi Serguei Thanks for fixing this. I don't have official reviewer status but the changes look good to me. As we've already discussed, this does not fix JDK-8245128, unfortunately. Best regards, Christian On 04.06.20 01:05, serguei.spit...@oracle.com wrote: Hi Dean, Thank you a lot for the review! I hope, Christian will have a chance to look at it. Thanks, Serguei On 6/3/20 14:56, Dean Long wrote: Hi Serguei, I like the latest changes so that JVMCI matches C2. Please get another review because this is not a trivial change. dl On 6/3/20 10:06 AM, serguei.spit...@oracle.com wrote: Hi Dean, The updated webrev is: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.3/ Probably, the JVMCI part can be simplified. Only the compile_state line has to be moved up: + JVMCICompileState compile_state(task); // Skip redefined methods - if (target_handle->is_old()) { + if (compile_state.target_method_is_old()) { failure_reason = "redefined method"; retry_message = "not retryable"; compilable = ciEnv::MethodCompilable_never; } else { - JVMCICompileState compile_state(task); Fixes in the jvmciEnv.?pp are not really needed Please, let me know what do you think. This version does not fail at all (in 300 runs for both C2 and JVMCI). It seems, other two issues disappeared as well: This was seen with the C2: https://bugs.openjdk.java.net/browse/JDK-8245128 This was seen with the JVMCI: https://bugs.openjdk.java.net/browse/JDK-8245446 Thanks, Serguei On 6/1/20 23:40, serguei.spit...@oracle.com wrote: Hi Dean, Thank you for the reply. The problem is I do not fully understand your suggestion, especially the part about caching the method,is_old() value in the cache_jvmti_state(). This is a preliminary webrev where I tried to implement your suggestion: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.2/ This variant is failing in half of test runs for both C1/C2 and JVMCI. I think, the root cause is a safepoint in a ThreadInVMfromNative desctructor. Here: 232 void ciEnv::cache_jvmti_state() { 233 VM_ENTRY_MARK; Then we check for the target_method_is_old() value which is not up-to-date any more. I feel, it was correct and more simple before introducing this approach. Probably, I'm missing something here. I also have a question about the update fragment: 1696 { 1697 // Must switch to native to allocate ci_env 1698 ThreadToNativeFromVM ttn(thread); 1699 ciEnv ci_env((CompileTask*)NULL); 1700 1701 // Switch back to VM state to do compiler initialization 1702 ThreadInVMfromNative tv(thread); 1703 ResetNoHandleMark rnhm; 1704 1705 // Perform per-thread and global initializations 1706 comp->initialize(); 1707 } Can we remove the ciEnv object initialization above with the state transitions? Or it has some side effects? Please, let me know what you think. Thanks, Serguei On 6/1/20 15:10, Dean Long wrote: On 5/31/20 11:16 PM, serguei.spit...@oracle.com wrote: Hi Dean, To check the is_old as you suggest the target method has to be passed to the cache_jvmti_state() as argument. Is it what you are suggesting? I believe you can use use _task->method()->is_old(), as the ciEnv already has the task. Just want to make sure I understand you correctly. The cache_jvmti_state() and cache_dtrace_flags() are called in the CompileBroker::init_compiler_runtime() for a ciEnv with the NULL CompileTask which looks unnecessary (or I don't understand it): bool CompileBroker::init_compiler_runtime() { CompilerThread* thread = CompilerThread::current(); . . . ciEnv ci_env((CompileTask*)NULL); // Cache Jvmti state ci_env.cache_jvmti_state(); // Cache DTrace flags ci_env.cache_dtrace_flags(); These calls look unnecessary to me, as the ci_env will cache these again before compiling a method. I suggest removing these calls. We should make sure the cache fields are initialized to sane values in the ciEnv ctor. The JVMCI has a separate implementation for ciEnv which is jvmciEnv and its own set of cache_jvmti_state() and jvmti_state_changed() functions. Both are not called in the JVMCI case. So, these checks look as broken in JVMCI now. JVMCI is in better shape, because it doesn't transition out of _thread_in_vm state, but yes it needs similar changes. Not sure, I have enough compiler knowledge to fix this at this stage of release. Would it better to file a separate hotspot/compiler RFE targeted to 16? It can be assigned to me if it helps. This is a P3 so I believe we have time to fix it for 15. Please go ahead and let's see if we can get it in. I can help with the JVMCI changes if they are not straightforward. dl Thanks, Serguei On 5/28/20 10:54, Dean Long wrote: Sure, you could just have cache_jvmti_state() return a boolean to bail out immediately for
Re: RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
Hi Dean, Thank you a lot for the review! I hope, Christian will have a chance to look at it. Thanks, Serguei On 6/3/20 14:56, Dean Long wrote: Hi Serguei, I like the latest changes so that JVMCI matches C2. Please get another review because this is not a trivial change. dl On 6/3/20 10:06 AM, serguei.spit...@oracle.com wrote: Hi Dean, The updated webrev is: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.3/ Probably, the JVMCI part can be simplified. Only the compile_state line has to be moved up: +JVMCICompileState compile_state(task); // Skip redefined methods -if (target_handle->is_old()) { +if (compile_state.target_method_is_old()) { failure_reason = "redefined method"; retry_message = "not retryable"; compilable = ciEnv::MethodCompilable_never; } else { - JVMCICompileState compile_state(task); Fixes in the jvmciEnv.?pp are not really needed Please, let me know what do you think. This version does not fail at all (in 300 runs for both C2 and JVMCI). It seems, other two issues disappeared as well: This was seen with the C2: https://bugs.openjdk.java.net/browse/JDK-8245128 This was seen with the JVMCI: https://bugs.openjdk.java.net/browse/JDK-8245446 Thanks, Serguei On 6/1/20 23:40, serguei.spit...@oracle.com wrote: Hi Dean, Thank you for the reply. The problem is I do not fully understand your suggestion, especially the part about caching the method,is_old() value in the cache_jvmti_state(). This is a preliminary webrev where I tried to implement your suggestion: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.2/ This variant is failing in half of test runs for both C1/C2 and JVMCI. I think, the root cause is a safepoint in a ThreadInVMfromNative desctructor. Here: 232 void ciEnv::cache_jvmti_state() { 233 VM_ENTRY_MARK; Then we check for the target_method_is_old() value which is not up-to-date any more. I feel, it was correct and more simple before introducing this approach. Probably, I'm missing something here. I also have a question about the update fragment: 1696 { 1697 // Must switch to native to allocate ci_env 1698 ThreadToNativeFromVM ttn(thread); 1699 ciEnv ci_env((CompileTask*)NULL); 1700 1701 // Switch back to VM state to do compiler initialization 1702 ThreadInVMfromNative tv(thread); 1703 ResetNoHandleMark rnhm; 1704 1705 // Perform per-thread and global initializations 1706 comp->initialize(); 1707 } Can we remove the ciEnv object initialization above with the state transitions? Or it has some side effects? Please, let me know what you think. Thanks, Serguei On 6/1/20 15:10, Dean Long wrote: On 5/31/20 11:16 PM, serguei.spit...@oracle.com wrote: Hi Dean, To check the is_old as you suggest the target method has to be passed to the cache_jvmti_state() as argument. Is it what you are suggesting? I believe you can use use _task->method()->is_old(), as the ciEnv already has the task. Just want to make sure I understand you correctly. The cache_jvmti_state() and cache_dtrace_flags() are called in the CompileBroker::init_compiler_runtime() for a ciEnv with the NULL CompileTask which looks unnecessary (or I don't understand it): bool CompileBroker::init_compiler_runtime() { CompilerThread* thread = CompilerThread::current(); . . . ciEnv ci_env((CompileTask*)NULL); // Cache Jvmti state ci_env.cache_jvmti_state(); // Cache DTrace flags ci_env.cache_dtrace_flags();
Re: RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
Hi Dean, The updated webrev is: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.3/ Probably, the JVMCI part can be simplified. Only the compile_state line has to be moved up: +JVMCICompileState compile_state(task); // Skip redefined methods -if (target_handle->is_old()) { +if (compile_state.target_method_is_old()) { failure_reason = "redefined method"; retry_message = "not retryable"; compilable = ciEnv::MethodCompilable_never; } else { - JVMCICompileState compile_state(task); Fixes in the jvmciEnv.?pp are not really needed Please, let me know what do you think. This version does not fail at all (in 300 runs for both C2 and JVMCI). It seems, other two issues disappeared as well: This was seen with the C2: https://bugs.openjdk.java.net/browse/JDK-8245128 This was seen with the JVMCI: https://bugs.openjdk.java.net/browse/JDK-8245446 Thanks, Serguei On 6/1/20 23:40, serguei.spit...@oracle.com wrote: Hi Dean, Thank you for the reply. The problem is I do not fully understand your suggestion, especially the part about caching the method,is_old() value in the cache_jvmti_state(). This is a preliminary webrev where I tried to implement your suggestion: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.2/ This variant is failing in half of test runs for both C1/C2 and JVMCI. I think, the root cause is a safepoint in a ThreadInVMfromNative desctructor. Here: 232 void ciEnv::cache_jvmti_state() { 233 VM_ENTRY_MARK; Then we check for the target_method_is_old() value which is not up-to-date any more. I feel, it was correct and more simple before introducing this approach. Probably, I'm missing something here. I also have a question about the update fragment: 1696 { 1697 // Must switch to native to allocate ci_env 1698 ThreadToNativeFromVM ttn(thread); 1699 ciEnv ci_env((CompileTask*)NULL); 1700 1701 // Switch back to VM state to do compiler initialization 1702 ThreadInVMfromNative tv(thread); 1703 ResetNoHandleMark rnhm; 1704 1705 // Perform per-thread and global initializations 1706 comp->initialize(); 1707 } Can we remove the ciEnv object initialization above with the state transitions? Or it has some side effects? Please, let me know what you think. Thanks, Serguei On 6/1/20 15:10, Dean Long wrote: On 5/31/20 11:16 PM, serguei.spit...@oracle.com wrote: Hi Dean, To check the is_old as you suggest the target method has to be passed to the cache_jvmti_state() as argument. Is it what you are suggesting? I believe you can use use _task->method()->is_old(), as the ciEnv already has the task. Just want to make sure I understand you correctly. The cache_jvmti_state() and cache_dtrace_flags() are called in the CompileBroker::init_compiler_runtime() for a ciEnv with the NULL CompileTask which looks unnecessary (or I don't understand it): bool CompileBroker::init_compiler_runtime() { CompilerThread* thread = CompilerThread::current(); . . . ciEnv ci_env((CompileTask*)NULL); // Cache Jvmti state ci_env.cache_jvmti_state(); // Cache DTrace flags ci_env.cache_dtrace_flags(); These calls look unnecessary to me, as the ci_env will cache these again before compiling a method. I suggest removing these calls. We should make sure the cache fields are initialized to sane values in the ciEnv ctor. The JVMCI has a separate implementation for ciEnv which is jvmciEnv and its own set of cache_jvmti_state() and jvmti_state_changed() functions. Both are not called in the JVMCI case. So, these checks look as broken in JVMCI now. JVMCI is in better shape, because it doesn't transition out of _thread_in_vm state, but yes it needs similar changes. Not sure, I have enough compiler knowledge to fix this at this stage of release.
Re: RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
Hi Dean, Thank you for the reply. The problem is I do not fully understand your suggestion, especially the part about caching the method,is_old() value in the cache_jvmti_state(). This is a preliminary webrev where I tried to implement your suggestion: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.2/ This variant is failing in half of test runs for both C1/C2 and JVMCI. I think, the root cause is a safepoint in a ThreadInVMfromNative desctructor. Here: 232 void ciEnv::cache_jvmti_state() { 233 VM_ENTRY_MARK; Then we check for the target_method_is_old() value which is not up-to-date any more. I feel, it was correct and more simple before introducing this approach. Probably, I'm missing something here. I also have a question about the update fragment: 1696 { 1697 // Must switch to native to allocate ci_env 1698 ThreadToNativeFromVM ttn(thread); 1699 ciEnv ci_env((CompileTask*)NULL); 1700 1701 // Switch back to VM state to do compiler initialization 1702 ThreadInVMfromNative tv(thread); 1703 ResetNoHandleMark rnhm; 1704 1705 // Perform per-thread and global initializations 1706 comp->initialize(); 1707 } Can we remove the ciEnv object initialization above with the state transitions? Or it has some side effects? Please, let me know what you think. Thanks, Serguei On 6/1/20 15:10, Dean Long wrote: On 5/31/20 11:16 PM, serguei.spit...@oracle.com wrote: Hi Dean, To check the is_old as you suggest the target method has to be passed to the cache_jvmti_state() as argument. Is it what you are suggesting? I believe you can use use _task->method()->is_old(), as the ciEnv already has the task. Just want to make sure I understand you correctly. The cache_jvmti_state() and cache_dtrace_flags() are called in the CompileBroker::init_compiler_runtime() for a ciEnv with the NULL CompileTask which looks unnecessary (or I don't understand it): bool CompileBroker::init_compiler_runtime() { CompilerThread* thread = CompilerThread::current(); . . . ciEnv ci_env((CompileTask*)NULL); // Cache Jvmti state ci_env.cache_jvmti_state(); // Cache DTrace flags ci_env.cache_dtrace_flags(); These calls look unnecessary to me, as the ci_env will cache these again before compiling a method. I suggest removing these calls. We should make sure the cache fields are initialized to sane values in the ciEnv ctor. The JVMCI has a separate implementation for ciEnv which is jvmciEnv and its own set of cache_jvmti_state() and jvmti_state_changed() functions. Both are not called in the JVMCI case. So, these checks look as broken in JVMCI now. JVMCI is in better shape, because it doesn't transition out of _thread_in_vm state, but yes it needs similar changes. Not sure, I have enough compiler knowledge to fix this at this stage of release. Would it better to file a separate hotspot/compiler RFE targeted to 16? It can be assigned to me if it helps. This is a P3 so I believe we have time to fix it for 15. Please go ahead and let's see if we can get it in. I can help with the JVMCI changes if they are not straightforward. dl Thanks, Serguei On 5/28/20 10:54, Dean Long wrote: Sure, you could just have cache_jvmti_state() return a boolean to bail out immediately for is_old. dl On 5/28/20 7:23 AM, serguei.spit...@oracle.com wrote: Hi Dean, Thank you for looking at this! Okay. Let me check what cab be done in this direction. There is no point to cache is_old. The compilation has to bail out if it is discovered to be true. Thanks, Serguei On 5/28/20 00:59, Dean Long wrote: This seems OK as long as the memory barriers in the thread state transitions prevent the C++ compiler from doing
Re: RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
On 5/31/20 11:16 PM, serguei.spit...@oracle.com wrote: Hi Dean, To check the is_old as you suggest the target method has to be passed to the cache_jvmti_state() as argument. Is it what you are suggesting? I believe you can use use _task->method()->is_old(), as the ciEnv already has the task. Just want to make sure I understand you correctly. The cache_jvmti_state() and cache_dtrace_flags() are called in the CompileBroker::init_compiler_runtime() for a ciEnv with the NULL CompileTask which looks unnecessary (or I don't understand it): bool CompileBroker::init_compiler_runtime() { CompilerThread* thread = CompilerThread::current(); . . . ciEnv ci_env((CompileTask*)NULL); // Cache Jvmti state ci_env.cache_jvmti_state(); // Cache DTrace flags ci_env.cache_dtrace_flags(); These calls look unnecessary to me, as the ci_env will cache these again before compiling a method. I suggest removing these calls. We should make sure the cache fields are initialized to sane values in the ciEnv ctor. The JVMCI has a separate implementation for ciEnv which is jvmciEnv and its own set of cache_jvmti_state() and jvmti_state_changed() functions. Both are not called in the JVMCI case. So, these checks look as broken in JVMCI now. JVMCI is in better shape, because it doesn't transition out of _thread_in_vm state, but yes it needs similar changes. Not sure, I have enough compiler knowledge to fix this at this stage of release. Would it better to file a separate hotspot/compiler RFE targeted to 16? It can be assigned to me if it helps. This is a P3 so I believe we have time to fix it for 15. Please go ahead and let's see if we can get it in. I can help with the JVMCI changes if they are not straightforward. dl Thanks, Serguei On 5/28/20 10:54, Dean Long wrote: Sure, you could just have cache_jvmti_state() return a boolean to bail out immediately for is_old. dl On 5/28/20 7:23 AM, serguei.spit...@oracle.com wrote: Hi Dean, Thank you for looking at this! Okay. Let me check what cab be done in this direction. There is no point to cache is_old. The compilation has to bail out if it is discovered to be true. Thanks, Serguei On 5/28/20 00:59, Dean Long wrote: This seems OK as long as the memory barriers in the thread state transitions prevent the C++ compiler from doing something like reading is_old before reading redefinition_count. I would feel better if both JVMCI and C1/C2 cached is_old and redefinition_count at the same time (making sure to be in the _thread_in_vm state), then bail out based on the cached value of is_old. dl On 5/26/20 12:04 AM, serguei.spit...@oracle.com wrote: 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
Re: RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
On 5/31/20 23:16, serguei.spit...@oracle.com wrote: Hi Dean, To check the is_old as you suggest the target method has to be passed to the cache_jvmti_state() as argument. Is it what you are suggesting? Just want to make sure I understand you correctly. The cache_jvmti_state() and cache_dtrace_flags() are called in the CompileBroker::init_compiler_runtime() for a ciEnv with the NULL CompileTask which looks unnecessary (or I don't understand it): bool CompileBroker::init_compiler_runtime() { CompilerThread* thread = CompilerThread::current(); . . . ciEnv ci_env((CompileTask*)NULL); // Cache Jvmti state ci_env.cache_jvmti_state(); // Cache DTrace flags ci_env.cache_dtrace_flags(); The JVMCI has a separate implementation for ciEnv which is jvmciEnv and its own set of cache_jvmti_state() and jvmti_state_changed() functions. Both are not called in the JVMCI case. So, these checks look as broken in JVMCI now. Not sure, I have enough compiler knowledge to fix this at this stage of release. Would it better to file a separate hotspot/compiler RFE targeted to 16? It can be assigned to me if it helps. I forgot to tell that this RFE should probably include some refactoring to make all this more clear and elegant. Thanks, Serguei Thanks, Serguei On 5/28/20 10:54, Dean Long wrote: Sure, you could just have cache_jvmti_state() return a boolean to bail out immediately for is_old. dl On 5/28/20 7:23 AM, serguei.spit...@oracle.com wrote: Hi Dean, Thank you for looking at this! Okay. Let me check what cab be done in this direction. There is no point to cache is_old. The compilation has to bail out if it is discovered to be true. Thanks, Serguei On 5/28/20 00:59, Dean Long wrote: This seems OK as long as the memory barriers in the thread state transitions prevent the C++ compiler from doing something like reading is_old before reading redefinition_count. I would feel better if both JVMCI and C1/C2 cached is_old and redefinition_count at the same time (making sure to be in the _thread_in_vm state), then bail out based on the cached value of is_old. dl On 5/26/20 12:04 AM, serguei.spit...@oracle.com wrote: 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()
Re: RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
Hi Dean, To check the is_old as you suggest the target method has to be passed to the cache_jvmti_state() as argument. Is it what you are suggesting? Just want to make sure I understand you correctly. The cache_jvmti_state() and cache_dtrace_flags() are called in the CompileBroker::init_compiler_runtime() for a ciEnv with the NULL CompileTask which looks unnecessary (or I don't understand it): bool CompileBroker::init_compiler_runtime() { CompilerThread* thread = CompilerThread::current(); . . . ciEnv ci_env((CompileTask*)NULL); // Cache Jvmti state ci_env.cache_jvmti_state(); // Cache DTrace flags ci_env.cache_dtrace_flags(); The JVMCI has a separate implementation for ciEnv which is jvmciEnv and its own set of cache_jvmti_state() and jvmti_state_changed() functions. Both are not called in the JVMCI case. So, these checks look as broken in JVMCI now. Not sure, I have enough compiler knowledge to fix this at this stage of release. Would it better to file a separate hotspot/compiler RFE targeted to 16? It can be assigned to me if it helps. Thanks, Serguei On 5/28/20 10:54, Dean Long wrote: Sure, you could just have cache_jvmti_state() return a boolean to bail out immediately for is_old. dl On 5/28/20 7:23 AM, serguei.spit...@oracle.com wrote: Hi Dean, Thank you for looking at this! Okay. Let me check what cab be done in this direction. There is no point to cache is_old. The compilation has to bail out if it is discovered to be true. Thanks, Serguei On 5/28/20 00:59, Dean Long wrote: This seems OK as long as the memory barriers in the thread state transitions prevent the C++ compiler from doing something like reading is_old before reading redefinition_count. I would feel better if both JVMCI and C1/C2 cached is_old and redefinition_count at the same time (making sure to be in the _thread_in_vm state), then bail out based on the cached value of is_old. dl On 5/26/20 12:04 AM, serguei.spit...@oracle.com wrote: 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
Re: RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
Sure, you could just have cache_jvmti_state() return a boolean to bail out immediately for is_old. dl On 5/28/20 7:23 AM, serguei.spit...@oracle.com wrote: Hi Dean, Thank you for looking at this! Okay. Let me check what cab be done in this direction. There is no point to cache is_old. The compilation has to bail out if it is discovered to be true. Thanks, Serguei On 5/28/20 00:59, Dean Long wrote: This seems OK as long as the memory barriers in the thread state transitions prevent the C++ compiler from doing something like reading is_old before reading redefinition_count. I would feel better if both JVMCI and C1/C2 cached is_old and redefinition_count at the same time (making sure to be in the _thread_in_vm state), then bail out based on the cached value of is_old. dl On 5/26/20 12:04 AM, serguei.spit...@oracle.com wrote: 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
Re: RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
Hi Dean, Thank you for looking at this! Okay. Let me check what cab be done in this direction. There is no point to cache is_old. The compilation has to bail out if it is discovered to be true. Thanks, Serguei On 5/28/20 00:59, Dean Long wrote: This seems OK as long as the memory barriers in the thread state transitions prevent the C++ compiler from doing something like reading is_old before reading redefinition_count. I would feel better if both JVMCI and C1/C2 cached is_old and redefinition_count at the same time (making sure to be in the _thread_in_vm state), then bail out based on the cached value of is_old. dl On 5/26/20 12:04 AM, serguei.spit...@oracle.com wrote: 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
Re: RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
This seems OK as long as the memory barriers in the thread state transitions prevent the C++ compiler from doing something like reading is_old before reading redefinition_count. I would feel better if both JVMCI and C1/C2 cached is_old and redefinition_count at the same time (making sure to be in the _thread_in_vm state), then bail out based on the cached value of is_old. dl On 5/26/20 12:04 AM, serguei.spit...@oracle.com wrote: 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
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