Re: RFR 8184289: Obsolete -XX:+UnsyncloadClass and -XX:+MustCallLoadClassInternal options

2018-02-12 Thread David Holmes 

Looks good!

Thanks,
David

On 13/02/2018 6:50 AM, harold seigel wrote:

Hi Karen,

Thanks for looking at this!

I re-ran the ParallelClassLoading tests with no options, with 
-XX:+AllowParallelDefineClass, and with -XX:+AlwaysLockClassLoader, and 
all the tests passed.  I also determined that the few Mach5 regression 
test failures that I encountered were unrelated to my change.


Please see this latest webrev that adds 12 as an expiration date, 
removes the 'guarantee' section, and fixes the comment at line 786 of 
systemDictionary.cpp.


http://cr.openjdk.java.net/~hseigel/bug_8184289.3/webrev/index.html

Thanks, Harold

On 2/12/2018 2:39 PM, Karen Kinnear wrote:

Harold,

Thanks for doing this.

I think you told me that
1) the version change has made it in
2) you also put 12 as an expiration date
3) you are running the ParallelClassLoading tests with the remaining 
two flags (you’ve already

run them without any flags):
  AllowParallelDefineClass = true and AlwaysLockClassLoader=true

In terms of the guarantee in question
// For UnsyncloadClass only
  848   // If they got a linkageError, check if a parallel class load 
succeeded.
  849   // If it did, then for bytecode resolution the specification 
requires
  850   // that we return the same result we did for the other thread, i.e. 
the
  851   // successfully loaded InstanceKlass
  852   // Should not get here for classloaders that support parallelism
  853   // with the new cleaner mechanism, even with 
AllowParallelDefineClass
  854   // Bootstrap goes through here to allow for an extra guarantee check
855 if (UnsyncloadClass || (class_loader.is_null())) {
  856 if (k == NULL && HAS_PENDING_EXCEPTION
  857   && 
PENDING_EXCEPTION->is_a(SystemDictionary::LinkageError_klass())) {
  858   MutexLocker mu(SystemDictionary_lock, THREAD);
  859   InstanceKlass* check = find_class(d_hash, name, dictionary);
  860   if (check != NULL) {
  861 // Klass is already loaded, so just use it
  862 k = check;
  863 CLEAR_PENDING_EXCEPTION;
  864 guarantee((!class_loader.is_null()), "dup definition for bootstrap 
loader?");
  865   }
  866 }
  867   }

  1) I agree you can remove the entire section
      - the guarantee was there for future proofing in case we ever 
allowed parallel class loading of the
        same class for the null loader and to make sure I didn’t have 
any logic holes.
      - I would not put an assertion for the first half of the 
condition - I would remove completely
      - the code currently prevents parallel class loading of the same 
class for the null loader at:

