[13] RFR(L) 8220623: [JVMCI] Update JVMCI to support JVMCI based Compiler compiled into shared library
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Tue Apr 9 20:36:28 UTC 2019
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.
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.
+ 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.
+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.
+ 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.
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.
>
> It shouldn't be conditional on JVMCI and should use the normal unified
> logging mechanism.
>
> 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.
>
> 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.
>
> Otherwise, I've reviewed the runtime code.
>
> 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