Request for reviews (S): 7071709: JSR 292: switchpoint invalidation should be pushed not pulled

[Tom Rodriguez]

On Aug 29, 2011, at 9:52 AM, Christian Thalinger wrote:

> 
> On Aug 28, 2011, at 1:44 AM, John Rose wrote:
> 
>> On Aug 26, 2011, at 2:16 AM, Christian Thalinger wrote:
>> 
>>>> Also, get_field_by_offset should be called on the actual type of the ciCallSite, not env->CallSite_klass.  Otherwise you might get NULL for the ciField, if the target fields are split out across different call site subclasses.
>>> 
>>> Are there plans to do this?  I changed ciField::is_call_site_target to check for subclasses of CallSite.
>> 
>> No, no plans.  Just a move toward robustness.  Right now we have a common field inherited from CS that is faked into final or volatile field semantics in the subclasses.  I'm afraid we could get forced to split the field at some point.
> 
> I think the point is now.  setTargetVolatile uses Unsafe to fake the volatile field semantics and the compiler doesn't recognize this as a field store to CS.target.  Thus we miss the field stores to a VCS and end up with wrong behavior.
> 
> I'm currently preparing something that does this refactoring.

I'm not so sure this is a good idea.  A fair amount of code assumes that the structure of all CallSites is the same:

  __ load_heap_oop(rcx_method_handle, Address(rax_callsite, __ delayed_value(java_lang_invoke_CallSite::target_offset_in_bytes, rdx)));
  __ null_check(rcx_method_handle);
  __ verify_oop(rcx_method_handle);
  __ prepare_to_jump_from_interpreted();
  __ jump_to_method_handle_entry(rcx_method_handle, rdx);

I guess we could require/enforce that all call site subclasses have their target field at the same offset but it does seem to be break something fairly fundamental.

You could trap these writes in the Unsafe machinery instead.  That's fairly ugly but easy enough to do with a few assumptions about how it will be written.  We might have to worry about reflection too, though that should either use Unsafe I think.

Maybe we should move forward with what we have and deal with VCS later?

tom

> 
> Why was this Unsafe trick used in the first place?
> 
> -- Christian
> 
>> 
>> On Aug 26, 2011, at 2:23 AM, Christian Thalinger wrote:
>> 
>>> 
>>> On Aug 26, 2011, at 1:47 AM, John Rose wrote:
>>> 
>>>> On Aug 25, 2011, at 11:32 AM, Tom Rodriguez wrote:
>>>> 
>>>>> The docs for VolatileCallSite suggest setTarget can be called whenever you feel like it, so it seems like it can be set many times.  MutableCallSite can be as well but the implication is that it's not updated very often.  I'm actually unclear what distinction is trying to be made with VolatileCallSite.
>>>> 
>>>> The MCS and VCS have the same semantics, except for the extra memory barriers on VCS.  These barriers do not affect the validity or applicability of push notification.  Also, either an MCS or VCS might end up being "megamutable", so there has to be some sort of cutoff that will prevent target-prediction for a CS which has been mispredicted too many times.  We need to squeeze a bit of state into the CS, somewhere, which displays how many times the thing has been mispredicted.
>>> 
>>> So the question is should we go with what we currently have for invokedynamic (only optimize CCS and MCS) or should we allow all CSs to be optimized and start working on the logic John describes.  I'm fine with both.
>> 
>> I think we need the throttling logic right away.  I'm not comfortable with encouraging users to use VCS just because MCS has a performance bug.
>> 
>> -- John
>