RFR 8227103: Shenandoah: Refactor ShenandoahNMethod in preparation of concurrent nmethod iteration

[Zhengyu Gu]

On 7/3/19 4:47 AM, Aleksey Shipilev wrote:
> On 7/2/19 6:51 PM, Zhengyu Gu wrote:
>> This refactor is one of subtasks for concurrent nmethod unloading [1].
>>
>> It moves ShenandoahNMethod out of ShenandoahCodeRoots, in preparation for more complicated
>> concurrent nmethod iteration.
>>
>> Borrowing what ZGC does, only records oops embedded in relocation stream to reduce memory footprint.
>> Also, attaches ShenandoahNMethod to nmethod's gc_data, so that it avoids additional searching during
>> re-register nmethod.
>>
>> Let's use shenandoah/jdk for concurrent nmethod unloading work, while we stabilize early concurrent
>> root work in jdk/jdk.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8227103
>> Webrev: http://cr.openjdk.java.net/~zgu/JDK-8227103/webrev.00/
> 
> I have several design concerns here:
> 
>   *) The ability to attach binary blob data to nmethod only exists since JDK 12 (JDK-8214056), which
> would break the backporting. Current code deliberately does its own thing, so that it works everywhere.Attaching ShenandoahNMethod to nmethod is needed for concurrent nmethod 
unloading. So, I guess there is no point to break down concurrent 
nmethod unloading changeset, seems JDK13 is the limit.

So I would like to withdraw this RFR.

> 
>   *) If you use nmethod data, you need to implement CollectedHeap::flush_nmethod that would delete
> ShenandoahNMethod, otherwise this would leak memory.

Yes. It will be deleted in flush_nmethod(). Concurrently it is deleted 
in unregister_nmethod().

> 
>   *) Scanning relocation stream only might be an interesting thing to do, but I think the performance
> improvements we would get from this does not justify the risks of avoiding "proper" oops_do,
> especially after code roots are scanned concurrently.
I don't get this. Concurrent scanning and updating oops array, etc. will 
always be protected by reentrant lock. I am not sure what "risks of 
avoiding "proper" oops_do means.

Most of generated methods (e.g. Lambda methods) only contain one 
embedded oop for keeping them alive, so creating oop array for them 
seems overkill.

> 
> Can you say what exactly do you need to enable "more complicated concurrent nmethod iteration"?

During unloading concurrent nmethod iteration, it can trigger nmethod 
unregistration by iterating threads, while sweep and compiler threads 
can register/unregister/flush nmethod concurrently.

Thanks,

-Zhengyu



> 
> -Aleksey
>