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


Reply via email to