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

[Vladimir Kozlov]

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/
>