resolve_instance_class_or_null see line 785 …
  while (!class_has_been_loaded && old probe && old 
probe->instance_load_in_progress()) {

     //  case x: bootstrap class loader: prevent futile class loading,
     // wait on first requestor
     if (class_loader.is_null()) {
        SystemDictionary_lock->wait();

This logic means that there is a registered INSTANCE_LOAD on this 
placeholder entry.



Other minor comments (sorry if you already got these and I missed them 
in earlier emails)

- all in SystemDictionary.cpp

1. line 72 comment “Five cases:” -> “Four cases:”
So you removed case 3 and renumbered, so old references to case 4 -> 
case 3 ,and old references

to case 5 become case 4:

So - line 786, “Case 4” -> “case 3”

thanks,
Karen


On Feb 12, 2018, at 11:13 AM, harold seigel > wrote:


Hi Alan,

Thanks for looking at this.

Harold

On 2/12/2018 2:52 AM, Alan Bateman wrote:

On 12/02/2018 06:54, David Holmes wrote:

Hi Harold,

Adding core-libs-dev so they are aware of the ClassLoader change.

Thanks, that part is okay and good to see this going away.

-Alan

Re: RFR 8184289: Obsolete -XX:+UnsyncloadClass and -XX:+MustCallLoadClassInternal options

2018-02-12 Thread Karen Kinnear 
Harold,

Looks good. Many thanks!

Karen

> On Feb 12, 2018, at 3:50 PM, harold seigel  wrote:
> 
> Hi Karen,
> 
> Thanks for looking at this!
> 
> I re-ran the ParallelClassLoading tests with no options, with 
> -XX:+AllowParallelDefineClass, and with -XX:+AlwaysLockClassLoader, and all 
> the tests passed.  I also determined that the few Mach5 regression test 
> failures that I encountered were unrelated to my change.
> Please see this latest webrev that adds 12 as an expiration date, removes the 
> 'guarantee' section, and fixes the comment at line 786 of 
> systemDictionary.cpp.
> http://cr.openjdk.java.net/~hseigel/bug_8184289.3/webrev/index.html 
> 
> Thanks, Harold
> 
> On 2/12/2018 2:39 PM, Karen Kinnear wrote:
>> Harold,
>> 
>> Thanks for doing this. 
>> 
>> I think you told me that
>> 1) the version change has made it in
>> 2) you also put 12 as an expiration date
>> 3) you are running the ParallelClassLoading tests with the remaining two 
>> flags (you’ve already
>> run them without any flags):
>>   AllowParallelDefineClass = true and AlwaysLockClassLoader=true
>> 
>> In terms of the guarantee in question
>>   // For UnsyncloadClass only
>>  848   // If they got a linkageError, check if a parallel class load 
>> succeeded.
>>  849   // If it did, then for bytecode resolution the specification 
>> requires
>>  850   // that we return the same result we did for the other thread, 
>> i.e. the
>>  851   // successfully loaded InstanceKlass
>>  852   // Should not get here for classloaders that support parallelism
>>  853   // with the new cleaner mechanism, even with 
>> AllowParallelDefineClass
>>  854   // Bootstrap goes through here to allow for an extra guarantee 
>> check
>>  855   if (UnsyncloadClass || (class_loader.is_null())) {
>>  856 if (k == NULL && HAS_PENDING_EXCEPTION
>>  857   && 
>> PENDING_EXCEPTION->is_a(SystemDictionary::LinkageError_klass())) {
>>  858   MutexLocker mu(SystemDictionary_lock, THREAD);
>>  859   InstanceKlass* check = find_class(d_hash, name, dictionary);
>>  860   if (check != NULL) {
>>  861 // Klass is already loaded, so just use it
>>  862 k = check;
>>  863 CLEAR_PENDING_EXCEPTION;
>>  864 guarantee((!class_loader.is_null()), "dup definition for 
>> bootstrap loader?");
>>  865   }
>>  866 }
>>  867   }
>> 
>>   1) I agree you can remove the entire section
>>   - the guarantee was there for future proofing in case we ever allowed 
>> parallel class loading of the
>> same class for the null loader and to make sure I didn’t have any 
>> logic holes.
>>   - I would not put an assertion for the first half of the condition - I 
>> would remove completely
>>   - the code currently prevents parallel class loading of the same class 
>> for the null loader at:
>> resolve_instance_class_or_null see line 785 …
>>   while (!class_has_been_loaded && old probe && old 
>> probe->instance_load_in_progress()) {
>>  //  case x: bootstrap class loader: prevent futile class loading,
>>  // wait on first requestor
>>  if (class_loader.is_null()) {
>> SystemDictionary_lock->wait();
>> 
>> This logic means that there is a registered INSTANCE_LOAD on this 
>> placeholder entry.
>> 
>> 
>> Other minor comments (sorry if you already got these and I missed them in 
>> earlier emails)
>> - all in SystemDictionary.cpp
>> 
>> 1. line 72 comment “Five cases:” -> “Four cases:”
>> So you removed case 3 and renumbered, so old references to case 4 -> case 3 
>> ,and old references
>> to case 5 become case 4:
>> 
>> So - line 786, “Case 4” -> “case 3”
>> 
>> thanks,
>> Karen
>> 
>> 
>>> On Feb 12, 2018, at 11:13 AM, harold seigel >> > wrote:
>>> 
>>> Hi Alan,
>>> 
>>> Thanks for looking at this.
>>> 
>>> Harold
>>> 
>>> On 2/12/2018 2:52 AM, Alan Bateman wrote:
 On 12/02/2018 06:54, David Holmes wrote:
> Hi Harold,
> 
> Adding core-libs-dev so they are aware of the ClassLoader change.
 Thanks, that part is okay and good to see this going away.
 
 -Alan
>>> 
>> 
>

Re: RFR 8184289: Obsolete -XX:+UnsyncloadClass and -XX:+MustCallLoadClassInternal options

2018-02-12 Thread harold seigel 

Hi Karen,

Thanks for looking at this!

I re-ran the ParallelClassLoading tests with no options, with 
-XX:+AllowParallelDefineClass, and with -XX:+AlwaysLockClassLoader, and 
all the tests passed.  I also determined that the few Mach5 regression 
test failures that I encountered were unrelated to my change.


Please see this latest webrev that adds 12 as an expiration date, 
removes the 'guarantee' section, and fixes the comment at line 786 of 
systemDictionary.cpp.


   http://cr.openjdk.java.net/~hseigel/bug_8184289.3/webrev/index.html

Thanks, Harold

On 2/12/2018 2:39 PM, Karen Kinnear wrote:

Harold,

Thanks for doing this.

I think you told me that
1) the version change has made it in
2) you also put 12 as an expiration date
3) you are running the ParallelClassLoading tests with the remaining 
two flags (you’ve already

run them without any flags):
  AllowParallelDefineClass = true and AlwaysLockClassLoader=true

