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
