Ask for a patch review: CallSite.target is not volatile

[John Rose]

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 <forax at univ-mlv.fr>
Date: September 18, 2008 4:40:34 PM PDT
To: Da Vinci Machine Project <mlvm-dev at openjdk.java.net>
Subject: Re: Ask for a patch review: CallSite.target is not volatile
Reply-To: Da Vinci Machine Project <mlvm-dev at openjdk.java.net>

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
mlvm-dev at openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev




Begin forwarded message:

From: John Rose <John.Rose at Sun.COM>
Date: September 18, 2008 5:40:06 PM PDT
To: Da Vinci Machine Project <mlvm-dev at openjdk.java.net>
Subject: Re: Ask for a patch review: CallSite.target is not volatile
Reply-To: Da Vinci Machine Project <mlvm-dev at openjdk.java.net>

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
mlvm-dev at openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/mlvm-dev/attachments/20090421/477e652f/attachment.html