Hi,
On 2015-11-12 17:26, Peter Levart wrote:
Hi Claes,
I have one concern...
645 private static NamedFunction getConstantFunction(int idx) {
646 NamedFunction function = FUNCTIONS[idx];
647 if (function != null) {
648 return function;
649 }
650 return setCachedFunction(idx, makeConstantFunction(idx));
651 }
652
653 private static synchronized NamedFunction
setCachedFunction(int idx, final NamedFunction function) {
654 // Simulate a CAS, to avoid racy duplication of results.
655 NamedFunction prev = FUNCTIONS[idx];
656 if (prev != null) {
657 return prev;
658 }
659 FUNCTIONS[idx] = function;
660 function.resolve();
661 return function;
662 }
Above is a classical double-checked locking idiom, but it is not using
volatile variable to publish the NamedFunction instance. I wonder if
this is safe. Even if the FUNCTIONS[idx] slot was a volatile
variable, you would publish new instance before resolving it. Is it
OK to publish unresolved NamedFunction(s)? There is a
NamedFunction.resolvedHandle() instance method that makes sure
NamedFunction is resolved before returning a MethodHandle, but there
are also usages of dereferencing NamedFunction.resolvedHandle field
directly in code. Are you sure that such unresolved or almost resolved
instance of NamedFunction is never used in such places where
NamedFunction.resolvedHandle field is dereferenced directly?
In original code those NamedFunctions were resolved in static
initializer so they were published properly.
I think we're relying on the fact that relevant field, member, is final
and thus will be published correctly.
resolvedHandle will be set to null in the constructor we're using anyhow:
NamedFunction(Method method) {
this(new MemberName(method));
}
...
NamedFunction(MemberName member) {
this.member = member;
this.resolvedHandle = null;
}
I inquired myself about the rationale for using of DCL without volatile
elsewhere in the java.lang.invoke code and was pointed to this earlier
discussion that shed light on the assumptions and caveats:
http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-May/013902.html
So, all in all, I think the partially unsafe publication is actually
safe for our purposes here.
Thanks!
/Claes
