RFR: 8262046: Clean up parallel class loading code and comments

classic Classic list List threaded Threaded
21 messages Options
12
Reply | Threaded
Open this post in threaded view
|

RFR: 8262046: Clean up parallel class loading code and comments

Coleen Phillimore-3
This change reduces the length of SystemDictionary::resolve_instance_class_or_null, the main class loading function to 4 pages on my screen, down from ~6.   Some functions are made static inside systemDictionary.hpp.

The only executable change is that handle_parallel_super_load used to have a wait while the class's superclass was being loaded after calling resolve_super_or_fail.  It was similar to the subsequent code that waited for the LOAD_INSTANCE placeholder, immediately following the return from handle_parallel_super_load.  I consolidated this code to wait for both conditions and kept the comment from handle_parallel_super_load.  This wait is now in the function handle_parallel_loading.

I also added a new load_instance_class that calls load_instance_class_impl.  The new load_instance_class does constraint checking and bookkeeping after it is called so that it can return with CHECK_NULL and not check pending exceptions. These functions aren't called outside of systemDictionary.cpp.

I added a test to show why handle_parallel_super_load is needed (see bug for details of the deadlock that this test would get if not for handle_parallel_super_load).

This updates comments to:
1. rewrote ClassCircularityError detection above resolve_super_or_fail so that it made more sense to me (hope it does for others too).
2. move comments to be near the code they describe
3. removed some comments that referred to code somewhere else but not helpful at that location, or to describe code that was deleted or obsolete, or repetitive
4. added a comment for resolve_instance_class_or_null

Ran tier1-3 testing, jck vm, jck lang and our internal parallel class loading tests.  Retesting with tiers 4-8 in progress (I tested an earlier version of this with no failures).

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

Commit messages:
 - 8262046: Clean up parallel class loading code and comments

Changes: https://git.openjdk.java.net/jdk/pull/3200/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3200&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8262046
  Stats: 827 lines in 11 files changed: 606 ins; 100 del; 121 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3200.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3200/head:pull/3200

PR: https://git.openjdk.java.net/jdk/pull/3200
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262046: Clean up parallel class loading code and comments [v2]

Coleen Phillimore-3
> This change reduces the length of SystemDictionary::resolve_instance_class_or_null, the main class loading function to 4 pages on my screen, down from ~6.   Some functions are made static inside systemDictionary.hpp.
>
> The only executable change is that handle_parallel_super_load used to have a wait while the class's superclass was being loaded after calling resolve_super_or_fail.  It was similar to the subsequent code that waited for the LOAD_INSTANCE placeholder, immediately following the return from handle_parallel_super_load.  I consolidated this code to wait for both conditions and kept the comment from handle_parallel_super_load.  This wait is now in the function handle_parallel_loading.
>
> I also added a new load_instance_class that calls load_instance_class_impl.  The new load_instance_class does constraint checking and bookkeeping after it is called so that it can return with CHECK_NULL and not check pending exceptions. These functions aren't called outside of systemDictionary.cpp.
>
> I added a test to show why handle_parallel_super_load is needed (see bug for details of the deadlock that this test would get if not for handle_parallel_super_load).
>
> This updates comments to:
> 1. rewrote ClassCircularityError detection above resolve_super_or_fail so that it made more sense to me (hope it does for others too).
> 2. move comments to be near the code they describe
> 3. removed some comments that referred to code somewhere else but not helpful at that location, or to describe code that was deleted or obsolete, or repetitive
> 4. added a comment for resolve_instance_class_or_null
>
> Ran tier1-3 testing, jck vm, jck lang and our internal parallel class loading tests.  Retesting with tiers 4-8 in progress (I tested an earlier version of this with no failures).

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

  Fix some comments and add placeholder CCE logging.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3200/files
  - new: https://git.openjdk.java.net/jdk/pull/3200/files/2118aac2..23cbf92a

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

  Stats: 24 lines in 1 file changed: 16 ins; 1 del; 7 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3200.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3200/head:pull/3200

PR: https://git.openjdk.java.net/jdk/pull/3200
Reply | Threaded
Open this post in threaded view
|

Withdrawn: 8262046: Clean up parallel class loading code and comments

Coleen Phillimore-3
In reply to this post by Coleen Phillimore-3
On Thu, 25 Mar 2021 17:23:42 GMT, Coleen Phillimore <[hidden email]> wrote:

