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