In terms of the guarantee in question
// For UnsyncloadClass only
  848   // If they got a linkageError, check if a parallel class load 
succeeded.
  849   // If it did, then for bytecode resolution the specification 
requires
  850   // that we return the same result we did for the other thread, i.e. 
the
  851   // successfully loaded InstanceKlass
  852   // Should not get here for classloaders that support parallelism
  853   // with the new cleaner mechanism, even with 
AllowParallelDefineClass
  854   // Bootstrap goes through here to allow for an extra guarantee check
855 if (UnsyncloadClass || (class_loader.is_null())) {
  856 if (k == NULL && HAS_PENDING_EXCEPTION
  857   && 
PENDING_EXCEPTION->is_a(SystemDictionary::LinkageError_klass())) {
  858   MutexLocker mu(SystemDictionary_lock, THREAD);
  859   InstanceKlass* check = find_class(d_hash, name, dictionary);
  860   if (check != NULL) {
  861 // Klass is already loaded, so just use it
  862 k = check;
  863 CLEAR_PENDING_EXCEPTION;
  864 guarantee((!class_loader.is_null()), "dup definition for bootstrap 
loader?");
  865   }
  866 }
  867   }

  1) I agree you can remove the entire section
      - the guarantee was there for future proofing in case we ever 
allowed parallel class loading of the
        same class for the null loader and to make sure I didn’t have 
any logic holes.
      - I would not put an assertion for the first half of the 
condition - I would remove completely
      - the code currently prevents parallel class loading of the same 
class for the null loader at:

resolve_instance_class_or_null see line 785 …
  while (!class_has_been_loaded && old probe && old 
probe->instance_load_in_progress()) {

     //  case x: bootstrap class loader: prevent futile class loading,
     // wait on first requestor
     if (class_loader.is_null()) {
        SystemDictionary_lock->wait();

This logic means that there is a registered INSTANCE_LOAD on this 
placeholder entry.



Other minor comments (sorry if you already got these and I missed them 
in earlier emails)

- all in SystemDictionary.cpp

1. line 72 comment “Five cases:” -> “Four cases:”
So you removed case 3 and renumbered, so old references to case 4 -> 
case 3 ,and old references

to case 5 become case 4:

So - line 786, “Case 4” -> “case 3”

thanks,
Karen


On Feb 12, 2018, at 11:13 AM, harold seigel > wrote:


Hi Alan,

Thanks for looking at this.

Harold

On 2/12/2018 2:52 AM, Alan Bateman wrote:

On 12/02/2018 06:54, David Holmes wrote:

Hi Harold,

Adding core-libs-dev so they are aware of the ClassLoader change.

Thanks, that part is okay and good to see this going away.

-Alan

Re: RFR 8184289: Obsolete -XX:+UnsyncloadClass and -XX:+MustCallLoadClassInternal options

2018-02-12 Thread Karen Kinnear 
Harold,

Thanks for doing this. 

I think you told me that
1) the version change has made it in
2) you also put 12 as an expiration date
3) you are running the ParallelClassLoading tests with the remaining two flags 
(you’ve already
run them without any flags):
  AllowParallelDefineClass = true and AlwaysLockClassLoader=true

