RFR: 8261921: ClassListParser::current should be used only by main thread

[Ioi Lam-2]

During -Xshare:dump, ClassListParser::current() should be used only by the main thread, which has created the only ClassListParser instance. Accessing it from other threads could cause intermittent failures. We observed this only on certain hosts with -Xcomp.

The fix is to check for `ClassListParser::is_parsing_thread()` before calling `ClassListParser::current()`. After the fix, I can no longer reproduce the crash.

I also did some renaming and comment cleaning.

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

Commit messages:
 - 8261921: ClassListParser::current should be used only by main thread

Changes: https://git.openjdk.java.net/jdk/pull/2619/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2619&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261921
  Stats: 43 lines in 5 files changed: 26 ins; 7 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2619.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2619/head:pull/2619

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

[David Holmes-2]

On Thu, 18 Feb 2021 02:25:38 GMT, Ioi Lam <[hidden email]> wrote:

> During -Xshare:dump, ClassListParser::current() should be used only by the main thread, which has created the only ClassListParser instance. Accessing it from other threads could cause intermittent failures. We observed this only on certain hosts with -Xcomp.
>
> The fix is to check for `ClassListParser::is_parsing_thread()` before calling `ClassListParser::current()`. After the fix, I can no longer reproduce the crash.
>
> I also did some renaming and comment cleaning.

Hi Ioi,

Thanks for the side-bar explanations on the background here.

Changes seem fine, but a couple of comments below.

Thanks,
David

src/hotspot/share/classfile/classListParser.cpp line 80:

> 78:   assert(_instance == NULL, "must be singleton");
> 79:   _instance = this;
> 80:   Atomic::store(&_parsing_thread, Thread::current());

The use of atomics here seems overkill as we have no issues with atomicity, tearing or ordering, when checking if the field is the current thread. You can't get a false positive and the current thread must always see the value it set.

src/hotspot/share/classfile/classListParser.cpp line 94:

> 92:   Atomic::store(&_parsing_thread, (Thread*)NULL);
> 93:   _instance = NULL;
> 94: }

This gives the impression that we can create a new ClassListParser later, potentially in a different thread - is that really the case?

src/hotspot/share/classfile/classListParser.hpp line 86:

> 84:   };
> 85:
> 86:   static Thread* _parsing_thread;    // the thread that created _instance

I forget whether we still want to use `volatile` here or not.

src/hotspot/share/classfile/systemDictionaryShared.hpp line 251:

> 249:   static bool add_unregistered_class(InstanceKlass* k, TRAPS);
> 250:   static InstanceKlass* lookup_super_for_unregistered_class(Symbol* class_name,
> 251:                             Symbol* super_name,  bool is_superclass);

Indentation is ad-hoc here, please align Symbol

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

Marked as reviewed by dholmes (Reviewer).

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

[Calvin Cheung]

On Thu, 18 Feb 2021 02:25:38 GMT, Ioi Lam <[hidden email]> wrote:

> During -Xshare:dump, ClassListParser::current() should be used only by the main thread, which has created the only ClassListParser instance. Accessing it from other threads could cause intermittent failures. We observed this only on certain hosts with -Xcomp.
>
> The fix is to check for `ClassListParser::is_parsing_thread()` before calling `ClassListParser::current()`. After the fix, I can no longer reproduce the crash.
>
> I also did some renaming and comment cleaning.

Looks good. Just couple of comments.

Thanks,
Calvin

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

> 371:     // Special processing for handling UNREGISTERED shared classes.
> 372:     InstanceKlass* k = SystemDictionaryShared::lookup_super_for_unregistered_class(class_name,
> 373:                            super_name, is_superclass);

Please align `super_name` with `class_name` in the previous line.

src/hotspot/share/classfile/classListParser.hpp line 86:

> 84:   };
> 85:
> 86:   static Thread* _parsing_thread;    // the thread that created _instance

Should this be declared `volatile`?

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

Marked as reviewed by ccheung (Reviewer).

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

[Coleen Phillimore-3]

On Thu, 18 Feb 2021 02:25:38 GMT, Ioi Lam <[hidden email]> wrote:

> During -Xshare:dump, ClassListParser::current() should be used only by the main thread, which has created the only ClassListParser instance. Accessing it from other threads could cause intermittent failures. We observed this only on certain hosts with -Xcomp.
>
> The fix is to check for `ClassListParser::is_parsing_thread()` before calling `ClassListParser::current()`. After the fix, I can no longer reproduce the crash.
>
> I also did some renaming and comment cleaning.