> This change reduces the length of SystemDictionary::resolve_instance_class_or_null, the main class loading function to 4 pages on my screen, down from ~6.   Some functions are made static inside systemDictionary.hpp.
>
> The only executable change is that handle_parallel_super_load used to have a wait while the class's superclass was being loaded after calling resolve_super_or_fail.  It was similar to the subsequent code that waited for the LOAD_INSTANCE placeholder, immediately following the return from handle_parallel_super_load.  I consolidated this code to wait for both conditions and kept the comment from handle_parallel_super_load.  This wait is now in the function handle_parallel_loading.
>
> I also added a new load_instance_class that calls load_instance_class_impl.  The new load_instance_class does constraint checking and bookkeeping after it is called so that it can return with CHECK_NULL and not check pending exceptions. These functions aren't called outside of systemDictionary.cpp.
>
> I added a test to show why handle_parallel_super_load is needed (see bug for details of the deadlock that this test would get if not for handle_parallel_super_load).
>
> This updates comments to:
> 1. rewrote ClassCircularityError detection above resolve_super_or_fail so that it made more sense to me (hope it does for others too).
> 2. move comments to be near the code they describe
> 3. removed some comments that referred to code somewhere else but not helpful at that location, or to describe code that was deleted or obsolete, or repetitive
> 4. added a comment for resolve_instance_class_or_null
>
> Ran tier1-3 testing, jck vm, jck lang and our internal parallel class loading tests.  Retesting with tiers 4-8 in progress (I tested an earlier version of this with no failures).

This pull request has been closed without being integrated.

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

PR: https://git.openjdk.java.net/jdk/pull/3200
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262046: Clean up parallel class loading code and comments [v2]

Coleen Phillimore-3
In reply to this post by Coleen Phillimore-3
On Sun, 28 Mar 2021 21:16:17 GMT, Coleen Phillimore <[hidden email]> wrote:

>> This change reduces the length of SystemDictionary::resolve_instance_class_or_null, the main class loading function to 4 pages on my screen, down from ~6.   Some functions are made static inside systemDictionary.hpp.
>>
>> The only executable change is that handle_parallel_super_load used to have a wait while the class's superclass was being loaded after calling resolve_super_or_fail.  It was similar to the subsequent code that waited for the LOAD_INSTANCE placeholder, immediately following the return from handle_parallel_super_load.  I consolidated this code to wait for both conditions and kept the comment from handle_parallel_super_load.  This wait is now in the function handle_parallel_loading.
>>
>> I also added a new load_instance_class that calls load_instance_class_impl.  The new load_instance_class does constraint checking and bookkeeping after it is called so that it can return with CHECK_NULL and not check pending exceptions. These functions aren't called outside of systemDictionary.cpp.
>>
>> I added a test to show why handle_parallel_super_load is needed (see bug for details of the deadlock that this test would get if not for handle_parallel_super_load).
>>
>> This updates comments to:
>> 1. rewrote ClassCircularityError detection above resolve_super_or_fail so that it made more sense to me (hope it does for others too).
>> 2. move comments to be near the code they describe
>> 3. removed some comments that referred to code somewhere else but not helpful at that location, or to describe code that was deleted or obsolete, or repetitive
>> 4. added a comment for resolve_instance_class_or_null
>>
>> Ran tier1-3 testing, jck vm, jck lang and our internal parallel class loading tests.  Retesting with tiers 4-8 in progress (I tested an earlier version of this with no failures).
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>
>   Fix some comments and add placeholder CCE logging.

Added some comments and questions for reviewers.

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

> 381: // the super class directly, then resume steps 4-6.
> 382: //
> 383: //

resolve_super_or_fail had this comment that I found difficult to understand and work through, which I did at one point, so I wanted to replace it with one that seemed less so.  It occurs to me that this is probably unhelpful also.   Maybe the whole comment from lines 351 to 381 should be replaced with a sentence like:

//resolve_super_or_fail adds a LOAD_SUPER placeholder to the placeholder table before calling
// resolve_instance_class_or_null.  ClassCircularityError is detected when a LOAD_SUPER or LOAD_INSTANCE
// placeholder for the same thread, class, classloader is found.

I'm unlikely to work through the new comment example again myself.

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

> 391:   // the name of the class might not be known until the stream is actually
> 392:   // parsed.
> 393:   // Bugs 4643874, 4715493

placeholders used to be added in resolve_from_stream so this comment is obsolete.

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

> 441:   // The instanceKlass is kept alive because the class loader is on the stack,
> 442:   // which keeps the loader_data alive, as well as all instanceKlasses in
> 443:   // the loader_data. parseClassFile adds the instanceKlass to loader_data.

This comment refers to code that is no longer here.  The placeholder entry used to keep the class alive once.

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

> 503: // super class loading here.
> 504: // This also is critical in cases where the original thread gets stalled
> 505: // even in non-circularity situations.

