Ask for a patch review: CallSite.target is not volatile
John.Rose at Sun.COM
Thu Sep 18 15:31:14 PDT 2008
On Sep 13, 2008, at 7:27 AM, Rémi Forax wrote:
> currently CallSite.target is not volatile, so if an invokedynamic
> method calls setTarget() in one thread, other threads may not see the
> new target.
> Proposed changes :
> - make target volatile
> - use unsafe.putOrdered to set the field in a more lightweight way.
Hmm... The EDR is ambiguous about this, and needs to be tightened
up. It does say that the bulk invalidation API is intended to
provide a global interlock, and gives some freedom to the propagation
of setTarget effects. What the EDR says about "next call" refers to
a global ordering that doesn't really exist in the JVM apart from
Native JVM implementations don't require target to be explicitly
volatile. In fact, individual threads need the freedom to cache the
target, e.g., as a loop invariant for a fast-path version of a loop.
The important thing to optimize here is the speed of *reading* the
target field. Writes can be as slow as we want (I use a native
All this is independent of whether the code says "volatile" or not on
the field, but it may be better not to mark it volatile, since that
seems to make a guarantee about propagation of setTarget effects.
Note that the JVM reads the target field in a specialized way, not
necessarily through a bytecode. Also, the writes to the target field
go through an accessor, so once again, the volatility of the field
does not necessarily matter. Except to the backport. For the
backport, I guess the volatile bit is needed. So, yes, that's a good
Maybe the right thing to do is move target into a mixin-style
superclass, java.dyn.impl.CS, and have it be volatile in the backport.
This takes me to the second proposed change.
> - change alls field visibility from protected to private because :
> 1) protected fields appear in the JavaDoc,
> so another implementation of CallSite (like the one currently
> use by the backport)
> is not allowed.
> 2) caller, name and type are final so caller(), name() and type()
> can be used instead.
> subclasses can override checkTarget() to do nothing, in that
> case setTarget() is equivalent to change the target field.
> So I see no reason to declare this fields protected.
I want the CallSite to be usable two ways:
1. created by the JVM to reify invokedynamic instructions
2. creatable by Java code to simulate invokedynamic sites interpretively
Therefore, it's important to allow Java code to make its own flavor
of CallSite. Hence the protected methods & fields.
This could also be done with a CallSite interface, but it seems
easier to me (and to other JVM vendors I think) to make it a concrete
Therefore, let's keep the protected stuff for now, unless we decide
to make CallSite into an interface.
I want to factor the 292 API into common classes, including the
public ones, and non-standard implementation-specific classes, one
version for each kind of JVM and one for the backport. The
java.dyn.impl package contains the implementation-specific classes.
(This name will change to go outside of the java.* hierarchy.) When
a public type needs to mix in fields or methods, it does so via
inheritance from an implementation superclass. Switching between
backport and native implementations will therefore require
adjustments to boot class path or the installed rt.jar.
Why use mixins? Although such pluggable implementations are more
often done via delegation or subclassing, the performance
requirements of method handles requires a more aggressive (less
flexible) pattern of single-inheritance mixins.
More information about the mlvm-dev