Looks good.

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

Marked as reviewed by coleenp (Reviewer).

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

[Coleen Phillimore-3]

On Thu, 18 Feb 2021 05:35:29 GMT, David Holmes <[hidden email]> wrote:

>> During -Xshare:dump, ClassListParser::current() should be used only by the main thread, which has created the only ClassListParser instance. Accessing it from other threads could cause intermittent failures. We observed this only on certain hosts with -Xcomp.
>>
>> The fix is to check for `ClassListParser::is_parsing_thread()` before calling `ClassListParser::current()`. After the fix, I can no longer reproduce the crash.
>>
>> I also did some renaming and comment cleaning.
>
> src/hotspot/share/classfile/classListParser.hpp line 86:
>
>> 84:   };
>> 85:
>> 86:   static Thread* _parsing_thread;    // the thread that created _instance
>
> I forget whether we still want to use `volatile` here or not.

_parsing_thread and _instance are read by another thread so that's why they have Atomic::load and store.  This should be volatile if they need that level of memory ordering.

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

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

[Ioi Lam-2]

> During -Xshare:dump, ClassListParser::current() should be used only by the main thread, which has created the only ClassListParser instance. Accessing it from other threads could cause intermittent failures. We observed this only on certain hosts with -Xcomp.
>
> The fix is to check for `ClassListParser::is_parsing_thread()` before calling `ClassListParser::current()`. After the fix, I can no longer reproduce the crash.
>
> I also did some renaming and comment cleaning.

Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:

 - Merge branch 'master' of https://github.com/openjdk/jdk into 8261921-classlistparser-only-main-thread
 - changed _parsing_thread to volatile
 - 8261921: ClassListParser::current should be used only by main thread

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2619/files
  - new: https://git.openjdk.java.net/jdk/pull/2619/files/9aeb9c1a..08dc542e

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

  Stats: 5830 lines in 182 files changed: 3711 ins; 1128 del; 991 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2619.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2619/head:pull/2619

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

[Ioi Lam-2]

On Thu, 18 Feb 2021 05:33:46 GMT, David Holmes <[hidden email]> wrote:

>> Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
>>
>>  - Merge branch 'master' of https://github.com/openjdk/jdk into 8261921-classlistparser-only-main-thread
>>  - changed _parsing_thread to volatile
>>  - 8261921: ClassListParser::current should be used only by main thread
>
> src/hotspot/share/classfile/classListParser.cpp line 80:
>
>> 78:   assert(_instance == NULL, "must be singleton");
>> 79:   _instance = this;
>> 80:   Atomic::store(&_parsing_thread, Thread::current());
>
> The use of atomics here seems overkill as we have no issues with atomicity, tearing or ordering, when checking if the field is the current thread. You can't get a false positive and the current thread must always see the value it set.

As we discussed off-line, theoretically we could have tearing if _parsing_thread spanned across a cache line. We don't actually support such platforms today (as you pointed out, Atomic:load is just a naked load for now). However, it's better to use the proper atomic operations for future proofing.

> src/hotspot/share/classfile/classListParser.cpp line 94:
>
>> 92:   Atomic::store(&_parsing_thread, (Thread*)NULL);
>> 93:   _instance = NULL;
>> 94: }
>
> This gives the impression that we can create a new ClassListParser later, potentially in a different thread - is that really the case?

Currently we don't do that, but I want to make sure we have proper clean up. Since we are modifying _instance, we should clean up other statics as well.

> src/hotspot/share/classfile/systemDictionaryShared.hpp line 251:
>
>> 249:   static bool add_unregistered_class(InstanceKlass* k, TRAPS);
>> 250:   static InstanceKlass* lookup_super_for_unregistered_class(Symbol* class_name,
>> 251:                             Symbol* super_name,  bool is_superclass);
>
> Indentation is ad-hoc here, please align Symbol

Fixed.

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

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

[Ioi Lam-2]

On Fri, 19 Feb 2021 02:38:38 GMT, Coleen Phillimore <[hidden email]> wrote:

