[9] RFR(S): 8134493: Cleaning inline caches of unloaded nmethods should be done in sweeper

[Tobias Hartmann]

Hi Martin,

thanks for looking at this.

On 27.08.2015 12:26, Doerr, Martin wrote:
> Hi,
> 
> one question about always avoiding transition stubs for unloaded nmethods in cleanup_inline_caches() came into my mind when taking a second look at the change.
> When we transition to zombie state, this should be safe because it's guaranteed that no activations exist.
> However, cleanup_inline_caches() is used at other places, too.
> 
> The comment about the unloaded state only says "// there should be no activations". Is it guaranteed that there is no activation of an nmethod which is in unloaded state?
> If not, the change may have undesired side effects.

I think it is guaranteed that unloaded nmethods are not on the stack and it should be save to clean their ICs without a transition stub. We also rely on this assumption in the sweeper by directly flushing unloaded OSR nmethods (see line 644 in sweeper.cpp [1]).

Maybe someone from the GC team (CC'ed) can clarify this.

Thanks,
Tobias

[1] http://cr.openjdk.java.net/~thartmann/8134493/webrev.00/src/share/vm/runtime/sweeper.cpp.html

> 
> Besides this, the change looks good to me.
> 
> Best regards,
> Martin
> 
> 
> -----Original Message-----
> From: Tobias Hartmann [mailto:tobias.hartmann at oracle.com] 
> Sent: Donnerstag, 27. August 2015 09:10
> To: hotspot-compiler-dev at openjdk.java.net
> Cc: Doerr, Martin
> Subject: [9] RFR(S): 8134493: Cleaning inline caches of unloaded nmethods should be done in sweeper
> 
> Hi,
> 
> please review the following patch.
> 
> https://bugs.openjdk.java.net/browse/JDK-8134493
> http://cr.openjdk.java.net/~thartmann/8134493/webrev.00/
> 
> Problem:
> This is a follow up on JDK-8075805 [1] which modified CodeCache::gc_epilogue() to clean the ICs of unloaded nmethods as well. The problem is that this code is executed at a safepoint and may affect safepoint duration. The other changes of JDK-8134493 are fine.
> 
> Solution:
> We do the cleaning of unloaded nmethods at the unloaded -> zombie transition in the sweeper. I also modified nmethod::cleanup_inline_caches() to not emit any transition stubs if the nmethod is already dead.
> 
> As Martin Doerr pointed out in another thread, we have to be careful with accessing CompiledIC::cached_metadata() of unloaded nmethods. For example, the following scenario may happen (IC of A references B):
> 
>     state of A       state of B
> -------------------------------
>     not-entrant
> S   [not-on-stack]      
> S   zombie           
> 	             unloaded
> 
> Now the IC of A still references the unloaded nmethod B and is_call_to_compiled() will access the unloaded metadata. I fixed this by checking caller->is_alive().
> 
> Thanks,
> Tobias
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8075805
> 
>