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

[Ioi Lam]

On Thu, 18 Feb 2021 05:33:46 GMT, David Holmes <dholmes at openjdk.org> 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