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

[dean.long at oracle.com]

Is there any need for is_safe_for_patching() anymore?

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.)

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?

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.

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?

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());


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.

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