Re: RFR: 8244078: ProcessTools executeTestJvm and createJavaProcessBuilder have inconsistent handling of test.*.opts
On 2020-05-01 21:34, Chris Plummer wrote: On 4/30/20 2:07 AM, Stefan Karlsson wrote: ... There was one odd thing in jdi that requires extra scrutiny: https://cr.openjdk.java.net/~stefank/8244078/webrev.01/test/jdk/com/sun/jdi/lib/jdb/Debuggee.java.udiff.html Yes, that did look a odd at first glance, but I think that fact that your removed addTestVmAndJavaOptions() and everything still built is proof enough that it was just bit rotted code and not needed. Thanks for checking on this. StefanK Chris I've run this through tier1-3, and are currently running this through higher tiers. Thanks, StefanK
RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)
Please review this change which implements part of JEP 381: JBS: https://bugs.openjdk.java.net/browse/JDK-8244224 webrev: http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/hotspot/open/webrev/ JEP: https://bugs.openjdk.java.net/browse/JDK-8241787 Note: When reviewing this, please be aware that this exercise was *extremely* mind-numbing, so I appreciate your help reviewing all the individual changes carefully. You may want to get that coffee cup filled up (or whatever keeps you awake)! Background: Because of the size of the total patch and wide range of areas touched, this patch is one out of in total six partial patches which together make up the necessary changes to remove the Solaris and SPARC ports. The other patches are being sent out for review to mailing lists appropriate for the respective areas the touch. An email will be sent to jdk-dev summarizing all the patches/reviews. To be clear: this patch is *not* in itself complete and stand-alone - all of the (six) patches are needed to form a complete patch. Some changes in this patch may look wrong or incomplete unless also looking at the corresponding changes in other areas. For convenience, I’m including a link below[1] to the full webrev, but in case you have comments on changes in other areas, outside of the files included in this thread, please provide those comments directly in the thread on the appropriate mailing list for that area if possible. In case it helps, the changes were effectively produced by searching for and updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”, etc. More information about the areas impacted can be found in the JEP itself. A big thank you to Igor Ignatyev for helping make the changes to the hotspot tests! Also, I have a short list of follow-ups which I’m going to look at separately from this JEP/patch, mainly related to command line options/flags which are no longer relevant and should be deprecated/obsoleted/removed. Testing: A slightly earlier version of this change successfully passed tier1-8, as well as client tier1-2. Additional testing will be done after the first round of reviews has been completed. Cheers, Mikael [1] http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/
RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (serviceability)
Please review this change which implements part of JEP 381: JBS: https://bugs.openjdk.java.net/browse/JDK-8244224 webrev: http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/serviceability/open/webrev/ JEP: https://bugs.openjdk.java.net/browse/JDK-8241787 Note: When reviewing this, please be aware that this exercise was *extremely* mind-numbing, so I appreciate your help reviewing all the individual changes carefully. You may want to get that coffee cup filled up (or whatever keeps you awake)! Background: Because of the size of the total patch and wide range of areas touched, this patch is one out of in total six partial patches which together make up the necessary changes to remove the Solaris and SPARC ports. The other patches are being sent out for review to mailing lists appropriate for the respective areas the touch. An email will be sent to jdk-dev summarizing all the patches/reviews. To be clear: this patch is *not* in itself complete and stand-alone - all of the (six) patches are needed to form a complete patch. Some changes in this patch may look wrong or incomplete unless also looking at the corresponding changes in other areas. For convenience, I’m including a link below[1] to the full webrev, but in case you have comments on changes in other areas, outside of the files included in this thread, please provide those comments directly in the thread on the appropriate mailing list for that area if possible. In case it helps, the changes were effectively produced by searching for and updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”, etc. More information about the areas impacted can be found in the JEP itself. Testing: A slightly earlier version of this change successfully passed tier1-8, as well as client tier1-2. Additional testing will be done after the first round of reviews has been completed. Cheers, Mikael [1] http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/
Re: (S) 8237750: Load libzip.so only if necessary
Hi Dean,
The code uses double-checked locking so we can be thread-safe when
loading the zip library, while avoiding using the MutexLock every time:
https://en.wikipedia.org/wiki/Double-checked_locking
To work correctly on modern memory architectures, we need the memory
barriers:
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 }
(Atomic::load_acquire is much cheaper than MutexLocker).
If we read Crc32 without memory barriers, as in the original code, in
two concurrent threads, I think there may be a chance for one of the
threads to read an invalid value of Crc32.
int ClassLoader::crc32(int crc, const char* buf, int len) {
return (*Crc32)(crc, (const jbyte*)buf, len);
}
Thanks
- Ioi
On 5/2/20 12:05 AM, Dean Long wrote:
In ClassLoader::crc32, instead of checking if the library is loaded
every time, how about initializing the Crc32 function pointer to a
function that calls load_zip_library()?
Something like:
static jint initial_Crc32(jint crc, const jbyte *buf, jint len) {
load_zip_library();
assert(Crc32 != initial_Crc32, "load_zip_library did not update
Crc32");
return crc32(crc, buf, len); // ZIP_CRC32
}
static Crc32_t Crc32 = initial_Crc32;
I also suggest putting
ClassLoader::crc32 in classLoader.hpp so it can be inlined.
dl
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 0x757d4693 in ClassLoader::crc32 (crc=0,
buf=0x70244ee0 "\312\376\272\276", len=1888) at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:1028
#2 0x757cef5d in ClassFileStream::compute_fingerprint
(this=0x70245640) at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classFileStream.cpp:80
#3 0x757c40ed in ClassFileParser::create_instance_klass
(this=0x77fc6fc0, changed_by_loadhook=false, cl_inst_info=...,
__the_thread__=0x70033000) at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classFileParser.cpp:5630
#4 0x75dea647 in KlassFactory::create_from_stream
(stream=0x70245640, name=0x740550f0,
loader_data=0x7022dbc0, cl_info=...,
__the_thread__=0x70033000)
at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/klassFactory.cpp:207
#5 0x757d53e4 in ClassLoader::load_class
(name=0x740550f0, search_append_only=false,
__the_thread__=0x70033000) at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:1285
#6 0x76234fcf in SystemDictionary::load_instance_class
(class_name=0x740550f0, class_loader=...,
__the_thread__=0x70033000) at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:1550
#7 0x76232d0a in
SystemDictionary::resolve_instance_class_or_null
(name=0x740550f0, class_loader=..., protection_domain=...,
__the_thread__=0x70033000)
at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:882
#8 0x7623137e in
SystemDictionary::resolve_instance_class_or_null_helper
(class_name=0x740550f0, class_loader=...,
protection_domain=..., __the_thread__=0x70033000)
at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:302
#9 0x7623124c in SystemDictionary::resolve_or_null
(class_name=0x740550f0, class_loader=...,
protection_domain=..., __the_thread__=0x70033000) at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:285
#10 0x76230f57 in SystemDictionary::resolve_or_fail
(class_name=0x740550f0, class_loader=...,
protection_domain=..., throw_error=true,
__the_thread__=0x70033000)
at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:233
#11 0x762311eb in SystemDictionary::resolve_or_fail
(class_name=0x740550f0, throw_error=true,
__the_thread__=0x70033000) at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:275
#12 0x76236722 in SystemDictionary::resolve_wk_klass
(id=SystemDictionary::Object_kl
Re: (S) 8237750: Load libzip.so only if necessary
Hi Dean,
The code uses double-checked locking so we can be thread-safe when
loading the zip library, while avoiding using the MutexLock every time:
https://en.wikipedia.org/wiki/Double-checked_locking
To work correctly on modern memory architectures, we need the memory
barriers:
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 }
(Atomic::load_acquire is much cheaper than MutexLocker).
If we read Crc32 without memory barriers, as in the original code, in
two concurrent threads, I think there may be a chance for one of the
threads to read an invalid value of Crc32.
int ClassLoader::crc32(int crc, const char* buf, int len) {
return (*Crc32)(crc, (const jbyte*)buf, len);
}
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 0x757d4693 in ClassLoader::crc32 (crc=0,
buf=0x70244ee0 "\312\376\272\276", len=1888) at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:1028
#2 0x757cef5d in ClassFileStream::compute_fingerprint
(this=0x70245640) at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classFileStream.cpp:80
#3 0x757c40ed in ClassFileParser::create_instance_klass
(this=0x77fc6fc0, changed_by_loadhook=false, cl_inst_info=...,
__the_thread__=0x70033000) at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classFileParser.cpp:5630
#4 0x75dea647 in KlassFactory::create_from_stream
(stream=0x70245640, name=0x740550f0,
loader_data=0x7022dbc0, cl_info=..., __the_thread__=0x70033000)
at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/klassFactory.cpp:207
#5 0x757d53e4 in ClassLoader::load_class
(name=0x740550f0, search_append_only=false,
__the_thread__=0x70033000) at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:1285
#6 0x76234fcf in SystemDictionary::load_instance_class
(class_name=0x740550f0, class_loader=...,
__the_thread__=0x70033000) at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:1550
#7 0x76232d0a in
SystemDictionary::resolve_instance_class_or_null
(name=0x740550f0, class_loader=..., protection_domain=...,
__the_thread__=0x70033000)
at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:882
#8 0x7623137e in
SystemDictionary::resolve_instance_class_or_null_helper
(class_name=0x740550f0, class_loader=..., protection_domain=...,
__the_thread__=0x70033000)
at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:302
#9 0x7623124c in SystemDictionary::resolve_or_null
(class_name=0x740550f0, class_loader=..., protection_domain=...,
__the_thread__=0x70033000) at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:285
#10 0x76230f57 in SystemDictionary::resolve_or_fail
(class_name=0x740550f0, class_loader=..., protection_domain=...,
throw_error=true, __the_thread__=0x70033000)
at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:233
#11 0x762311eb in SystemDictionary::resolve_or_fail
(class_name=0x740550f0, throw_error=true,
__the_thread__=0x70033000) at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:275
#12 0x76236722 in SystemDictionary::resolve_wk_klass
(id=SystemDictionary::Object_klass_knum,
__the_thread__=0x70033000) at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:2029
#13 0x7623681e in SystemDictionary::resolve_wk_klasses_until
(limit_id=SystemDictionary::Cloneable_klass_knum,
start_id=@0x77fc79d4: SystemDictionary::Object_klass_knum,
__the_thread__=0x70033000)
at
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:2039
#14 0x7623ac01 in
SystemDictionary::resolve_wk_klasses_through
(end_id=SystemDictionary::Class_klass_knum,
start_id=@0x77fc79d4: SystemDictionary::Object_klass
