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

[John Rose]

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