RFR: 8209189: Make CompiledMethod::do_unloading more concurrent

[Vladimir Kozlov]

Hi Erik,

First, what testing you did? This is significant rewrite and requires a lot of testing. I don't see a link to test 
results in RFE. Any performance testing?

It seems you removed the only case (in do_unloading()) where CodeCache::do_unloading_nmethod_caches() is called. I can't 
find other place where it is called after your change. Why keep it then?

It seems CompiledMethod::is_unloading() is performance critical. Can we make 
IsUnloadingBehaviour::current()->is_unloading(this) as non virtual call? I see only ClosureIsUnloadingBehaviour subclass 
of IsUnloadingBehaviour.

Thanks,
Vladimir

On 10/19/18 7:22 AM, Erik Österlund wrote:
> Hi,
> 
> Today the nmethods are unloaded either serially or in parallel due to GC (and e.g. class unloading). With ZGC concurrent 
> class unloading, a concurrent fashion is required. Rather than inventing yet a third way of unloading nmethods due to 
> class unloading, it would be ideal if there could be one unified way of doing it that makes everyone happy.
> 
> To solve this problem in a more general way, a new member function on CompiledMethod is added: is_unloading(). It 
> returns whether a CompiledMethod has broken oops. In order to not have to iterate over all the oops every time this 
> question is asked, it caches the result, so it needs to be computed only once per "epoch". Every time a CodeCache 
> unloading is triggered by the GC, a new epoch is started, which forces re-computation of whether the CompiledMethod 
> is_unloading() the first time it is called.
> 
> So by having is_unloading() be lazily evaluated, it is now possible to build a do_unloading() method on nmethod that 
> simply makes the nmethod unloaded if it is_unloading(), and otherwise cleans the various caches. Cleaning e.g. the 
> inline caches of said nmethod, uses the same is_unloading() method on its targets to figure out if the IC cache should 
> be cleaned or not. Again, the epoched caching mechanism makes sure we don't recompute this value.
> 
> The new do_unloading() method may be called in both serial, parallel and concurrent contexts. So I removed the parallel 
> variation of this that we had before, that unnecessarily postponed the unloading due to not having computed whether the 
> nmethod should be unloaded or not. Since that is now computed on-demand lazily, there is no longer a need for postponing 
> this, nor to have two phases for parallel nmethod unloading.
> 
> While there is more work involved in making concurrent nmethod unloading work, this is a good cleanup towards that goal.
> 
> Bug ID:
> https://bugs.openjdk.java.net/browse/JDK-8209189
> 
> Webrev:
> cr.openjdk.java.net/~eosterlund/8209189/webrev.00/
> 
> Thanks,
> /Erik