RFR: 8261268: LOAD_INSTANCE placeholders unneeded for parallelCapable class loaders

[Coleen Phillimore-3]

See CR for more details.  This optimizes some code for class loading.
Tested with tier1-6.

-------------

Commit messages:
 - Make parallelCapable() classes not take LOAD_INSTANCE placeholder.

Changes: https://git.openjdk.java.net/jdk/pull/2469/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2469&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261268
  Stats: 84 lines in 3 files changed: 12 ins; 36 del; 36 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2469.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2469/head:pull/2469

PR: https://git.openjdk.java.net/jdk/pull/2469

[David Holmes-2]

On Tue, 9 Feb 2021 01:20:04 GMT, Coleen Phillimore <[hidden email]> wrote:

> See CR for more details.  This optimizes some code for class loading.
> Tested with tier1-6.

Hi Coleen,

The actual logic changes seem sound (though some of the reorganisation was a little hard to follow).

I have some comments on comments, but overall this seems okay provided all testing passes.

Thanks,
David

src/hotspot/share/classfile/placeholders.cpp line 107:

> 105: // Doubly-linked list of Threads per action for class/classloader pair
> 106: // Class circularity support: links in thread before loading superclass
> 107: // bootstrap loader support:  links in a thread before load_instance_class

I don't think the loader is what the comment is referring to. The reference to bootstrapsearchpath relates to this comment in SD::resolve_instance_class_or_null:
    // add placeholder entry to record loading instance class
    // Five cases:
    // All cases need to prevent modifying bootclasssearchpath
    // in parallel with a classload of same classname

src/hotspot/share/classfile/systemDictionary.cpp line 594:

> 592:   // and has not yet finished.
> 593:   // In both cases the original caller will clean up the placeholder
> 594:   // entry on error.

Is this commentary really no longer applicable? While I don't fully understand it it does indicate the general conditions under which we will call handle_parallel_super_load.

src/hotspot/share/classfile/systemDictionary.cpp line 1865:

> 1863:     }
> 1864:
> 1865:     DEBUG_ONLY(if (is_parallelCapable(class_loader)) verify_placeholder(name, loader_data));

I can't help but feel that somewhere in the loading process there should be a more general assertion that, not only is there a placeholder present, but that it is for this thread. Though such a check would also need to check the kind of placeholder found. Future RFE perhaps.

-------------

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2469

[Coleen Phillimore-3]

On Tue, 9 Feb 2021 02:33:19 GMT, David Holmes <[hidden email]> wrote:

>> See CR for more details.  This optimizes some code for class loading.
>> Tested with tier1-6.
>
> src/hotspot/share/classfile/placeholders.cpp line 107:
>
>> 105: // Doubly-linked list of Threads per action for class/classloader pair
>> 106: // Class circularity support: links in thread before loading superclass
>> 107: // bootstrap loader support:  links in a thread before load_instance_class
>
> I don't think the loader is what the comment is referring to. The reference to bootstrapsearchpath relates to this comment in SD::resolve_instance_class_or_null:
>     // add placeholder entry to record loading instance class
>     // Five cases:
>     // All cases need to prevent modifying bootclasssearchpath
>     // in parallel with a classload of same classname

I don't see any code that does this though.  The code used to compute_loader_lock_object() and return NULL in JVMTI where it was modifying the bootclass search path.  I wonder if that code should have a wait on the placeholder.  I'll investigate.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2469

[Coleen Phillimore-3]

On Tue, 9 Feb 2021 02:43:34 GMT, David Holmes <[hidden email]> wrote:

>> See CR for more details.  This optimizes some code for class loading.
>> Tested with tier1-6.
>
> src/hotspot/share/classfile/systemDictionary.cpp line 594:
>
>> 592:   // and has not yet finished.
>> 593:   // In both cases the original caller will clean up the placeholder
>> 594:   // entry on error.
>
> Is this commentary really no longer applicable? While I don't fully understand it it does indicate the general conditions under which we will call handle_parallel_super_load.