In terms of the guarantee in question
  // For UnsyncloadClass only
 848   // If they got a linkageError, check if a parallel class load 
succeeded.
 849   // If it did, then for bytecode resolution the specification requires
 850   // that we return the same result we did for the other thread, i.e. 
the
 851   // successfully loaded InstanceKlass
 852   // Should not get here for classloaders that support parallelism
 853   // with the new cleaner mechanism, even with AllowParallelDefineClass
 854   // Bootstrap goes through here to allow for an extra guarantee check
 855   if (UnsyncloadClass || (class_loader.is_null())) {
 856 if (k == NULL && HAS_PENDING_EXCEPTION
 857   && 
PENDING_EXCEPTION->is_a(SystemDictionary::LinkageError_klass())) {
 858   MutexLocker mu(SystemDictionary_lock, THREAD);
 859   InstanceKlass* check = find_class(d_hash, name, dictionary);
 860   if (check != NULL) {
 861 // Klass is already loaded, so just use it
 862 k = check;
 863 CLEAR_PENDING_EXCEPTION;
 864 guarantee((!class_loader.is_null()), "dup definition for 
bootstrap loader?");
 865   }
 866 }
 867   }

  1) I agree you can remove the entire section
  - the guarantee was there for future proofing in case we ever allowed 
parallel class loading of the
same class for the null loader and to make sure I didn’t have any logic 
holes.
  - I would not put an assertion for the first half of the condition - I 
would remove completely
  - the code currently prevents parallel class loading of the same class 
for the null loader at:
resolve_instance_class_or_null see line 785 …
  while (!class_has_been_loaded && old probe && old 
probe->instance_load_in_progress()) {
 //  case x: bootstrap class loader: prevent futile class loading,
 // wait on first requestor
 if (class_loader.is_null()) {
SystemDictionary_lock->wait();

This logic means that there is a registered INSTANCE_LOAD on this placeholder 
entry.


Other minor comments (sorry if you already got these and I missed them in 
earlier emails)
- all in SystemDictionary.cpp

1. line 72 comment “Five cases:” -> “Four cases:”
So you removed case 3 and renumbered, so old references to case 4 -> case 3 
,and old references
to case 5 become case 4:

So - line 786, “Case 4” -> “case 3”

thanks,
Karen


> On Feb 12, 2018, at 11:13 AM, harold seigel  wrote:
> 
> Hi Alan,
> 
> Thanks for looking at this.
> 
> Harold
> 
> On 2/12/2018 2:52 AM, Alan Bateman wrote:
>> On 12/02/2018 06:54, David Holmes wrote:
>>> Hi Harold,
>>> 
>>> Adding core-libs-dev so they are aware of the ClassLoader change.
>> Thanks, that part is okay and good to see this going away.
>> 
>> -Alan
>

Re: RFR 8184289: Obsolete -XX:+UnsyncloadClass and -XX:+MustCallLoadClassInternal options

2018-02-12 Thread harold seigel 

Hi Alan,

Thanks for looking at this.

Harold

On 2/12/2018 2:52 AM, Alan Bateman wrote:

On 12/02/2018 06:54, David Holmes wrote:

Hi Harold,

Adding core-libs-dev so they are aware of the ClassLoader change.

Thanks, that part is okay and good to see this going away.

-Alan

Re: RFR 8184289: Obsolete -XX:+UnsyncloadClass and -XX:+MustCallLoadClassInternal options

2018-02-12 Thread coleen . phillimore 



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.


---

 

Re: RFR 8184289: Obsolete -XX:+UnsyncloadClass and -XX:+MustCallLoadClassInternal options

2018-02-11 Thread Alan Bateman 

On 12/02/2018 06:54, David Holmes wrote:

Hi Harold,

Adding core-libs-dev so they are aware of the ClassLoader change.

Thanks, that part is okay and good to see this going away.

-Alan

Re: RFR 8184289: Obsolete -XX:+UnsyncloadClass and -XX:+MustCallLoadClassInternal options

2018-02-11 Thread David Holmes 

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.


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: