(S) 8237750: Load libzip.so only if necessary
Yumin Qi
yumin.qi at oracle.com
Tue May 5 22:30:02 UTC 2020
Ioi, Calvin
Thanks. The inline sometime prefix to function declaration sometimes
not. I will put inline without updating webrev.
Thanks
Yumin
On 5/5/20 2:23 PM, Ioi Lam wrote:
> Hi Yumin,
>
> 258 static void load_zip_library_if_needed();
>
> I think "inline" should be added to the declaration. It probably
> doesn't matter, but we usually add 'inline' to the declaration of
> functions that appear in xxx_inline.hpp files.
>
> Thanks
> - Ioi
>
> On 5/5/20 1:32 PM, Yumin Qi wrote:
>> Hi, Ioi and Calvin
>>
>> There is an file classLoader.inline.hpp which also includes
>> atomic.hpp. I put load_zip_library_if_needed in it. Updated webrev:
>> http://cr.openjdk.java.net/~minqi/8237750/webrev-03/
>> Also, double checked the function is not in libjvm.so by
>> nm libjvm.so | grep load_zip_library_if_needed
>>
>> Thanks
>> Yumin
>>
>> On 5/5/20 10:59 AM, Ioi Lam wrote:
>>> Hi Yumin,
>>>
>>> Looks good.
>>>
>>> Just small nits. No need for new webrev.
>>>
>>> [1] I think this should be named _libzip_loaded to be consistent
>>> with other fields
>>>
>>> static int libzip_loaded; // used to sync loading zip.
>>>
>>>
>>> [2] This introduces dependency on atomic.hpp to classLoader.hpp
>>>
>>> static void load_zip_library_if_needed() {
>>> if (Atomic::load_acquire(&libzip_loaded) == 0) {
>>> release_load_zip_library();
>>> }
>>> }
>>>
>>> Since this function is used only privately in the cpp file, I think
>>> it's better to change the hpp to this (to reduce unnecessary header
>>> file dependencies)
>>>
>>> inline static void load_zip_library_if_needed();
>>>
>>> and then move the implementation to the classLoader.cpp file. You
>>> also need to add include "runtime/atomic.hpp" to classLoader.cpp.
>>>
>>> Thanks
>>> - Ioi
>>>
>>>
>>>
>>>
>>>
>>> On 5/4/20 4:40 PM, Yumin Qi wrote:
>>>> Hi, Ioi
>>>>
>>>> Thanks. Updated webrev at:
>>>> http://cr.openjdk.java.net/~minqi/8237750/webrev-02/
>>>> libzip_loaded now is a class member of ClassLoader, no longer a
>>>> file private integer since it is used in both .hpp and .cpp files.
>>>>
>>>> Thanks
>>>> Yumin
>>>>
>>>> On 5/4/20 8:31 AM, Ioi Lam wrote:
>>>>> Hi Yumin,
>>>>>
>>>>> 977 void ClassLoader::load_zip_library() {
>>>>> 978 if (Atomic::load_acquire(&libzip_loaded) == 0) {
>>>>> 979 MutexLocker locker(Zip_lock,
>>>>> Monitor::_no_safepoint_check_flag);
>>>>> 980 if (libzip_loaded == 0) {
>>>>> 981 load_zip_library0();
>>>>> 982 Atomic::release_store(&libzip_loaded, 1);
>>>>> 983 }
>>>>> 984 }
>>>>> 985 }
>>>>>
>>>>> 1026 int ClassLoader::crc32(int crc, const char* buf, int len) {
>>>>> 1027 if (libzip_loaded == 0) {
>>>>> 1028 load_zip_library();
>>>>> 1029 }
>>>>> 1030 return (*Crc32)(crc, (const jbyte*)buf, len);
>>>>> 1031 }
>>>>>
>>>>> ClassLoader::crc32 access libzip_loaded without a memory barrier,
>>>>> so it may read libzip_loaded out-of-order from Crc32. This means,
>>>>> thread 1 may be writing the memory in this order:
>>>>>
>>>>> Crc32 = <addr>;
>>>>> libzip_loaded = 1;
>>>>>
>>>>> but the order observed in thread 2 may be
>>>>>
>>>>> libzip_loaded = 1;
>>>>> >> ClassLoader::crc32 called here in thread 2
>>>>> Crc32 = <addr>;
>>>>>
>>>>> as a result, thread 2 may read libzip_loaded = 1, but still reads
>>>>> a NULL from Crc32.
>>>>>
>>>>> To ensure the memory barrier is used, I think we should do this:
>>>>>
>>>>> // private
>>>>> inline void ClassLoader::load_zip_library_if_needed() {
>>>>> if (Atomic::load_acquire(&libzip_loaded) == 0) {
>>>>> release_load_zip_library();
>>>>> }
>>>>> }
>>>>>
>>>>> // private
>>>>> void ClassLoader::release_load_zip_library() {
>>>>> MutexLocker locker(Zip_lock, Monitor::_no_safepoint_check_flag);
>>>>> if (libzip_loaded == 0) {
>>>>> load_zip_library0();
>>>>> Atomic::release_store(&libzip_loaded, 1);
>>>>> }
>>>>> }
>>>>>
>>>>> int ClassLoader::crc32(int crc, const char* buf, int len) {
>>>>> load_zip_library_if_needed();
>>>>> return (*Crc32)(crc, (const jbyte*)buf, len);
>>>>> }
>>>>>
>>>>> HotSpot code uses the "release_" prefix to indicate that the
>>>>> function must be used as a part of an acquire/release memory
>>>>> barrier. (E.g., InstanceKlass::release_set_array_klasses()).
>>>>>
>>>>> Some backgrounds:
>>>>> https://preshing.com/20130922/acquire-and-release-fences/
>>>>>
>>>>> Thanks
>>>>> - Ioi
>>>>>
>>>>>
>>>>>
>>>>> On 5/1/20 8:00 PM, Yumin Qi wrote:
>>>>>> Dean,
>>>>>>
>>>>>> I have updated to use MutexLocker instead at same link:
>>>>>> http://cr.openjdk.java.net/~minqi/8237750/webrev-01/
>>>>>>
>>>>>> Tested locally, passed jtreg/runtime.
>>>>>>
>>>>>> Thanks
>>>>>> Yumin
>>>>>>
>>>>>> On 5/1/20 4:24 PM, Dean Long wrote:
>>>>>>> OK, I didn't realize compute_fingerprint is using ZIP_CRC32.
>>>>>>>
>>>>>>> dl
>>>>>>>
>>>>>>> On 5/1/20 2:42 PM, Yumin Qi wrote:
>>>>>>>> Hi, Dean
>>>>>>>> Thanks for the review. I will try MutextLocker, for AOT, I
>>>>>>>> think currently the tests are not using CDS then it will load
>>>>>>>> classes from stream, that will use libzip's Crc32.
>>>>>>>> I will check for AOT to see if it really loads libzip with
>>>>>>>> the patch. For test compiler/aot/DeoptimizationTest.java:
>>>>>>>>
>>>>>>>> #0 ClassLoader::load_zip_library () at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:978
>>>>>>>>
>>>>>>>> #1 0x00007ffff57d4693 in ClassLoader::crc32 (crc=0,
>>>>>>>> buf=0x7ffff0244ee0 "\312\376\272\276", len=1888) at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:1028
>>>>>>>>
>>>>>>>> #2 0x00007ffff57cef5d in ClassFileStream::compute_fingerprint
>>>>>>>> (this=0x7ffff0245640) at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classFileStream.cpp:80
>>>>>>>> #3 0x00007ffff57c40ed in
>>>>>>>> ClassFileParser::create_instance_klass (this=0x7ffff7fc6fc0,
>>>>>>>> changed_by_loadhook=false, cl_inst_info=...,
>>>>>>>> __the_thread__=0x7ffff0033000) at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classFileParser.cpp:5630
>>>>>>>>
>>>>>>>> #4 0x00007ffff5dea647 in KlassFactory::create_from_stream
>>>>>>>> (stream=0x7ffff0245640, name=0x7ffff40550f0,
>>>>>>>> loader_data=0x7ffff022dbc0, cl_info=...,
>>>>>>>> __the_thread__=0x7ffff0033000)
>>>>>>>> at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/klassFactory.cpp:207
>>>>>>>> #5 0x00007ffff57d53e4 in ClassLoader::load_class
>>>>>>>> (name=0x7ffff40550f0, search_append_only=false,
>>>>>>>> __the_thread__=0x7ffff0033000) at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:1285
>>>>>>>> #6 0x00007ffff6234fcf in SystemDictionary::load_instance_class
>>>>>>>> (class_name=0x7ffff40550f0, class_loader=...,
>>>>>>>> __the_thread__=0x7ffff0033000) at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:1550
>>>>>>>> #7 0x00007ffff6232d0a in
>>>>>>>> SystemDictionary::resolve_instance_class_or_null
>>>>>>>> (name=0x7ffff40550f0, class_loader=..., protection_domain=...,
>>>>>>>> __the_thread__=0x7ffff0033000)
>>>>>>>> at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:882
>>>>>>>> #8 0x00007ffff623137e in
>>>>>>>> SystemDictionary::resolve_instance_class_or_null_helper
>>>>>>>> (class_name=0x7ffff40550f0, class_loader=...,
>>>>>>>> protection_domain=..., __the_thread__=0x7ffff0033000)
>>>>>>>> at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:302
>>>>>>>> #9 0x00007ffff623124c in SystemDictionary::resolve_or_null
>>>>>>>> (class_name=0x7ffff40550f0, class_loader=...,
>>>>>>>> protection_domain=..., __the_thread__=0x7ffff0033000) at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:285
>>>>>>>>
>>>>>>>> #10 0x00007ffff6230f57 in SystemDictionary::resolve_or_fail
>>>>>>>> (class_name=0x7ffff40550f0, class_loader=...,
>>>>>>>> protection_domain=..., throw_error=true,
>>>>>>>> __the_thread__=0x7ffff0033000)
>>>>>>>> at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:233
>>>>>>>> #11 0x00007ffff62311eb in SystemDictionary::resolve_or_fail
>>>>>>>> (class_name=0x7ffff40550f0, throw_error=true,
>>>>>>>> __the_thread__=0x7ffff0033000) at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:275
>>>>>>>> #12 0x00007ffff6236722 in SystemDictionary::resolve_wk_klass
>>>>>>>> (id=SystemDictionary::Object_klass_knum,
>>>>>>>> __the_thread__=0x7ffff0033000) at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:2029
>>>>>>>> #13 0x00007ffff623681e in
>>>>>>>> SystemDictionary::resolve_wk_klasses_until
>>>>>>>> (limit_id=SystemDictionary::Cloneable_klass_knum,
>>>>>>>> start_id=@0x7ffff7fc79d4: SystemDictionary::Object_klass_knum,
>>>>>>>> __the_thread__=0x7ffff0033000)
>>>>>>>> at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:2039
>>>>>>>> #14 0x00007ffff623ac01 in
>>>>>>>> SystemDictionary::resolve_wk_klasses_through
>>>>>>>> (end_id=SystemDictionary::Class_klass_knum,
>>>>>>>> start_id=@0x7ffff7fc79d4: SystemDictionary::Object_klass_knum,
>>>>>>>> __the_thread__=0x7ffff0033000)
>>>>>>>> at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.hpp:418
>>>>>>>> #15 0x00007ffff62369b0 in
>>>>>>>> SystemDictionary::resolve_well_known_classes
>>>>>>>> (__the_thread__=0x7ffff0033000) at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:2082
>>>>>>>> #16 0x00007ffff6236517 in SystemDictionary::initialize
>>>>>>>> (__the_thread__=0x7ffff0033000) at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:1985
>>>>>>>> #17 0x00007ffff62afe15 in Universe::genesis
>>>>>>>> (__the_thread__=0x7ffff0033000) at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/hotspot/share/memory/universe.cpp:326
>>>>>>>> #18 0x00007ffff62b1e51 in universe2_init () at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/hotspot/share/memory/universe.cpp:847
>>>>>>>> #19 0x00007ffff5af21ed in init_globals () at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/hotspot/share/runtime/init.cpp:128
>>>>>>>> #20 0x00007ffff627feff in Threads::create_vm
>>>>>>>> (args=0x7ffff7fc7e50, canTryAgain=0x7ffff7fc7d5b) at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/hotspot/share/runtime/thread.cpp:3879
>>>>>>>> #21 0x00007ffff5bf537d in JNI_CreateJavaVM_inner
>>>>>>>> (vm=0x7ffff7fc7ea8, penv=0x7ffff7fc7eb0, args=0x7ffff7fc7e50)
>>>>>>>> at /home/minqi/ws/jdk15/jdk/src/hotspot/share/prims/jni.cpp:3789
>>>>>>>> #22 0x00007ffff5bf5677 in JNI_CreateJavaVM (vm=0x7ffff7fc7ea8,
>>>>>>>> penv=0x7ffff7fc7eb0, args=0x7ffff7fc7e50) at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/hotspot/share/prims/jni.cpp:3872
>>>>>>>> #23 0x00007ffff7bca4c9 in InitializeJVM (pvm=0x7ffff7fc7ea8,
>>>>>>>> penv=0x7ffff7fc7eb0, ifn=0x7ffff7fc7f00) at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/java.base/share/native/libjli/java.c:1538
>>>>>>>>
>>>>>>>> #24 0x00007ffff7bc70b5 in JavaMain (_args=0x7fffffffab10) at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/java.base/share/native/libjli/java.c:417
>>>>>>>>
>>>>>>>> #25 0x00007ffff7bceb5f in ThreadJavaMain (args=0x7fffffffab10)
>>>>>>>> at
>>>>>>>> /home/minqi/ws/jdk15/jdk/src/java.base/unix/native/libjli/java_md_solinux.c:734
>>>>>>>>
>>>>>>>> #26 0x00007ffff71c56ba in start_thread (arg=0x7ffff7fc8700) at
>>>>>>>> pthread_create.c:333
>>>>>>>> #27 0x00007ffff790041d in clone () at
>>>>>>>> ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
>>>>>>>>
>>>>>>>> This is not with CDS.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Yumin
>>>>>>>>
>>>>>>>> On 5/1/20 2:11 PM, Dean Long wrote:
>>>>>>>>> It looks like Zip_lock could be a MutexLocker instead of a
>>>>>>>>> MonitorLocker.
>>>>>>>>>
>>>>>>>>> dl
>>>>>>>>>
>>>>>>>>> On 5/1/20 10:24 AM, Yumin Qi wrote:
>>>>>>>>>> Hi, please review:
>>>>>>>>>> bug 8237750: https://bugs.openjdk.java.net/browse/JDK-8237750
>>>>>>>>>> webrev: http://cr.openjdk.java.net/~minqi/8237750/webrev-01/
>>>>>>>>>> Summary:
>>>>>>>>>> zip library is loaded unconditionally at start up but it is
>>>>>>>>>> only needed when
>>>>>>>>>> 1) bootclasspath contains zip or
>>>>>>>>>> 2) UseAOT enabled or
>>>>>>>>>> 3) VerifySharedArchive turned on or
>>>>>>>>>> 4) CDS archives custom loaded classes
>>>>>>>>>> If none of above in java application, it is not necessary
>>>>>>>>>> to have zip library loaded.
>>>>>>>>>>
>>>>>>>>>> Solution by loading zip library on demand when needed.
>>>>>>>>>>
>>>>>>>>>> Performance for java -Xint version:
>>>>>>>>>>
>>>>>>>>>> Results of " perf stat -r 50 bin/java -Xshare:on
>>>>>>>>>> -XX:SharedArchiveFile=jdk2.jsa -Xint -version "
>>>>>>>>>> 1: 59611556 59450206 (-161350) ----- 39.799 40.274
>>>>>>>>>> ( 0.475) ++
>>>>>>>>>> 2: 59602708 59425234 (-177474) ----- 40.591 41.183
>>>>>>>>>> ( 0.592) ++
>>>>>>>>>> 3: 59579718 59441272 (-138446) ---- 40.777 40.471 (
>>>>>>>>>> -0.306) -
>>>>>>>>>> 4: 59584882 59410155 (-174727) ----- 40.824 40.233
>>>>>>>>>> ( -0.591) --
>>>>>>>>>> 5: 59590998 59447252 (-143746) ---- 40.400 40.493
>>>>>>>>>> ( 0.093)
>>>>>>>>>> 6: 59589523 59441934 (-147589) ---- 40.475 40.064 (
>>>>>>>>>> -0.411) --
>>>>>>>>>> 7: 59581820 59413612 (-168208) ----- 39.763 40.077
>>>>>>>>>> ( 0.314) +
>>>>>>>>>> 8: 59593678 59418738 (-174940) ----- 40.912 39.724
>>>>>>>>>> ( -1.188) -----
>>>>>>>>>> 9: 59573058 59412554 (-160504) ----- 40.126 40.033
>>>>>>>>>> ( -0.093)
>>>>>>>>>> 10: 59591469 59419291 (-172178) ----- 40.731 40.689
>>>>>>>>>> ( -0.042)
>>>>>>>>>> ============================================================
>>>>>>>>>> 59589940 59428022 (-161917) ----- 40.438 40.322
>>>>>>>>>> ( -0.116)
>>>>>>>>>> instr delta = -161917 -0.2717%
>>>>>>>>>> time delta = -0.116 ms -0.2859%
>>>>>>>>>>
>>>>>>>>>> Tests: hs-tier1-4.
>>>>>>>>>> Due to zip library not loaded at default, I removed 'zip'
>>>>>>>>>> from pmap list in test case:
>>>>>>>>>> *test/hotspot/jtreg/serviceability/sa/ClhsdbPmap.java
>>>>>>>>>>
>>>>>>>>>> **
>>>>>>>>>> * Thanks
>>>>>>>>>> Yumin
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list