(S) 8237750: Load libzip.so only if necessary
Ioi Lam
ioi.lam at oracle.com
Mon May 4 15:31:02 UTC 2020
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 serviceability-dev
mailing list