I found no evidence of why sentence at 504-505 would be true.

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

> 559:       // threads trying to load its super class have removed their placeholders.
> 560:       while (oldprobe != NULL &&
> 561:              (oldprobe->instance_load_in_progress() || oldprobe->super_load_in_progress())) {

This loop in handle_parallel_super_load and the one in resolve_instance_class_or_null are the same loop.  The current code waits for all the LOAD_SUPER placeholders to be removed.  If there are 'n' threads loading a class with a super, the first thread will add a LOAD_INSTANCE placeholder, and the second thread finding LOAD_INSTANCE will wait at the loop in resolve_instance_class_or_null for the LOAD_INSTANCE to be done.
If the first thread loads the class, and calls resolve_super_or_fail and adds the LOAD_SUPER placeholder, the next threads will call handle_parallel_super_load and wait for all the LOAD_SUPER placeholder to be removed from all the threads calling handle_parallel_super_load.
Once the first thread completes loading the super class, and removes the LOAD_SUPER placeholders, and all the other threads complete trying to load the super class, the other threads will fall into the loop in resolve_instance_class_or_null, waiting for the first thread to remove LOAD_INSTANCE.
There's only one LOAD_INSTANCE placeholder for each class/class loader pair in progress. There may be many LOAD_SUPER placeholders. The point is that both conditions can be checked in the same loop.  The loop needs to wait for all LOAD_SUPERs and the main LOAD_INSTANCE to clear.
I could rename "handle_parallel_loading" to "wait_for_parallel_loading" if that makes it more clear(?)

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

> 748:     if (loaded_class == NULL) {
> 749:       // Do actual loading
> 750:       loaded_class = load_instance_class(name_hash, name, class_loader, THREAD);

The whole passing THREAD to worry about not returning before removing the placeholder entry is a lot simpler if this code is in its own function, so I moved it there.

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

PR: https://git.openjdk.java.net/jdk/pull/3200
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262046: Clean up parallel class loading code and comments [v2]

Coleen Phillimore-3
On Tue, 30 Mar 2021 10:36:48 GMT, Coleen Phillimore <[hidden email]> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Fix some comments and add placeholder CCE logging.
>
> Added some comments and questions for reviewers.

I closed this by mistake also yesterday trying to purge pull request branches from my fork.  I added some comments that explain the comments.

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

PR: https://git.openjdk.java.net/jdk/pull/3200
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262046: Clean up parallel class loading code and comments [v2]

Coleen Phillimore-3
In reply to this post by Coleen Phillimore-3
On Tue, 30 Mar 2021 10:14:35 GMT, Coleen Phillimore <[hidden email]> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Fix some comments and add placeholder CCE logging.
>
> src/hotspot/share/classfile/systemDictionary.cpp line 383:
>
>> 381: // the super class directly, then resume steps 4-6.
>> 382: //
>> 383: //
>
> resolve_super_or_fail had this comment that I found difficult to understand and work through, which I did at one point, so I wanted to replace it with one that seemed less so.  It occurs to me that this is probably unhelpful also.   Maybe the whole comment from lines 351 to 381 should be replaced with a sentence like:
>
> //resolve_super_or_fail adds a LOAD_SUPER placeholder to the placeholder table before calling
> // resolve_instance_class_or_null.  ClassCircularityError is detected when a LOAD_SUPER or LOAD_INSTANCE
> // placeholder for the same thread, class, classloader is found.
>
> I'm unlikely to work through the new comment example again myself.

Since nobody wants to read this example in this comment and neither do I, I'm removing it.  Debugging this using -Xlog:class+load+placeholders=debug will show the stacking of placeholders that detect CCE.

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

PR: https://git.openjdk.java.net/jdk/pull/3200
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262046: Clean up parallel class loading code and comments [v3]

Coleen Phillimore-3
In reply to this post by Coleen Phillimore-3
> This change reduces the length of SystemDictionary::resolve_instance_class_or_null, the main class loading function to 4 pages on my screen, down from ~6.   Some functions are made static inside systemDictionary.hpp.
>
> The only executable change is that handle_parallel_super_load used to have a wait while the class's superclass was being loaded after calling resolve_super_or_fail.  It was similar to the subsequent code that waited for the LOAD_INSTANCE placeholder, immediately following the return from handle_parallel_super_load.  I consolidated this code to wait for both conditions and kept the comment from handle_parallel_super_load.  This wait is now in the function handle_parallel_loading.
>
> I also added a new load_instance_class that calls load_instance_class_impl.  The new load_instance_class does constraint checking and bookkeeping after it is called so that it can return with CHECK_NULL and not check pending exceptions. These functions aren't called outside of systemDictionary.cpp.
>
> I added a test to show why handle_parallel_super_load is needed (see bug for details of the deadlock that this test would get if not for handle_parallel_super_load).
>
> This updates comments to:
> 1. rewrote ClassCircularityError detection above resolve_super_or_fail so that it made more sense to me (hope it does for others too).
> 2. move comments to be near the code they describe
> 3. removed some comments that referred to code somewhere else but not helpful at that location, or to describe code that was deleted or obsolete, or repetitive
> 4. added a comment for resolve_instance_class_or_null
>
> Ran tier1-3 testing, jck vm, jck lang and our internal parallel class loading tests.  Retesting with tiers 4-8 in progress (I tested an earlier version of this with no failures).

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

  remove example before resolve_super_or_fail.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3200/files
  - new: https://git.openjdk.java.net/jdk/pull/3200/files/23cbf92a..e357d971

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

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

PR: https://git.openjdk.java.net/jdk/pull/3200
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262046: Clean up parallel class loading code and comments [v2]

Ioi Lam-2
In reply to this post by Coleen Phillimore-3
On Wed, 31 Mar 2021 16:32:35 GMT, Coleen Phillimore <[hidden email]> wrote:

>> src/hotspot/share/classfile/systemDictionary.cpp line 383:
>>
>>> 381: // the super class directly, then resume steps 4-6.
>>> 382: //
>>> 383: //
>>
>> resolve_super_or_fail had this comment that I found difficult to understand and work through, which I did at one point, so I wanted to replace it with one that seemed less so.  It occurs to me that this is probably unhelpful also.   Maybe the whole comment from lines 351 to 381 should be replaced with a sentence like:
>>
>> //resolve_super_or_fail adds a LOAD_SUPER placeholder to the placeholder table before calling
>> // resolve_instance_class_or_null.  ClassCircularityError is detected when a LOAD_SUPER or LOAD_INSTANCE
>> // placeholder for the same thread, class, classloader is found.
>>
>> I'm unlikely to work through the new comment example again myself.
>
> Since nobody wants to read this example in this comment and neither do I, I'm removing it.  Debugging this using -Xlog:class+load+placeholders=debug will show the stacking of placeholders that detect CCE.

I guess it's OK to remove this comment, since most of us who have looked at this code in the past few years haven't found this comment to be helpful. Maybe you can add a comment about `-Xlog:class+load+placeholders=debug` instead?

If we do want to improve the comment, maybe we can replacing Base->Super with something like Car->Vehicle. That way it's clear which is the supertype.

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

PR: https://git.openjdk.java.net/jdk/pull/3200
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262046: Clean up parallel class loading code and comments [v4]

Coleen Phillimore-3
In reply to this post by Coleen Phillimore-3
> This change reduces the length of SystemDictionary::resolve_instance_class_or_null, the main class loading function to 4 pages on my screen, down from ~6.   Some functions are made static inside systemDictionary.hpp.
>
> The only executable change is that handle_parallel_super_load used to have a wait while the class's superclass was being loaded after calling resolve_super_or_fail.  It was similar to the subsequent code that waited for the LOAD_INSTANCE placeholder, immediately following the return from handle_parallel_super_load.  I consolidated this code to wait for both conditions and kept the comment from handle_parallel_super_load.  This wait is now in the function handle_parallel_loading.
>
> I also added a new load_instance_class that calls load_instance_class_impl.  The new load_instance_class does constraint checking and bookkeeping after it is called so that it can return with CHECK_NULL and not check pending exceptions. These functions aren't called outside of systemDictionary.cpp.
>
> I added a test to show why handle_parallel_super_load is needed (see bug for details of the deadlock that this test would get if not for handle_parallel_super_load).
>
> This updates comments to:
> 1. rewrote ClassCircularityError detection above resolve_super_or_fail so that it made more sense to me (hope it does for others too).
> 2. move comments to be near the code they describe
> 3. removed some comments that referred to code somewhere else but not helpful at that location, or to describe code that was deleted or obsolete, or repetitive
> 4. added a comment for resolve_instance_class_or_null
>
> Ran tier1-3 testing, jck vm, jck lang and our internal parallel class loading tests.  Retesting with tiers 4-8 in progress (I tested an earlier version of this with no failures).

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

  Add comment about logging placeholders.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3200/files
  - new: https://git.openjdk.java.net/jdk/pull/3200/files/e357d971..88431ca8

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3200&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3200&range=02-03

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

PR: https://git.openjdk.java.net/jdk/pull/3200
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262046: Clean up parallel class loading code and comments [v4]

Lois Foltan-3
On Thu, 1 Apr 2021 03:11:07 GMT, Coleen Phillimore <[hidden email]> wrote:

>> This change reduces the length of SystemDictionary::resolve_instance_class_or_null, the main class loading function to 4 pages on my screen, down from ~6.   Some functions are made static inside systemDictionary.hpp.
>>
>> The only executable change is that handle_parallel_super_load used to have a wait while the class's superclass was being loaded after calling resolve_super_or_fail.  It was similar to the subsequent code that waited for the LOAD_INSTANCE placeholder, immediately following the return from handle_parallel_super_load.  I consolidated this code to wait for both conditions and kept the comment from handle_parallel_super_load.  This wait is now in the function handle_parallel_loading.
>>
>> I also added a new load_instance_class that calls load_instance_class_impl.  The new load_instance_class does constraint checking and bookkeeping after it is called so that it can return with CHECK_NULL and not check pending exceptions. These functions aren't called outside of systemDictionary.cpp.
>>
>> I added a test to show why handle_parallel_super_load is needed (see bug for details of the deadlock that this test would get if not for handle_parallel_super_load).
>>
>> This updates comments to:
>> 1. rewrote ClassCircularityError detection above resolve_super_or_fail so that it made more sense to me (hope it does for others too).
>> 2. move comments to be near the code they describe
>> 3. removed some comments that referred to code somewhere else but not helpful at that location, or to describe code that was deleted or obsolete, or repetitive
>> 4. added a comment for resolve_instance_class_or_null
>>
>> Ran tier1-3 testing, jck vm, jck lang and our internal parallel class loading tests.  Retesting with tiers 4-8 in progress (I tested an earlier version of this with no failures).
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>
>   Add comment about logging placeholders.

I think this is a good clean up to consolidate the 2 while loops Coleen into SystemDictionary::handle_parallel_loading().  A couple of minor review comments.

Thanks,
Lois

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

> 327:   if (lt.is_enabled()) {
> 328:     ResourceMark rm;
> 329:     LogStream ls(lt);

Is it helpful to pass THREAD in for the ResourceMark?

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

> 339: //    parse_interfaces - from defineClass
> 340: // super-class callers:
> 341: //   ClassFileParser - from defineClass

picky review comment for line #340, comments below do not use a hyphenated "super-class", but instead one work "superclass".

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

> 490: // the superclass on the new thread internally, so we do parallel
> 491: // super class loading here.  This avoids deadlock for ClassCircularity
> 492: // detection for parallelCapable class loaders that lock a per-class lock.

"lock on a per-class lock"

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

> 532:       while (oldprobe != NULL &&
> 533:              (oldprobe->instance_load_in_progress() || oldprobe->super_load_in_progress())) {
> 534:

I thought in preliminary discussions we talked about adding an assert to make sure instance_load_in_progress and super_load_in_progress couldn't both be true.  Am I remembering that correctly?

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

> 784:           if (JvmtiExport::should_post_class_load()) {
> 785:             JvmtiExport::post_class_load(THREAD->as_Java_thread(), loaded_class);
> 786:           }

This seems to be all now handled in load_instance_class, correct?

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

PR: https://git.openjdk.java.net/jdk/pull/3200
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262046: Clean up parallel class loading code and comments [v4]

Ioi Lam-2
In reply to this post by Coleen Phillimore-3
On Thu, 1 Apr 2021 03:11:07 GMT, Coleen Phillimore <[hidden email]> wrote:

>> This change reduces the length of SystemDictionary::resolve_instance_class_or_null, the main class loading function to 4 pages on my screen, down from ~6.   Some functions are made static inside systemDictionary.hpp.
>>
>> The only executable change is that handle_parallel_super_load used to have a wait while the class's superclass was being loaded after calling resolve_super_or_fail.  It was similar to the subsequent code that waited for the LOAD_INSTANCE placeholder, immediately following the return from handle_parallel_super_load.  I consolidated this code to wait for both conditions and kept the comment from handle_parallel_super_load.  This wait is now in the function handle_parallel_loading.
>>
>> I also added a new load_instance_class that calls load_instance_class_impl.  The new load_instance_class does constraint checking and bookkeeping after it is called so that it can return with CHECK_NULL and not check pending exceptions. These functions aren't called outside of systemDictionary.cpp.
>>
>> I added a test to show why handle_parallel_super_load is needed (see bug for details of the deadlock that this test would get if not for handle_parallel_super_load).
>>
>> This updates comments to:
>> 1. rewrote ClassCircularityError detection above resolve_super_or_fail so that it made more sense to me (hope it does for others too).
>> 2. move comments to be near the code they describe
>> 3. removed some comments that referred to code somewhere else but not helpful at that location, or to describe code that was deleted or obsolete, or repetitive
>> 4. added a comment for resolve_instance_class_or_null
>>
>> Ran tier1-3 testing, jck vm, jck lang and our internal parallel class loading tests.  Retesting with tiers 4-8 in progress (I tested an earlier version of this with no failures).
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>
>   Add comment about logging placeholders.

LGTM.

I like the refactoring that combines two similar code paths into the single `handle_parallel_loading` function. The subtle change in behavior probably doesn't matter, but if it did we should be able to catch it with 2+ months of testing before 17 RDP1.

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

Marked as reviewed by iklam (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3200
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262046: Clean up parallel class loading code and comments [v4]

Lois Foltan-3
In reply to this post by Coleen Phillimore-3
On Thu, 1 Apr 2021 03:11:07 GMT, Coleen Phillimore <[hidden email]> wrote:

>> This change reduces the length of SystemDictionary::resolve_instance_class_or_null, the main class loading function to 4 pages on my screen, down from ~6.   Some functions are made static inside systemDictionary.hpp.
>>
>> The only executable change is that handle_parallel_super_load used to have a wait while the class's superclass was being loaded after calling resolve_super_or_fail.  It was similar to the subsequent code that waited for the LOAD_INSTANCE placeholder, immediately following the return from handle_parallel_super_load.  I consolidated this code to wait for both conditions and kept the comment from handle_parallel_super_load.  This wait is now in the function handle_parallel_loading.
>>
>> I also added a new load_instance_class that calls load_instance_class_impl.  The new load_instance_class does constraint checking and bookkeeping after it is called so that it can return with CHECK_NULL and not check pending exceptions. These functions aren't called outside of systemDictionary.cpp.
>>
>> I added a test to show why handle_parallel_super_load is needed (see bug for details of the deadlock that this test would get if not for handle_parallel_super_load).
>>
>> This updates comments to:
>> 1. rewrote ClassCircularityError detection above resolve_super_or_fail so that it made more sense to me (hope it does for others too).
>> 2. move comments to be near the code they describe
>> 3. removed some comments that referred to code somewhere else but not helpful at that location, or to describe code that was deleted or obsolete, or repetitive
>> 4. added a comment for resolve_instance_class_or_null
>>
>> Ran tier1-3 testing, jck vm, jck lang and our internal parallel class loading tests.  Retesting with tiers 4-8 in progress (I tested an earlier version of this with no failures).
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>
>   Add comment about logging placeholders.

Marked as reviewed by lfoltan (Reviewer).

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

PR: https://git.openjdk.java.net/jdk/pull/3200
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262046: Clean up parallel class loading code and comments [v4]

Coleen Phillimore-3
In reply to this post by Coleen Phillimore-3
On Thu, 1 Apr 2021 03:11:07 GMT, Coleen Phillimore <[hidden email]> wrote:

>> This change reduces the length of SystemDictionary::resolve_instance_class_or_null, the main class loading function to 4 pages on my screen, down from ~6.   Some functions are made static inside systemDictionary.hpp.
>>
>> The only executable change is that handle_parallel_super_load used to have a wait while the class's superclass was being loaded after calling resolve_super_or_fail.  It was similar to the subsequent code that waited for the LOAD_INSTANCE placeholder, immediately following the return from handle_parallel_super_load.  I consolidated this code to wait for both conditions and kept the comment from handle_parallel_super_load.  This wait is now in the function handle_parallel_loading.
>>
>> I also added a new load_instance_class that calls load_instance_class_impl.  The new load_instance_class does constraint checking and bookkeeping after it is called so that it can return with CHECK_NULL and not check pending exceptions. These functions aren't called outside of systemDictionary.cpp.
>>
>> I added a test to show why handle_parallel_super_load is needed (see bug for details of the deadlock that this test would get if not for handle_parallel_super_load).
>>
>> This updates comments to:
>> 1. rewrote ClassCircularityError detection above resolve_super_or_fail so that it made more sense to me (hope it does for others too).
>> 2. move comments to be near the code they describe
>> 3. removed some comments that referred to code somewhere else but not helpful at that location, or to describe code that was deleted or obsolete, or repetitive
>> 4. added a comment for resolve_instance_class_or_null
>>
>> Ran tier1-3 testing, jck vm, jck lang and our internal parallel class loading tests.  Retesting with tiers 4-8 in progress (I tested an earlier version of this with no failures).
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>
>   Add comment about logging placeholders.

Thank you for the comments.

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

PR: https://git.openjdk.java.net/jdk/pull/3200
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262046: Clean up parallel class loading code and comments [v4]

Coleen Phillimore-3
On Thu, 1 Apr 2021 20:36:51 GMT, Coleen Phillimore <[hidden email]> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Add comment about logging placeholders.
>
> Thank you for the comments.

Thanks Ioi for reviewing.  I think the refactoring of the waiting loops will make it easier to work with if there is a subtle change in behavior.  I could add a conditional to the function instead of the duplicated code.
Thanks!

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

PR: https://git.openjdk.java.net/jdk/pull/3200
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262046: Clean up parallel class loading code and comments [v4]

Coleen Phillimore-3
In reply to this post by Lois Foltan-3
On Thu, 1 Apr 2021 19:55:38 GMT, Lois Foltan <[hidden email]> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Add comment about logging placeholders.
>
> src/hotspot/share/classfile/systemDictionary.cpp line 329:
>
>> 327:   if (lt.is_enabled()) {
>> 328:     ResourceMark rm;
>> 329:     LogStream ls(lt);
>
> Is it helpful to pass THREAD in for the ResourceMark?

Yes, I have a THREAD handy.

> src/hotspot/share/classfile/systemDictionary.cpp line 341:
>
>> 339: //    parse_interfaces - from defineClass
>> 340: // super-class callers:
>> 341: //   ClassFileParser - from defineClass
>
> picky review comment for line #340, comments below do not use a hyphenated "super-class", but instead one work "superclass".

Ok.  How about a space?  Because the sentence has super-class and super-interface, so I want to change them both to super class and super interface.  Or should it be superclass and superinterface ?

> src/hotspot/share/classfile/systemDictionary.cpp line 492:
>
>> 490: // the superclass on the new thread internally, so we do parallel
>> 491: // super class loading here.  This avoids deadlock for ClassCircularity
>> 492: // detection for parallelCapable class loaders that lock a per-class lock.
>
> "lock on a per-class lock"

Ok.

> src/hotspot/share/classfile/systemDictionary.cpp line 534:
>
>> 532:       while (oldprobe != NULL &&
>> 533:              (oldprobe->instance_load_in_progress() || oldprobe->super_load_in_progress())) {
>> 534:
>
> I thought in preliminary discussions we talked about adding an assert to make sure instance_load_in_progress and super_load_in_progress couldn't both be true.  Am I remembering that correctly?

We were talking about when instance_load_in_progress is false, then we thought we should add an assert that super_load_in_progress is also false, but super_load could still be going on for parallel threads and we have to wait for them too.
ie. I tried this and found this out.  We have to wait for both conditions.

> src/hotspot/share/classfile/systemDictionary.cpp line 786:
>
>> 784:           if (JvmtiExport::should_post_class_load()) {
>> 785:             JvmtiExport::post_class_load(THREAD->as_Java_thread(), loaded_class);
>> 786:           }
>
> This seems to be all now handled in load_instance_class, correct?

Yes, I moved this down to that function.

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

PR: https://git.openjdk.java.net/jdk/pull/3200
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262046: Clean up parallel class loading code and comments [v2]

Coleen Phillimore-3
In reply to this post by Ioi Lam-2
On Wed, 31 Mar 2021 23:55:47 GMT, Ioi Lam <[hidden email]> wrote:

>> Since nobody wants to read this example in this comment and neither do I, I'm removing it.  Debugging this using -Xlog:class+load+placeholders=debug will show the stacking of placeholders that detect CCE.
>
> I guess it's OK to remove this comment, since most of us who have looked at this code in the past few years haven't found this comment to be helpful. Maybe you can add a comment about `-Xlog:class+load+placeholders=debug` instead?
>
> If we do want to improve the comment, maybe we can replacing Base->Super with something like Car->Vehicle. That way it's clear which is the supertype.

Thanks Ioi.

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

PR: https://git.openjdk.java.net/jdk/pull/3200
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262046: Clean up parallel class loading code and comments [v5]

Coleen Phillimore-3
In reply to this post by Coleen Phillimore-3
> This change reduces the length of SystemDictionary::resolve_instance_class_or_null, the main class loading function to 4 pages on my screen, down from ~6.   Some functions are made static inside systemDictionary.hpp.
>
> The only executable change is that handle_parallel_super_load used to have a wait while the class's superclass was being loaded after calling resolve_super_or_fail.  It was similar to the subsequent code that waited for the LOAD_INSTANCE placeholder, immediately following the return from handle_parallel_super_load.  I consolidated this code to wait for both conditions and kept the comment from handle_parallel_super_load.  This wait is now in the function handle_parallel_loading.
>
> I also added a new load_instance_class that calls load_instance_class_impl.  The new load_instance_class does constraint checking and bookkeeping after it is called so that it can return with CHECK_NULL and not check pending exceptions. These functions aren't called outside of systemDictionary.cpp.
>
> I added a test to show why handle_parallel_super_load is needed (see bug for details of the deadlock that this test would get if not for handle_parallel_super_load).
>
> This updates comments to:
> 1. rewrote ClassCircularityError detection above resolve_super_or_fail so that it made more sense to me (hope it does for others too).
> 2. move comments to be near the code they describe
> 3. removed some comments that referred to code somewhere else but not helpful at that location, or to describe code that was deleted or obsolete, or repetitive
> 4. added a comment for resolve_instance_class_or_null
>
> Ran tier1-3 testing, jck vm, jck lang and our internal parallel class loading tests.  Retesting with tiers 4-8 in progress (I tested an earlier version of this with no failures).

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

  Change super class to superclass/superinterface to match the spec which makes them one word.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3200/files
  - new: https://git.openjdk.java.net/jdk/pull/3200/files/88431ca8..4f1b42d5

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3200&range=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3200&range=03-04

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

PR: https://git.openjdk.java.net/jdk/pull/3200
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262046: Clean up parallel class loading code and comments [v6]

Coleen Phillimore-3
In reply to this post by Coleen Phillimore-3
> This change reduces the length of SystemDictionary::resolve_instance_class_or_null, the main class loading function to 4 pages on my screen, down from ~6.   Some functions are made static inside systemDictionary.hpp.
>
> The only executable change is that handle_parallel_super_load used to have a wait while the class's superclass was being loaded after calling resolve_super_or_fail.  It was similar to the subsequent code that waited for the LOAD_INSTANCE placeholder, immediately following the return from handle_parallel_super_load.  I consolidated this code to wait for both conditions and kept the comment from handle_parallel_super_load.  This wait is now in the function handle_parallel_loading.
>
> I also added a new load_instance_class that calls load_instance_class_impl.  The new load_instance_class does constraint checking and bookkeeping after it is called so that it can return with CHECK_NULL and not check pending exceptions. These functions aren't called outside of systemDictionary.cpp.
>
> I added a test to show why handle_parallel_super_load is needed (see bug for details of the deadlock that this test would get if not for handle_parallel_super_load).
>
> This updates comments to:
> 1. rewrote ClassCircularityError detection above resolve_super_or_fail so that it made more sense to me (hope it does for others too).
> 2. move comments to be near the code they describe
> 3. removed some comments that referred to code somewhere else but not helpful at that location, or to describe code that was deleted or obsolete, or repetitive
> 4. added a comment for resolve_instance_class_or_null
>
> Ran tier1-3 testing, jck vm, jck lang and our internal parallel class loading tests.  Retesting with tiers 4-8 in progress (I tested an earlier version of this with no failures).

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

  fix THREAD

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3200/files
  - new: https://git.openjdk.java.net/jdk/pull/3200/files/4f1b42d5..5cf6aa61

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3200&range=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3200&range=04-05

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

PR: https://git.openjdk.java.net/jdk/pull/3200
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262046: Clean up parallel class loading code and comments [v4]

Coleen Phillimore-3
In reply to this post by Coleen Phillimore-3
On Thu, 1 Apr 2021 20:24:10 GMT, Coleen Phillimore <[hidden email]> wrote:

>> src/hotspot/share/classfile/systemDictionary.cpp line 341:
>>
>>> 339: //    parse_interfaces - from defineClass
>>> 340: // super-class callers:
>>> 341: //   ClassFileParser - from defineClass
>>
>> picky review comment for line #340, comments below do not use a hyphenated "super-class", but instead one work "superclass".
>
> Ok.  How about a space?  Because the sentence has super-class and super-interface, so I want to change them both to super class and super interface.  Or should it be superclass and superinterface ?

Lois pointed out that the spec uses superclass and superinterface, so I changed the comments to use this convention.

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

PR: https://git.openjdk.java.net/jdk/pull/3200
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262046: Clean up parallel class loading code and comments [v4]

Coleen Phillimore-3
In reply to this post by Coleen Phillimore-3
On Thu, 1 Apr 2021 20:38:24 GMT, Coleen Phillimore <[hidden email]> wrote:

>> Thank you for the comments.
>
> Thanks Ioi for reviewing.  I think the refactoring of the waiting loops will make it easier to work with if there is a subtle change in behavior.  I could add a conditional to the function instead of the duplicated code.
> Thanks!

Thank you Lois and Ioi for reviewing this and discussions about these changes!

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

PR: https://git.openjdk.java.net/jdk/pull/3200
12