I'm in progress of investigating why this code exists.   The last two lines were false, and these lines seem to repeat the comments above handle_parallel_super_load.  Pointing out java.lang.instrument is even more befuddling.  There may have been a case once where two threads tried to load the same class in RedefineClasses.  I'm not sure why the LOAD_INSTANCE wouldn't have stopped them, just like it stops all classes from being duplicate loaded.  In any case, today RedefineClasses now locks loading the same class with the RedefineClasses_lock.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2469

[Coleen Phillimore-3]

On Tue, 9 Feb 2021 02:55:09 GMT, David Holmes <[hidden email]> wrote:

>> See CR for more details.  This optimizes some code for class loading.
>> Tested with tier1-6.
>
> src/hotspot/share/classfile/systemDictionary.cpp line 1865:
>
>> 1863:     }
>> 1864:
>> 1865:     DEBUG_ONLY(if (is_parallelCapable(class_loader)) verify_placeholder(name, loader_data));
>
> I can't help but feel that somewhere in the loading process there should be a more general assertion that, not only is there a placeholder present, but that it is for this thread. Though such a check would also need to check the kind of placeholder found. Future RFE perhaps.

In this case, we call check_constraints() after loadClass so the placeholder doesn't exist for parallelCapable class loaders (we do want one for the define class path).  Not sure what sort of assertion would be helpful here.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2469

[Coleen Phillimore-3]

On Tue, 9 Feb 2021 12:30:07 GMT, Coleen Phillimore <[hidden email]> wrote:

>> src/hotspot/share/classfile/systemDictionary.cpp line 594:
>>
>>> 592:   // and has not yet finished.
>>> 593:   // In both cases the original caller will clean up the placeholder
>>> 594:   // entry on error.
>>
>> Is this commentary really no longer applicable? While I don't fully understand it it does indicate the general conditions under which we will call handle_parallel_super_load.
>
> I'm in progress of investigating why this code exists.   The last two lines were false, and these lines seem to repeat the comments above handle_parallel_super_load.  Pointing out java.lang.instrument is even more befuddling.  There may have been a case once where two threads tried to load the same class in RedefineClasses.  I'm not sure why the LOAD_INSTANCE wouldn't have stopped them, just like it stops all classes from being duplicate loaded.  In any case, today RedefineClasses now locks loading the same class with the RedefineClasses_lock.

The second sentence of this comment seems correct but is repeated above handle_parallel_super_load. I was going to restore some of this but it doesn't add any information that isn't in the comment above this function.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2469

[Coleen Phillimore-3]

On Tue, 9 Feb 2021 12:27:12 GMT, Coleen Phillimore <[hidden email]> wrote:

>> src/hotspot/share/classfile/placeholders.cpp line 107:
>>
>>> 105: // Doubly-linked list of Threads per action for class/classloader pair
>>> 106: // Class circularity support: links in thread before loading superclass
>>> 107: // bootstrap loader support:  links in a thread before load_instance_class
>>
>> I don't think the loader is what the comment is referring to. The reference to bootstrapsearchpath relates to this comment in SD::resolve_instance_class_or_null:
>>     // add placeholder entry to record loading instance class
>>     // Five cases:
>>     // All cases need to prevent modifying bootclasssearchpath
>>     // in parallel with a classload of same classname
>
> I don't see any code that does this though.  The code used to compute_loader_lock_object() and return NULL in JVMTI where it was modifying the bootclass search path.  I wonder if that code should have a wait on the placeholder.  I'll investigate.

ClassLoader::load_class reads the bootstrapsearchpath lock free.  If it's modified after it's read, then the new entry isn't found.  I don't think there's a requirement for locking.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2469

[Coleen Phillimore-3]

On Tue, 9 Feb 2021 02:56:46 GMT, David Holmes <[hidden email]> wrote:

>The actual logic changes seem sound (though some of the reorganisation was a little hard to follow).

Unfortunately the diffs are bad because I did two things.  I changed the indentation and I check for the class in the dictionary before adding the LOAD_INSTANCE placeholder for the !parallelCapable() && !bootclass case.

> I have some comments on comments, but overall this seems okay provided all testing passes.
Yup, thanks!

-------------

