I'm gathering up issues from old mail, and found the incomplete
exchange below on the mlvm dev list.
I've followed your advice and removed the "protected" keyword from
CallSite fields. The accessors are sufficient.
I'm also going to remove the single-inheritance mixin pattern which
you objected to, since IBM has also objected to it (on the EG).
Basically, I'm going to have the JVM special-case MethodHandle instead
of a superclass sun.dyn.MethodHandleImpl.
(My English teacher in high school used to say, "kill your darlings",
meaning that, if you have a bit of writing you think particularly
clever, you probably need to edit it.)
I've raised the issues about target invalidation with the EG. Are
there other issues in the exchange below that we should call out?
Thanks,
-- John
Begin forwarded message:
From: Rémi Forax <[email protected]>
Date: September 18, 2008 4:40:34 PM PDT
To: Da Vinci Machine Project <[email protected]>
Subject: Re: Ask for a patch review: CallSite.target is not volatile
Reply-To: Da Vinci Machine Project <[email protected]>
John Rose a écrit :
On Sep 13, 2008, at 7:27 AM, Rémi Forax wrote:
currently CallSite.target is not volatile, so if an invokedynamic
bootstrap
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
synchronization.
yop, i have seen that. I'm currently not able to provide
that requirement for the backport.
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
method call).
The problem is if a new class is loaded by another thread,
I may want to change the target for any other threads.
If you cache the target, it will not change when a new class is
discovered.
i.e the target will change in RAM but not in the local cache.
ConcurrentLinkedQueue<Data> queue=...
thread1:
for(Data data:queue) {
// invokedynamic call using data
}
thread2:
queue.add(new DataSubClass());
// here the classloader load DataSubClass and
// defeat the optimisation done at
// invokedynamic call site.
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.
not agree, if the accessor is inlined, target can be not reloaded from
the RAM.
Except to the backport. For the
backport, I guess the volatile bit is needed. So, yes, that's a good
change.
Yes.
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.
I'am not sure you can avoid the volatile read.
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
class.
Therefore, let's keep the protected stuff for now, unless we decide
to make CallSite into an interface.
You don't need protected field if you have accessors. And currently
those accessors exist.
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 not use a static final field, like the anonk code already does ?
and detect at runtime if there is a native impl or if the backport
must be used.
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.
I think a static final field is a better option.
-- John
Rémi
_______________________________________________
mlvm-dev mailing list
[email protected]
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Begin forwarded message:
From: John Rose <[email protected]>
Date: September 18, 2008 5:40:06 PM PDT
To: Da Vinci Machine Project <[email protected]>
Subject: Re: Ask for a patch review: CallSite.target is not volatile
Reply-To: Da Vinci Machine Project <[email protected]>
On Sep 18, 2008, at 4:40 PM, Rémi Forax wrote:
The problem is if a new class is loaded by another thread,
I may want to change the target for any other threads.
If you cache the target, it will not change when a new class is
discovered.
i.e the target will change in RAM but not in the local cache.
Do we agree that an cached old target is not a correctness issue?
A system which an have out-of-date targets that has to be robust
against such changes even without a volatile variable. The target
method itself must include whatever version checks are needed for
correctness.
What volatile might buy you is a slightly better guarantee that the
new target will be seen earlier. But, if the old target has a
correctness check, it will presumably have a slow path to clean up
when a problem is seen, and this in turn, if it changes the state of
memory, will cause the thread to reload even a non-volatile target
field.
If a language system use targets that do not perform version checks
(against data structures with variable semantics, such as open
inheritance hierarchies), it will need to create some sort of
handshake to get out of the old code before the new code is loaded.
(See SystemDictionary::add_to_hierarchy in the JVM.) This requires a
safepoint mechanism of some sort to make sure that all threads are
blocked from entering call sites to old targets, and/or roll forward
out of such call sites.
Generally, only the JVM knows enough about each thread to make this
happen, hence the need for a bulk invalidation API. I'm thinking
that the language runtime will do the following to implement a change
to an unchecked quasi-invariant:
1. grab a lock on the variable global state (e.g., class hierarchy)
2. compute a limited set of call sites to invalidated, based on
language-specific dependency tables
3. call the 292 API to bulk-invalidate those call sites
4. make the change to the variable global state
5. eagerly patch invalidated call sites, if desired, and/or respond
to bootstrap requests (which can interact with the global lock)
6. release the lock
7. recover from any remaining consequences incrementally, as
bootstrap methods are called on invalidated call sites
Step 3 is magic and expensive. It may invalidate "innocent
bystander", call sites which are not actually invalid. It will at
least make sure that any pending dynamic calls at the invalid call
sites are fully executed, up to a global synchronization point that
it defines internally.
However, after step 3, there may well be old methods on the stack.
For example, a dynamic call might have completed to an outdated
It is up to the language runtime to cope with these. We come back to
the need to have self-verifying or self-correcting target methods.
In fact, the whole bulk invalidation API could be unnecessary, given
that target methods must always be self-verifying anyway. However, I
think it will simplify the self-verification tests, for some languages.
BTW, in HotSpot compiled methods are self-correcting, in that they
can be deoptimized at any time, replaced by interpreted code, which
is less optimal but more robustly correct. I'm trying to stay away
from defining such a mechanism above the level of bytecodes, because
I don't see how to define and implement it simply.
The "hook" you get now for this behavior is to generate an explicit
slow-patch check in the bytecodes. By that I mean a test which
always succeeds, until the quasi-invariant fails. The JIT is likely
to notice (from the profile) that the check "always" succeeds, and
compile it to an "uncommon trap", a dead-end in the compiled code
which deoptimizes and jumps back to the interpreter. The JIT does
not compile anything downstream of the uncommon trap, and so the
cleanup does not affect the optimization of the fast path. As long
as only the fast path is taken, the specialized JIT code rules.
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.
not agree, if the accessor is inlined, target can be not reloaded from
the RAM.
The point is, the accessor can have the required memory barrier
(e.g., from Unsafe, or an empty synch) without marking the field
volatile.
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.
I'am not sure you can avoid the volatile read.
I'm not yet sure that the volatile read even does what you hope it
does! See above.
You don't need protected field if you have accessors. And currently
those accessors exist.
Right, except for CallSite.callerObject.
Why not use a static final field, like the anonk code already does ?
and detect at runtime if there is a native impl or if the backport
must be used.
That doesn't let you mix in JVM-specific fields like a superclass does.
Best wishes,
-- John
_______________________________________________
mlvm-dev mailing list
[email protected]
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
_______________________________________________
mlvm-dev mailing list
[email protected]
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
