Re: [9] RFR(S): 8005873: JRuby test_respond_to.rb asserts with: MT-unsafe modification of inline cache
Tobias, I agree with your evaluation. My only concern is that @Stable doesn't work for AtomicReferenceArray, so JIT doesn't see what is stored in the array. Maybe use a lock instead? Best regards, Vladimir Ivanov On 5/14/14 2:33 PM, Tobias Hartmann wrote:
Hi Remi,
sorry, I accidentally reverted that part.. Here is the correct webrev:
http://cr.openjdk.java.net/~anoll/8005873/webrev.01/
Thanks, Tobias
On 14.05.2014 12:04, Remi Forax wrote:
your patch doesn't work !
the array is still allocated as an classical array in the constructor.
cheers, Remi
Envoyé avec AquaMail pour Android http://www.aqua-mail.com
Le 14 mai 2014 11:04:41 Tobias Hartmann <tobias.hartmann@oracle.com> a écrit :
Hi,
please review the following patch for bug 8005873.
*Problem* Bug: https://bugs.openjdk.java.net/browse/JDK-8005873
The test creates multiple threads (~500) that repeatedly execute invokeExact on a method handle referencing a static method. The call to invokeExact is performed through an optimized inline cache (CompiledStaticCall) that points to the LambdaForm generated for the method handle. The IC is first set to interpreted code by CompiledStaticCall::set_to_interpreted(...) but soon updated to refer to the compiled version of the lambda form (-Xcomp).
Because tiered compilation is enabled, the VM decides to compile the LambdaForm at a different level and sets the old version to not-entrant. The next call through the IC therefore triggers a re-resolving of the call site finally performing a Java upcall to java.lang.invoke.MethodHandleNatives::linkMethod(...) (see call sequence [1]). A *new* LambdaForm is returned and CompiledStaticCall::set_to_interpreted(...) is executed again to update the IC. The assert is hit because the callee differs.
The problem is that there are multiple LambdaForms created for the same invokeExact instruction. Caching in the Java runtime code should guarantee that only one LambdaForm is created and reused. Instrumenting Invokers::invokeHandleForm(...) shows that almost all requests result in a cache miss and return a new LambdaForm.
This behaviour is caused by a race condition in Invokers::invokeHandleForm(...). Threads may initially see a cache miss when invoking MethodTypeForm::cachedLambdaForm(...), then create a new LambdaForm and call MethodTypeForm::setCachedLambdaForm(...) to update the cache without checking it again. In a high concurrency setting, this leads to multiple LambdaForms being created for the same invokeExact instruction because the cache entry is overwritten by multiple threads.
*Solution* Webrev: http://cr.openjdk.java.net/~anoll/8005873/webrev.00/
An AtomicReferenceArray is used to cache the LambdaForms and .get(...) and .compareAndSet(...) are used to retrieve and update the cache entries. This allows only one thread to set the LambdaForm that is then being used by all instances of the invokeExact.
*Testing* - Failing test (vm/mlvm/meth/stress/jni/nativeAndMH) - Nashorn + Octane - JPRT
Many thanks to Christian Thalinger and Vladimir Ivanov!
Best, Tobias
[1] Call sequence of reresolving the IC target SharedRuntime::handle_wrong_method(...) -> SharedRuntime::reresolve_call_site(...) -> SharedRuntime::find_callee_method(...) -> SharedRuntime::find_callee_info_helper(...) -> LinkResolver::resolve_invoke(...) -> LinkResolver::resolve_invokehandle(...) -> LinkResolver::resolve_invokehandle(...) -> LinkResolver::lookup_polymorphic_method(...) -> SystemDictionary::find_method_handle_invoker(...) -> Java upcall to java.lang.invoke.MethodHandleNatives::linkMethod(...) -> Invokers::methodHandleInvokeLinkerMethod(...) -> Invokers::invokeHandleForm(...)
Backport?
On May 14, 2014, at 12:47 PM, Vladimir Ivanov <vladimir.x.ivanov@oracle.com> wrote:
Tobias, I agree with your evaluation.
V. tricky one to track down! From @Stable: * It is (currently) undefined what happens if a field annotated as stable * is given a third value. In practice, if the JVM relies on this annotation * to promote a field reference to a constant, it may be that the Java memory * model would appear to be broken, if such a constant (the second value of the field) * is used as the value of the field even after the field value has changed. I dunno if that was a contributing factor in this case.
My only concern is that @Stable doesn't work for AtomicReferenceArray, so JIT doesn't see what is stored in the array.
Yes, stability needs to be associated with the array elements.
Maybe use a lock instead?
Or perhaps use Unsafe.CAS directly within setCachedLambdaForm? Also, as a consequence of using AtomicReferenceArray the following change may result in a memory barrier on some architectures: public LambdaForm cachedLambdaForm(int which) { - return lambdaForms[which]; + return lambdaForms.get(which); } since lambdaForms.get will call Unsafe.getObjectVolatile. Separately, i think code that calls setCachedLambdaForm needs to be double checked to ensure that the return value is used. For example, in MethodHandleImpl.makeGuardWithCatchForm i see: basicType.form().setCachedLambdaForm(MethodTypeForm.LF_GWC, lform); return lform; which i think needs to be: return basicType.form().setCachedLambdaForm(MethodTypeForm.LF_GWC, lform); Paul.
On 5/14/14 4:12 PM, Paul Sandoz wrote:
On May 14, 2014, at 12:47 PM, Vladimir Ivanov <vladimir.x.ivanov@oracle.com> wrote:
Tobias, I agree with your evaluation.
V. tricky one to track down!
From @Stable:
* It is (currently) undefined what happens if a field annotated as stable * is given a third value. In practice, if the JVM relies on this annotation * to promote a field reference to a constant, it may be that the Java memory * model would appear to be broken, if such a constant (the second value of the field) * is used as the value of the field even after the field value has changed.
I dunno if that was a contributing factor in this case.
No, @Stable doesn't contribute to the problem.
My only concern is that @Stable doesn't work for AtomicReferenceArray, so JIT doesn't see what is stored in the array.
Yes, stability needs to be associated with the array elements.
Maybe use a lock instead?
Or perhaps use Unsafe.CAS directly within setCachedLambdaForm?
Yes, Unsafe is another option here. But since cache updates should be rare, Unsafe is an overkill here IMO - locking should be fine.
Also, as a consequence of using AtomicReferenceArray the following change may result in a memory barrier on some architectures:
public LambdaForm cachedLambdaForm(int which) { - return lambdaForms[which];
+ return lambdaForms.get(which); }
since lambdaForms.get will call Unsafe.getObjectVolatile.
MTF.cachedLambaForm isn't on a fast path, so it shouldn't be a problem.
Separately, i think code that calls setCachedLambdaForm needs to be double checked to ensure that the return value is used. For example, in MethodHandleImpl.makeGuardWithCatchForm i see:
basicType.form().setCachedLambdaForm(MethodTypeForm.LF_GWC, lform); return lform;
which i think needs to be:
return basicType.form().setCachedLambdaForm(MethodTypeForm.LF_GWC, lform); Good catch! MTF.setCachedLambdaForm usages should be fixed as well.
Best regards, Vladimir Ivanov
Paul.
On May 14, 2014, at 3:47 AM, Vladimir Ivanov <vladimir.x.ivanov@oracle.com> wrote:
Tobias, I agree with your evaluation.
My only concern is that @Stable doesn't work for AtomicReferenceArray, so JIT doesn't see what is stored in the array.
Now that is really unfortunate.
Maybe use a lock instead?
Best regards, Vladimir Ivanov
On 5/14/14 2:33 PM, Tobias Hartmann wrote:
Hi Remi,
sorry, I accidentally reverted that part.. Here is the correct webrev:
http://cr.openjdk.java.net/~anoll/8005873/webrev.01/
Thanks, Tobias
On 14.05.2014 12:04, Remi Forax wrote:
your patch doesn't work !
the array is still allocated as an classical array in the constructor.
cheers, Remi
Envoyé avec AquaMail pour Android http://www.aqua-mail.com
Le 14 mai 2014 11:04:41 Tobias Hartmann <tobias.hartmann@oracle.com> a écrit :
Hi,
please review the following patch for bug 8005873.
*Problem* Bug: https://bugs.openjdk.java.net/browse/JDK-8005873
The test creates multiple threads (~500) that repeatedly execute invokeExact on a method handle referencing a static method. The call to invokeExact is performed through an optimized inline cache (CompiledStaticCall) that points to the LambdaForm generated for the method handle. The IC is first set to interpreted code by CompiledStaticCall::set_to_interpreted(...) but soon updated to refer to the compiled version of the lambda form (-Xcomp).
Because tiered compilation is enabled, the VM decides to compile the LambdaForm at a different level and sets the old version to not-entrant. The next call through the IC therefore triggers a re-resolving of the call site finally performing a Java upcall to java.lang.invoke.MethodHandleNatives::linkMethod(...) (see call sequence [1]). A *new* LambdaForm is returned and CompiledStaticCall::set_to_interpreted(...) is executed again to update the IC. The assert is hit because the callee differs.
The problem is that there are multiple LambdaForms created for the same invokeExact instruction. Caching in the Java runtime code should guarantee that only one LambdaForm is created and reused. Instrumenting Invokers::invokeHandleForm(...) shows that almost all requests result in a cache miss and return a new LambdaForm.
This behaviour is caused by a race condition in Invokers::invokeHandleForm(...). Threads may initially see a cache miss when invoking MethodTypeForm::cachedLambdaForm(...), then create a new LambdaForm and call MethodTypeForm::setCachedLambdaForm(...) to update the cache without checking it again. In a high concurrency setting, this leads to multiple LambdaForms being created for the same invokeExact instruction because the cache entry is overwritten by multiple threads.
*Solution* Webrev: http://cr.openjdk.java.net/~anoll/8005873/webrev.00/
An AtomicReferenceArray is used to cache the LambdaForms and .get(...) and .compareAndSet(...) are used to retrieve and update the cache entries. This allows only one thread to set the LambdaForm that is then being used by all instances of the invokeExact.
*Testing* - Failing test (vm/mlvm/meth/stress/jni/nativeAndMH) - Nashorn + Octane - JPRT
Many thanks to Christian Thalinger and Vladimir Ivanov!
Best, Tobias
[1] Call sequence of reresolving the IC target SharedRuntime::handle_wrong_method(...) -> SharedRuntime::reresolve_call_site(...) -> SharedRuntime::find_callee_method(...) -> SharedRuntime::find_callee_info_helper(...) -> LinkResolver::resolve_invoke(...) -> LinkResolver::resolve_invokehandle(...) -> LinkResolver::resolve_invokehandle(...) -> LinkResolver::lookup_polymorphic_method(...) -> SystemDictionary::find_method_handle_invoker(...) -> Java upcall to java.lang.invoke.MethodHandleNatives::linkMethod(...) -> Invokers::methodHandleInvokeLinkerMethod(...) -> Invokers::invokeHandleForm(...)
Backport?
participants (3)
-
Christian Thalinger
-
Paul Sandoz
-
Vladimir Ivanov