On 2/12/18 1:54 AM, David Holmes wrote:
Hi Harold,
Adding core-libs-dev so they are aware of the ClassLoader change.
On 10/02/2018 5:44 AM, harold seigel wrote:
Hi David,
Thanks for reviewing this.
Please see updated webrev:
http://cr.openjdk.java.net/~hseigel/bug_8184289.2/webrev/index.html
This all seems fine to me.
I agree there is question mark over the SystemDictionary code now only
used for the null/boot loader:
848 if ((class_loader.is_null())) {
849 if (k == NULL && HAS_PENDING_EXCEPTION
850 &&
PENDING_EXCEPTION->is_a(SystemDictionary::LinkageError_klass())) {
851 MutexLocker mu(SystemDictionary_lock, THREAD);
852 InstanceKlass* check = find_class(d_hash, name,
dictionary);
853 if (check != NULL) {
854 // Klass is already loaded, so just use it
855 k = check;
856 CLEAR_PENDING_EXCEPTION;
857 guarantee((!class_loader.is_null()), "dup definition
for bootstrap loader?");
858 }
859 }
This seems to be a negative test, that we should in fact never get in
this situation when dealing with the boot loader. But in that case we
don't need lines 855 and 856 anymore and the code would just collapse to:
848 if ((class_loader.is_null())) {
849 if (k == NULL && HAS_PENDING_EXCEPTION
850 &&
PENDING_EXCEPTION->is_a(SystemDictionary::LinkageError_klass())) {
851 MutexLocker mu(SystemDictionary_lock, THREAD);
852 guarantee(find_class(d_hash, name, dictionary) == NULL,
853 "dup definition for bootstrap loader?");
854 }
855 }
and in that case I'd probably rather see this as an assertion than a
guarantee, and the whole block can be debug-only.
(just to you two). If this is decided that it's an assert only, my vote
would be to remove it to simplify the logic here, which is the main
benefit of removing these options. It's too special-casey as it is.
Coleen
Thanks,
David
-----
And, please see in-line comments.
On 2/8/2018 5:42 PM, David Holmes wrote:
Hi Harold,
First I'm pretty sure this one can't be pushed until the version
bump arrives in jdk/hs :)
I hope the version bump arrives soon.
On 9/02/2018 6:53 AM, harold seigel wrote:
Hi,
Please review this JDK-11 change to obsolete the UnsyncloadClass
and MustCallLoadClassInternal options. With this change, these
options are still accepted on the command line but have no affect
other than to generate these warning messages:
Java HotSpot(TM) 64-Bit Server VM warning: Ignoring option
UnsyncloadClass; support was removed in 11.0
Java HotSpot(TM) 64-Bit Server VM warning: Ignoring option
MustCallLoadClassInternal; support was removed in 11.0
Open Webrev:
http://cr.openjdk.java.net/~hseigel/bug_8184289/webrev/index.html
Overall looks good. Tricky to untangle things in the SD especially!
src/hotspot/share/classfile/systemDictionary.cpp
Looking at this change, and the comment:
- // For UnsyncloadClass only
// If they got a linkageError, check if a parallel class
load succeeded.
// If it did, then for bytecode resolution the specification
requires
// that we return the same result we did for the other
thread, i.e. the
// successfully loaded InstanceKlass
// Should not get here for classloaders that support
parallelism
// with the new cleaner mechanism, even with
AllowParallelDefineClass
// Bootstrap goes through here to allow for an extra
guarantee check
! if (UnsyncloadClass || (class_loader.is_null())) {
It's not clear why all the "UnsyncLoadClass only" stuff is also
being done for the "Bootstrap" (? bootloader?) case. But in any case
the comment block doesn't read correctly now as this is all, and
only, about the bootstrap case. I'd suggest:
- // For UnsyncloadClass only
+ // For bootloader only:
// If they got a linkageError, check if a parallel class
load succeeded.
// If it did, then for bytecode resolution the specification
requires
// that we return the same result we did for the other
thread, i.e. the
// successfully loaded InstanceKlass
// Should not get here for classloaders that support
parallelism
// with the new cleaner mechanism, even with
AllowParallelDefineClass
- // Bootstrap goes through here to allow for an extra
guarantee check
Done.
Question: is ClassLoader.loadClassInternal now obsolete as well?
Yes. Thanks for pointing that out. The new webrev contains
significant changes needed to remove loadClassInternal.
---
src/hotspot/share/runtime/arguments.cpp
Is there some reason to leave the expiration version unset? Do you
think the obsoletion warning may be needed for a couple of releases ??
I figured whoever expires the option can put in the version.
---
test/hotspot/jtreg/runtime/CommandLine/ObsoleteFlagErrorMessage.java
You don't need to add anything here. This is not a test of all
obsolete flags, it is just testing some specific handling of
obsolete flags.
I reverted this change.
Thanks! Harold
Thanks,
David
-----
JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8184289
The change was tested with Mach5 tiers 1-5 and the NSK parallel
class loading tests. Also, JDK's containing the change were built
on all Mach5 platforms.
Thanks, Harold