>> src/hotspot/share/classfile/classListParser.hpp line 86:
>>
>>> 84:   };
>>> 85:
>>> 86:   static Thread* _parsing_thread;    // the thread that created _instance
>>
>> I forget whether we still want to use `volatile` here or not.
>
> _parsing_thread and _instance are read by another thread so that's why they have Atomic::load and store.  This should be volatile if they need that level of memory ordering.

I've changed it to volatile.

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

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

[Ioi Lam-2]

On Fri, 19 Feb 2021 01:14:45 GMT, Calvin Cheung <[hidden email]> wrote:

>> Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
>>
>>  - Merge branch 'master' of https://github.com/openjdk/jdk into 8261921-classlistparser-only-main-thread
>>  - changed _parsing_thread to volatile
>>  - 8261921: ClassListParser::current should be used only by main thread
>
> src/hotspot/share/classfile/systemDictionary.cpp line 373:
>
>> 371:     // Special processing for handling UNREGISTERED shared classes.
>> 372:     InstanceKlass* k = SystemDictionaryShared::lookup_super_for_unregistered_class(class_name,
>> 373:                            super_name, is_superclass);
>
> Please align `super_name` with `class_name` in the previous line.

Fixed.

> src/hotspot/share/classfile/classListParser.hpp line 86:
>
>> 84:   };
>> 85:
>> 86:   static Thread* _parsing_thread;    // the thread that created _instance
>
> Should this be declared `volatile`?

Fixed.

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

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

[David Holmes-2]

On Mon, 22 Feb 2021 23:59:00 GMT, Ioi Lam <[hidden email]> wrote:

>> During -Xshare:dump, ClassListParser::current() should be used only by the main thread, which has created the only ClassListParser instance. Accessing it from other threads could cause intermittent failures. We observed this only on certain hosts with -Xcomp.
>>
>> The fix is to check for `ClassListParser::is_parsing_thread()` before calling `ClassListParser::current()`. After the fix, I can no longer reproduce the crash.
>>
>> I also did some renaming and comment cleaning.
>
> Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
>
>  - Merge branch 'master' of https://github.com/openjdk/jdk into 8261921-classlistparser-only-main-thread
>  - changed _parsing_thread to volatile
>  - 8261921: ClassListParser::current should be used only by main thread

Still good.

Thanks,
David

src/hotspot/share/classfile/classListParser.cpp line 84:

> 82:
> 83: bool ClassListParser::is_parsing_thread() {
> 84:   return (Atomic::load(&_parsing_thread) == Thread::current());

Nit: superfluous brackets on a binary operator

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

Marked as reviewed by dholmes (Reviewer).

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

[Ioi Lam-2]

> During -Xshare:dump, ClassListParser::current() should be used only by the main thread, which has created the only ClassListParser instance. Accessing it from other threads could cause intermittent failures. We observed this only on certain hosts with -Xcomp.
>
> The fix is to check for `ClassListParser::is_parsing_thread()` before calling `ClassListParser::current()`. After the fix, I can no longer reproduce the crash.
>
> I also did some renaming and comment cleaning.

Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:

  removed unnecessary parenthesis

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2619/files
  - new: https://git.openjdk.java.net/jdk/pull/2619/files/08dc542e..fd902f17

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

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

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

[Ioi Lam-2]

On Fri, 19 Feb 2021 01:17:48 GMT, Calvin Cheung <[hidden email]> wrote:

>> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   removed unnecessary parenthesis
>
> Looks good. Just couple of comments.
>
> Thanks,
> Calvin

Thanks @calvinccheung @coleenp @dholmes-ora for the review!

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

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

[Ioi Lam-2]

On Thu, 18 Feb 2021 02:25:38 GMT, Ioi Lam <[hidden email]> wrote:

> During -Xshare:dump, ClassListParser::current() should be used only by the main thread, which has created the only ClassListParser instance. Accessing it from other threads could cause intermittent failures. We observed this only on certain hosts with -Xcomp.
>
> The fix is to check for `ClassListParser::is_parsing_thread()` before calling `ClassListParser::current()`. After the fix, I can no longer reproduce the crash.
>
> I also did some renaming and comment cleaning.

This pull request has now been integrated.

Changeset: 8cfea7c5
Author:    Ioi Lam <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/8cfea7c5
Stats:     42 lines in 5 files changed: 25 ins; 7 del; 10 mod

8261921: ClassListParser::current should be used only by main thread

Reviewed-by: dholmes, ccheung, coleenp

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

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