[13] RFR(L) 8220623: [JVMCI] Update JVMCI to support JVMCI based Compiler compiled into shared library

[Vladimir Kozlov]

Hi Stefan,

I collected some data on MetadataHandleBlock.

First, do_unloading() code is executed only when class_unloading_occurred is 'true' - it is rare 
case. It should not affect normal G1 remark pause.

Second, I run a test with -Xcomp. I got about 10,000 compilations by Graal and next data at the end 
of execution:

max_blocks = 232
max_handles_per_block = 32 (since handles array has 32 elements)
max_total_alive_values = 4631

Thanks,
Vladimir

On 3/28/19 2:44 PM, Vladimir Kozlov wrote:
> Thank you, Stefan
> 
> On 3/28/19 12:54 PM, Stefan Karlsson wrote:
>> Hi Vladimir,
>>
>> I started to check the GC code.
>>
>> ========================================================================
>> I see that you've added guarded includes in the middle of the include list:
>>    #include "gc/shared/strongRootsScope.hpp"
>>    #include "gc/shared/weakProcessor.hpp"
>> + #if INCLUDE_JVMCI
>> + #include "jvmci/jvmci.hpp"
>> + #endif
>>    #include "oops/instanceRefKlass.hpp"
>>    #include "oops/oop.inline.hpp"
>>
>> The style we use is to put these conditional includes at the end of the include lists.
> 
> okay
> 
>>
>> ========================================================================
>> Could you also change the following:
>>
>> + #if INCLUDE_JVMCI
>> +     // Clean JVMCI metadata handles.
>> +     JVMCI::do_unloading(is_alive_closure(), purged_class);
>> + #endif
>>
>> to:
>> +     // Clean JVMCI metadata handles.
>> +     JVMCI_ONLY(JVMCI::do_unloading(is_alive_closure(), purged_class);)
>>
>> to get rid of some of the line noise in the GC files.
> 
> okay
> 
>>
>> ========================================================================
>> In the future we will need version of JVMCI::do_unloading that supports concurrent cleaning for ZGC.
> 
> Yes, we need to support concurrent cleaning in a future.
> 
>>
>> ========================================================================
>> What's the performance impact for G1 remark pause with this serial walk over the MetadataHandleBlock?
>>
>> 3275 void G1CollectedHeap::complete_cleaning(BoolObjectClosure* is_alive,
>> 3276                                         bool class_unloading_occurred) {
>> 3277   uint num_workers = workers()->active_workers();
>> 3278   ParallelCleaningTask unlink_task(is_alive, num_workers, class_unloading_occurred, false);
>> 3279   workers()->run_task(&unlink_task);
>> 3280 #if INCLUDE_JVMCI
>> 3281   // No parallel processing of JVMCI metadata handles for now.
>> 3282   JVMCI::do_unloading(is_alive, class_unloading_occurred);
>> 3283 #endif
>> 3284 }
> 
> There should not be impact if Graal is not used. Only cost of call (which most likely is inlined in 
> product VM) and check:
> 
> http://hg.openjdk.java.net/metropolis/dev/file/530fc1427d02/src/hotspot/share/jvmci/jvmciRuntime.cpp#l1237 
> 
> 
> If Graal is used it should not have big impact since these metadata has regular pattern (32  handles 
> per array and array per MetadataHandleBlock block which are linked in list) and not large.
> If there will be noticeable impact - we will work on it as you suggested by using ParallelCleaningTask.
> 
>>
>> ========================================================================
>> Did you consider adding it as a task for one of the worker threads to execute in 
>> ParallelCleaningTask?
>>
>> See how other tasks are claimed by one worker:
>> void KlassCleaningTask::work() {
>>    ResourceMark rm;
>>
>>    // One worker will clean the subklass/sibling klass tree.
>>    if (claim_clean_klass_tree_task()) {
>>      Klass::clean_subklass_tree();
>>    }
> 
> These changes were ported from JDK8u based changes in graal-jvmci-8 and there are no 
> ParallelCleaningTask in JDK8.
> 
> Your suggestion is interesting and I agree that we should investigate it.
> 
>>
>> ========================================================================
>> In MetadataHandleBlock::do_unloading:
>>
>> +        if (klass->class_loader_data()->is_unloading()) {
>> +          // This needs to be marked so that it's no longer scanned
>> +          // but can't be put on the free list yet. The
>> +          // ReferenceCleaner will set this to NULL and
>> +          // put it on the free list.
>>
>> I couldn't find the ReferenceCleaner in the patch or in the source. Where can I find this code?
> 
> I think it is typo (I will fix it) - it references new HandleCleaner class:
> 
> http://cr.openjdk.java.net/~kvn/8220623/webrev.03/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HandleCleaner.java.html 
> 
> 
> Thanks,
> Vladimir
> 
>>
>> Thanks,
>> StefanK
>>
>> On 2019-03-28 20:15, Vladimir Kozlov wrote:
>>> https://bugs.openjdk.java.net/browse/JDK-8220623
>>> http://cr.openjdk.java.net/~kvn/8220623/webrev.03/
>>>
>>> Update JVMCI to support pre-compiled as shared library Graal.
>>> Using aoted Graal can offers benefits including:
>>>  - fast startup
>>>  - compile time similar to native JIt compilers (C2)
>>>  - memory usage disjoint from the application Java heap
>>>  - no profile pollution of JDK code used by the application
>>>
>>> This is JDK13 port of JVMCI changes done in graal-jvmci-8 [1] up to date.
>>> Changes were collected in Metropolis repo [2] and tested there.
>>>
>>> Changes we reviewed by Oracle Labs (authors of JVMCI and Graal) and our compiler group.
>>> Changes in shared code are guarded by #if INCLUDE_JVMCI and JVMCI flags.
>>>
>>> I ran tier1-tier8 (which includes HotSpot and JDK tests) and it was clean. In this set Graal was 
>>> tested only in tier3.
>>>
>>> And I ran all hs-tier3-graal .. hs-tier8-graal Graal tests available in our system. Several issue 
>>> were found which were present before these changes.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> [1] https://github.com/graalvm/graal-jvmci-8/commit/49ff2045fb603e35516a3a427d8023c00e1607af
>>> [2] http://hg.openjdk.java.net/metropolis/dev/
>>