PR: https://git.openjdk.java.net/jdk/pull/2469

[Ioi Lam-2]

On Tue, 9 Feb 2021 01:20:04 GMT, Coleen Phillimore <[hidden email]> wrote:

> See CR for more details.  This optimizes some code for class loading.
> Tested with tier1-6.

LGTM

-------------

Marked as reviewed by iklam (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2469

[Coleen Phillimore-3]

> See CR for more details.  This optimizes some code for class loading.
> Tested with tier1-6.

Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:

  Fix some comment

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2469/files
  - new: https://git.openjdk.java.net/jdk/pull/2469/files/9eb1634b..c28d8ded

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2469&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2469&range=00-01

  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2469.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2469/head:pull/2469

PR: https://git.openjdk.java.net/jdk/pull/2469

[Coleen Phillimore-3]

On Tue, 9 Feb 2021 21:19:58 GMT, Ioi Lam <[hidden email]> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Fix some comment
>
> LGTM

Thanks Ioi.  I reworded the comment we talked about offline.
+    //    These class loaders lock a per-class object lock when ClassLoader.loadClass()
+    //    is called. A LOAD_INSTANCE placeholder isn't used for mutual exclusion.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2469

[Ioi Lam-2]

On Tue, 9 Feb 2021 22:22:55 GMT, Coleen Phillimore <[hidden email]> wrote:

>> See CR for more details.  This optimizes some code for class loading.
>> Tested with tier1-6.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>
>   Fix some comment

Marked as reviewed by iklam (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/2469

[David Holmes]

Hi Coleen,

On 9/02/2021 11:36 pm, Coleen Phillimore wrote:

> On Tue, 9 Feb 2021 12:27:12 GMT, Coleen Phillimore <[hidden email]> wrote:
>
>>> src/hotspot/share/classfile/placeholders.cpp line 107:
>>>
>>>> 105: // Doubly-linked list of Threads per action for class/classloader pair
>>>> 106: // Class circularity support: links in thread before loading superclass
>>>> 107: // bootstrap loader support:  links in a thread before load_instance_class
>>>
>>> I don't think the loader is what the comment is referring to. The reference to bootstrapsearchpath relates to this comment in SD::resolve_instance_class_or_null:
>>>      // add placeholder entry to record loading instance class
>>>      // Five cases:
>>>      // All cases need to prevent modifying bootclasssearchpath
>>>      // in parallel with a classload of same classname
>>
>> I don't see any code that does this though.  The code used to compute_loader_lock_object() and return NULL in JVMTI where it was modifying the bootclass search path.  I wonder if that code should have a wait on the placeholder.  I'll investigate.
>
> ClassLoader::load_class reads the bootstrapsearchpath lock free.  If it's modified after it's read, then the new entry isn't found.  I don't think there's a requirement for locking.

I looked through the JDK 6 code and it has these comments connecting the
bootclass search path, but no actual code that shows any  connection
between that path and the placeholder table. So it seems fine to expunge
all those comments in the placeholder and SD code.

Thanks,
David

> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/2469
>

[Coleen Phillimore-3]

On Tue, 9 Feb 2021 02:56:46 GMT, David Holmes <[hidden email]> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Fix some comment
>
> Hi Coleen,
>
> The actual logic changes seem sound (though some of the reorganisation was a little hard to follow).
>
> I have some comments on comments, but overall this seems okay provided all testing passes.
>
> Thanks,
> David

@dholmes-ora thanks for double checking the older code.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2469

[Coleen Phillimore-3]

On Tue, 9 Feb 2021 01:20:04 GMT, Coleen Phillimore <[hidden email]> wrote:

> See CR for more details.  This optimizes some code for class loading.
> Tested with tier1-6.

This pull request has now been integrated.

Changeset: 52fc01b3
Author:    Coleen Phillimore <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/52fc01b3
Stats:     85 lines in 3 files changed: 12 ins; 36 del; 37 mod

8261268: LOAD_INSTANCE placeholders unneeded for parallelCapable class loaders

Reviewed-by: dholmes, iklam

-------------

PR: https://git.openjdk.java.net/jdk/pull/2469