[13] RFR(L) 8220623: [JVMCI] Update JVMCI to support JVMCI based Compiler compiled into shared library
Vladimir Kozlov
vladimir.kozlov at oracle.com
Wed Apr 10 02:25:35 UTC 2019
Thank you, Coleen
On 4/9/19 1:36 PM, coleen.phillimore at oracle.com wrote:
>
> I think I missed graal-dev with this reply. I have a few other comments.
>
> +void MetadataHandleBlock::do_unloading(BoolObjectClosure* is_alive) {
>
>
> We've removed the is_alive parameter from all do_unloading, and it appears unused here also.
Yes, I can remove it.
>
> I don't know about this MetadataHandles block. It seems that it could be a concurrent hashtable
> with a WeakHandle<> if it's for jdk11 and beyond. Kim might have mentioned this (I haven't read all
> the replies thoroughly) but JNIHandleBlock wasn't MT safe, and the new OopStorage is safe and scalable.
Yes, Kim also suggested OopStorage. I did not get into that part yet but I will definitely do.
>
> + jmetadata allocate_handle(methodHandle handle) { return allocate_metadata_handle(handle()); }
> + jmetadata allocate_handle(constantPoolHandle handle) { return allocate_metadata_handle(handle()); }
>
> +CompLevel JVMCI::adjust_comp_level(methodHandle method, bool is_osr, CompLevel level, JavaThread*
> thread) {
>
> +JVMCIObject JVMCIEnv::new_StackTraceElement(methodHandle method, int bci, JVMCI_TRAPS) {
>
> +JVMCIObject JVMCIEnv::new_HotSpotNmethod(methodHandle method, const char* name, jboolean isDefault,
> jlong compileId, JVMCI_TRAPS) {
>
> Passing metadata Handles by copy will call the copy constructor and destructor for these parameters
> unnecessarily. They should be passed as *const* references to avoid this.
Okay.
>
> +class MetadataHandleBlock : public CHeapObj<mtInternal> {
>
>
> There should be a better mt? for this. mtCompiler seems appropriate here. Depending on how many
> others of these, you could add an mtJVMCI.
mtJVMCI is good suggestion.
>
> + if (TraceNMethodInstalls) {
>
>
> We've had Unified Logging in the sources for a long time now. New code should use UL rather than
> adding a TraceSomething option. I understand it's supposed to be shared with JDK8 code but it
> seems that you're forward porting what looks like old code into the repository.
Yes, we should use UL for this.
Existing JIT code (ciEnv.cpp) still not using UL for this:
http://hg.openjdk.java.net/jdk/jdk/file/f847a42ddc01/src/hotspot/share/ci/ciEnv.cpp#l1075
May be I should update it too ...
>
> Coleen
>
>
> On 4/9/19 4:00 PM, coleen.phillimore at oracle.com wrote:
>>
>> http://cr.openjdk.java.net/~kvn/8220623/webrev.06/src/hotspot/share/classfile/classFileParser.cpp.udiff.html
>>
>>
>> It appears this change is to implement https://bugs.openjdk.java.net/browse/JDK-8193513 which we
>> closed as WNF. If you want this change, remove it from this giant patch and reopen and submit a
>> separate patch for this bug.
Thank you for pointing it. I will do as you suggested.
>>
>> It shouldn't be conditional on JVMCI and should use the normal unified logging mechanism.
Okay.
>>
>> http://cr.openjdk.java.net/~kvn/8220623/webrev.06/src/hotspot/share/runtime/thread.hpp.udiff.html
>>
>> *!_jlong__pending_failed_speculation;*
>>
>>
>> We've been trying to remove and avoid java types in hotspot code and use the appropriate C++ types
>> instead. Can this be changed to int64_t? 'long' is generally wrong though.
This field should be java type since it is accessed from Java Graal:
http://hg.openjdk.java.net/jdk/jdk/file/f847a42ddc01/src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java#l401
>>
>> I seem to remember there was code to deal with metadata in oops for redefinition, but I can't find
>> it in this big patch. I was going to look at that.
May be it is MetadataHandleBlock::metadata_do() (in jvmciRuntime.cpp)?
>>
>> Otherwise, I've reviewed the runtime code.
Thanks,
Vladimir
>>
>> Coleen
>>
>> On 4/4/19 3:22 AM, Vladimir Kozlov wrote:
>>> New delta:
>>> http://cr.openjdk.java.net/~kvn/8220623/webrev_delta.06/
>>>
>>> Full:
>>> http://cr.openjdk.java.net/~kvn/8220623/webrev.06/
>>>
>>> New changes are based on Kim and Stefan suggestions:
>>>
>>> - Moved JVMCI::oops_do() from JNIHandles to places where it should be called.
>>> - Moved JVMCI cleanup task to the beginning of ParallelCleaningTask::work().
>>> - Used JVMCI_ONLY macro with COMMA.
>>> - Disable JVMCI build on SPARC. We don't use it - neither Graal or AOT are built on SPARC.
>>> Disabling also helps to find missing JVMCI guards.
>>>
>>> I ran hs-tier1-3 testing - it passed (hs-tier3 includes graal testing).
>>> I started hs-tier4..8-graal testing.
>>> I will do performance testing next.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 4/3/19 9:54 AM, Vladimir Kozlov wrote:
>>>> On 4/2/19 11:35 PM, Stefan Karlsson wrote:
>>>>> On 2019-04-02 22:41, Vladimir Kozlov wrote:
>>>>>> I ran Kitchensink with G1 and -Xmx8g. I observed that Remark pause times are not consistent
>>>>>> even without Graal.
>>>>>> To see effect I added time spent in JVMCI::do_unloading() to GC log (see below [3]). The
>>>>>> result is < 1ms - it is less than 1% of a pause time.
>>>>>
>>>>> Kitchensink isn't really a benchmark, but a stress test. I sent you a private mail how to run
>>>>> these changes through our internal performance test setup.
>>>>
>>>> Okay, I will run performance tests there too.
>>>>
>>>>>
>>>>>>
>>>>>> It will have even less effect since I moved JVMCI::do_unloading() from serial path to parallel
>>>>>> worker thread as Stefan suggested.
>>>>>>
>>>>>> Stefan, are you satisfied with these changes now?
>>>>>
>>>>> Yes, the clean-ups look good. Thanks for cleaning this up.
>>>>>
>>>>> Kim had some extra comments about a few more places where JVMCI_ONLY could be used.
>>>>>
>>>>> I also agree with him that JVMCI::oops_do should not be placed in JNIHandles::oops_do. I think
>>>>> you should put it where you put the AOTLoader::oops_do calls.
>>>>
>>>> Okay.
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>>>
>>>>> Thanks,
>>>>> StefanK
>>>>>
>>>>>
>>>>>>
>>>>>> Here is latest delta update which includes previous [1] delta and
>>>>>> - use CompilerThreadStackSize * 2 for libgraal instead of exact value,
>>>>>> - removed HandleMark added for debugging (reverted changes in jvmtiImpl.cpp),
>>>>>> - added recent jvmci-8 changes to fix registration of native methods in libgraal
>>>>>> (jvmciCompilerToVM.cpp)
>>>>>>
>>>>>> http://cr.openjdk.java.net/~kvn/8220623/webrev_delta.05/
>>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>> [1] http://cr.openjdk.java.net/~kvn/8220623/webrev_delta.04/
>>>>>> [2] Original webrev http://cr.openjdk.java.net/~kvn/8220623/webrev.03/
>>>>>> [3] Pauses times from Kitchensink (0.0ms means there were no unloaded classes, 'NNN alive'
>>>>>> shows how many metadata references were processed):
>>>>>>
>>>>>> [1.083s][1554229160638ms][info ][gc,start ] GC(2) Pause Remark
>>>>>> [1.085s][1554229160639ms][info ][gc ] GC(2) JVMCI::do_unloading(): 0 alive 0.000ms
>>>>>> [1.099s][1554229160654ms][info ][gc ] GC(2) Pause Remark 28M->28M(108M) 16.123ms
>>>>>>
>>>>>> [3.097s][1554229162651ms][info ][gc,start ] GC(12) Pause Remark
>>>>>> [3.114s][1554229162668ms][info ][gc ] GC(12) JVMCI::do_unloading(): 3471 alive 0.164ms
>>>>>> [3.148s][1554229162702ms][info ][gc ] GC(12) Pause Remark 215M->213M(720M) 51.103ms
>>>>>>
>>>>>> [455.111s][1554229614666ms][info ][gc,phases,start] GC(1095) Phase 1: Mark live objects
>>>>>> [455.455s][1554229615010ms][info ][gc ] GC(1095) JVMCI::do_unloading(): 4048 alive
>>>>>> 0.821ms
>>>>>> [455.456s][1554229615010ms][info ][gc,phases ] GC(1095) Phase 1: Mark live objects 344.107ms
>>>>>>
>>>>>> [848.932s][1554230008486ms][info ][gc,phases,start] GC(1860) Phase 1: Mark live objects
>>>>>> [849.248s][1554230008803ms][info ][gc ] GC(1860) JVMCI::do_unloading(): 3266 alive
>>>>>> 0.470ms
>>>>>> [849.249s][1554230008803ms][info ][gc,phases ] GC(1860) Phase 1: Mark live objects 316.527ms
>>>>>>
>>>>>> [1163.778s][1554230323332ms][info ][gc,start ] GC(2627) Pause Remark
>>>>>> [1163.932s][1554230323486ms][info ][gc ] GC(2627) JVMCI::do_unloading(): 3474
>>>>>> alive 0.642ms
>>>>>> [1163.941s][1554230323496ms][info ][gc ] GC(2627) Pause Remark 2502M->2486M(4248M)
>>>>>> 163.296ms
>>>>>>
>>>>>> [1242.587s][1554230402141ms][info ][gc,phases,start] GC(2734) Phase 1: Mark live objects
>>>>>> [1242.899s][1554230402453ms][info ][gc ] GC(2734) JVMCI::do_unloading(): 3449
>>>>>> alive 0.570ms
>>>>>> [1242.899s][1554230402453ms][info ][gc,phases ] GC(2734) Phase 1: Mark live objects
>>>>>> 311.719ms
>>>>>>
>>>>>> [1364.164s][1554230523718ms][info ][gc,phases,start] GC(3023) Phase 1: Mark live objects
>>>>>> [1364.613s][1554230524167ms][info ][gc ] GC(3023) JVMCI::do_unloading(): 3449
>>>>>> alive 0.000ms
>>>>>> [1364.613s][1554230524167ms][info ][gc,phases ] GC(3023) Phase 1: Mark live objects
>>>>>> 448.495ms
>>>>>>
>>>>>> [1425.222s][1554230584776ms][info ][gc,phases,start] GC(3151) Phase 1: Mark live objects
>>>>>> [1425.587s][1554230585142ms][info ][gc ] GC(3151) JVMCI::do_unloading(): 3491
>>>>>> alive 0.882ms
>>>>>> [1425.587s][1554230585142ms][info ][gc,phases ] GC(3151) Phase 1: Mark live objects
>>>>>> 365.403ms
>>>>>>
>>>>>> [1456.401s][1554230615955ms][info ][gc,phases,start] GC(3223) Phase 1: Mark live objects
>>>>>> [1456.769s][1554230616324ms][info ][gc ] GC(3223) JVMCI::do_unloading(): 3478
>>>>>> alive 0.616ms
>>>>>> [1456.769s][1554230616324ms][info ][gc,phases ] GC(3223) Phase 1: Mark live objects
>>>>>> 368.643ms
>>>>>>
>>>>>> [1806.139s][1554230965694ms][info ][gc,start ] GC(4014) Pause Remark
>>>>>> [1806.161s][1554230965716ms][info ][gc ] GC(4014) JVMCI::do_unloading(): 3478
>>>>>> alive 0.000ms
>>>>>> [1806.163s][1554230965717ms][info ][gc ] GC(4014) Pause Remark
>>>>>> 1305M->1177M(2772M) 23.190ms
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 4/1/19 12:34 AM, Stefan Karlsson wrote:
>>>>>>> On 2019-03-29 17:55, Vladimir Kozlov wrote:
>>>>>>>> Stefan,
>>>>>>>>
>>>>>>>> Do you have a test (and flags) which can allow me to measure effect of this code on G1
>>>>>>>> remark pause?
>>>>>>>
>>>>>>>
>>>>>>> -Xlog:gc prints the remark times:
>>>>>>> [4,296s][info][gc ] GC(89) Pause Remark 4M->4M(28M) 36,412ms
>>>>>>>
>>>>>>> StefanK
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Vladimir
>>>>>>>>
>>>>>>>> On 3/29/19 12:36 AM, Stefan Karlsson wrote:
>>>>>>>>> On 2019-03-29 03:07, Vladimir Kozlov wrote:
>>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>> It's only rare for applications that don't do dynamic class loading and unloading. The
>>>>>>>>> applications that do, will be affected.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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
>>>>>>>>>
>>>>>>>>> OK. Thanks for the info.
>>>>>>>>>
>>>>>>>>> StefanK
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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/
>>>>>>>>>>>>
>>
>
More information about the hotspot-dev
mailing list