RFR: 8219101: Stop using ICStubs for CompiledIC transitions to clean

[Erik Osterlund]

Hi Dean,

> On 26 Feb 2019, at 08:29, dean.long at oracle.com wrote:
> 
> Is there any need for is_safe_for_patching() anymore?

Probably not!

> entry_point == _call->get_resolve_call_stub(is_optimized() is used in several places.  Can we refactor it into something like is_clean()? (I realize that name is taken.)

Sure.

> Undoing an ICStub (it appears this happens in ICStub::finalize) also sets both data and destination, so it needs to be done at a safepoint.  Can we add an assert there to document that?

I’m currently working on reclaiming these concurrently so I don’t think I would like to add that assert if that is okay. I really don’t like safepoints, what can I say...

> Unrelated to your change, but the comment "called when a method is removed" for ICStub::finalize in icBuffer.hpp doesn't sound right to me.

Yeah that comment is wrong.

> Also, not part of your change, but in CompiledIC::internal_set_ic_destination(), don't we need a memory barrier between setting the entry point and setting the data?

That is a long story. The short story is that all patching calls wrote() which performs mfence/isb/isync and invalidates instruction caches. There is another much much longer story, that I really have to refrain from telling yet.

> In CompiledIC::set_to_monomorphic(), why is the following transition safe?  I would think we need a transition stub here:
> 
> 446 } else if (!is_in_transition_state() && get_data() == Universe::non_oop_word()) {
> 447 set_ic_destination_and_value(info.entry(), info.cached_metadata());

The thought has been thst all interleavings have a happy ending. Any execution that sees the old value will miss in the unverified entry, regardless of destination. And whatever data was observed, executions with the old destination will call a resolution handler. Only executions observing the new data and new destination will get past the unverified entry.

> I would feel better if there was a complete state diagram that captured all the important details.  There is a small one in compiledIC.hpp but it doesn't capture everything, especially ICStub transitions and safepoints.

Agreed. I will try to address this. I am out of office until tuesday next week though. But thank you for the review!

Thanks,
/Erik

> dl
> 
> 
>> On 2/18/19 4:01 AM, Erik Österlund wrote:
>> Hi,
>> 
>> When transitioning CompiledICs to clean, we sometimes need to use transitional states (using JIT-compiled ICStubs).
>> The trick is that today when we clean ICs, we atomically and logically write NULL to the data section, and set the destination to some resolve stub. In reality, the NULL gets translated to Universe::non_oop_word. But the point is that in order to set both data and destination atomically, we use an ICStub.
>> 
>> ...but we don't need to set them both.
>> 
>> When the inline cache is cleaned, the data part is no longer relevant, and could really be set to any nonsense value, as long as subsequent transitions away from that cleaned inline cache (monomorphic/interpreted) uses a transitional state to do so.
>> I suspect that at some point in time, when inline caches pointed at oops in perm gen or something, it was crucial that the data part of the inline cache made sense to the GC. This is not the case today, and we can just ignore the value pretending it doesn't exist instead.
>> 
>> The benefit with deferring the use of transitional state to when someone performs a subsequent transition away from clean (set_to_monomorphic), is that all code paths where cleaning inline caches fail can be removed. This in particular simplifies the sweeper and ZGC nmethod unloading code, that needs to handle failures in inline cache cleaning due to running out of ICStubs forcing us to trigger safepoints to refill IC stubs. It should also reduce the overall number of safepoints due to running out of ICStubs.
>> 
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8219101
>> 
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8219101/webrev.00/
>> 
>> Testing: mach5 hs-tier1-5.
>> 
>> Thanks,
>> /Erik
>