RFR 8178604: JVM does not allow defining boot loader modules in exploded build after module system initialization
David Holmes
david.holmes at oracle.com
Thu May 11 02:11:07 UTC 2017
Hi Harold,
On 11/05/2017 7:18 AM, harold seigel wrote:
> Hi Lois, David,
>
> Please review this latest webrev that initializes _exploded_entries
> without locking:
>
> http://cr.openjdk.java.net/~hseigel/bug_8178604.4/webrev/index.html
Ok.
It would be a lot clearer, in my opinion, that the initialization is
indeed thread-safe, if it were done eagerly instead of lazily. The
execution contexts of the first and second calls (in separate threads)
to that chunk of code are not readily discernible by any reasonable
means. But the extensive comment is appreciated.
Thanks,
David
> Thanks, Harold
>
>
> On 5/10/2017 1:19 PM, Lois Foltan wrote:
>> On 5/9/2017 5:18 PM, David Holmes wrote:
>>> On 9/05/2017 11:14 PM, harold seigel wrote:
>>>> Hi David,
>>>>
>>>> See comments embedded below.
>>>>
>>>> Thanks, Harold
>>>>
>>>>
>>>> On 5/8/2017 8:52 PM, David Holmes wrote:
>>>>> Hi Harold,
>>>>>
>>>>> On 9/05/2017 4:00 AM, harold seigel wrote:
>>>>>> Hi David,
>>>>>>
>>>>>> Please see this updated webrev:
>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8178604.3/webrev/index.html
>>>>>>
>>>>>> It contains the changes you requested below.
>>>>>
>>>>> Thank you. I will point out that Lois originally suggested the same
>>>>> changes.
>>>>>
>>>>>> Note that the _exploded_entries table gets created very early during
>>>>>> SystemDictionary initialization, before initializing the pre-loaded
>>>>>> classes. So, the code in load_class() at line ~1503 can assume that
>>>>>> _exploded_entries is not null.
>>>>>
>>>>> Which thread executes ClassLoader::add_to_exploded_build_list, and
>>>>> which thread executes load_class() first? If they are guaranteed to be
>>>>> the same thread then you don't need locking on the construction. If
>>>>> they can be different threads then there must be some synchronization
>>>>> between them that ensures _exploded_entries is properly visible after
>>>>> construction.
>>>> They are the same thread. At startup, the thread calls
>>>
>>> Then as I said, why do we need any locking? I note Lois is the one
>>> that requested this and you questioned it. I also question it.
>>
>> Thank you Harold for investigating this. I stand corrected, it seems
>> that we do not need the lock after all when allocating
>> ClassLoader::_exploded_entries.
>> Lois
>>
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>> SystemDictionary::initialize_preloaded_classes(), which calls
>>>> ClassLoader::classLoader_init2(), which calls
>>>> add_to_exploded_build_list().
>>>>
>>>> Control then returns back to
>>>> SystemDictionary::initialize_preloaded_classes() which eventually calls
>>>> SystemDictionary::load_instance_class(), which calls
>>>> ClassLoader::load_class(). Here's the call stack showing these calls:
>>>>
>>>> V [libjvm.so+0x974c0c] ClassLoader::load_class(Symbol*, bool,
>>>> Thread*)+0x41c
>>>> V [libjvm.so+0x1648339]
>>>> SystemDictionary::load_instance_class(Symbol*, Handle,
>>>> Thread*)+0x819
>>>> V [libjvm.so+0x1648e25]
>>>> SystemDictionary::resolve_instance_class_or_null(Symbol*, Handle,
>>>> Handle, Thread*)+0x985
>>>> V [libjvm.so+0x16494ba] SystemDictionary::resolve_or_null(Symbol*,
>>>> Handle, Handle, Thread*)+0x5a
>>>> V [libjvm.so+0x16498fe] SystemDictionary::resolve_or_fail(Symbol*,
>>>> Handle, Handle, bool, Thread*)+0x1e
>>>> V [libjvm.so+0x1649aa9]
>>>> SystemDictionary::initialize_wk_klass(SystemDictionary::WKID, int,
>>>> Thread*)+0x149
>>>> V [libjvm.so+0x1649bfe]
>>>> SystemDictionary::initialize_wk_klasses_until(SystemDictionary::WKID, SystemDictionary::WKID&,
>>>>
>>>> Thread*)+0x5e
>>>> V [libjvm.so+0x1649dba]
>>>> SystemDictionary::*initialize_preloaded_classes*(Thread*)+0xea
>>>> V [libjvm.so+0x164a28a]
>>>> SystemDictionary::initialize(Thread*)+0x26a
>>>> V [libjvm.so+0x16cd7e0] Universe::genesis(Thread*)+0x7c0
>>>> V [libjvm.so+0x16ce4cc] universe2_init()+0x2c
>>>> V [libjvm.so+0xe0f268] init_globals()+0xa8
>>>> V [libjvm.so+0x1696e6f] Threads::create_vm(JavaVMInitArgs*,
>>>> bool*)+0x2df
>>>> ...
>>>>
>>>> So, is the existing synchronization in this change sufficient?
>>>>
>>>> Thanks, Harold
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>> Thanks, Harold
>>>>>>
>>>>>> On 5/4/2017 5:32 PM, David Holmes wrote:
>>>>>>> Hi Harold,
>>>>>>>
>>>>>>> On 5/05/2017 3:47 AM, harold seigel wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Please review this updated webrev:
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8178604.2/webrev/index.html
>>>>>>>>
>>>>>>>> The updated webrev takes out a lock when creating
>>>>>>>> _exploded_entries. It
>>>>>>>> also contains additional comments and an assert() to address Lois's
>>>>>>>> concerns with reduced code readability involving what situations
>>>>>>>> warrant
>>>>>>>> taking out a lock in ClassLoader::search_module_entries().
>>>>>>>>
>>>>>>>> I don't think there needs to be two different initial entry
>>>>>>>> points as
>>>>>>>> suggested by David below. The method behaves slightly differently
>>>>>>>> depending on the value of its 'need_lock' parameter. But, there is
>>>>>>>> enough common code for it to remain one method.
>>>>>>>
>>>>>>> I think this highlights uncertainty about the concurrent
>>>>>>> behaviour in
>>>>>>> relation to this code. On the one hand you are worried about an
>>>>>>> initialization race and so use the lock, but when you do:
>>>>>>>
>>>>>>> 1500 // The exploded build entries can be added to at any time
>>>>>>> so pass 'true' for
>>>>>>> 1501 // need_lock so that a lock is taken out when searching
>>>>>>> them.
>>>>>>> 1502 assert(_exploded_entries != NULL, "No exploded build
>>>>>>> entries present");
>>>>>>> 1503 stream = search_module_entries(_exploded_entries,
>>>>>>> class_name, file_name, true, CHECK_NULL);
>>>>>>>
>>>>>>> you load _exploded_entries with no lock held and so must be certain
>>>>>>> that you can not possibly read a null value here.
>>>>>>>
>>>>>>> The needs_lock parameter seems superfluous - you know the condition
>>>>>>> for locking is that the list is the _exploded_entries, and you
>>>>>>> assert
>>>>>>> that. So no need to pass in needs_lock, just grab the lock when
>>>>>>> dealing with _exploded_entries.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> David
>>>>>>>
>>>>>>>> Thanks, Harold
>>>>>>>>
>>>>>>>> On 5/3/2017 12:59 AM, David Holmes wrote:
>>>>>>>>> On 3/05/2017 7:11 AM, Lois Foltan wrote:
>>>>>>>>>> On 5/2/2017 9:03 AM, harold seigel wrote:
>>>>>>>>>>> Hi Lois,
>>>>>>>>>>>
>>>>>>>>>>> Thanks for your comments. Please see my in-line responses.
>>>>>>>>>>>
>>>>>>>>>>> Harold
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 5/1/2017 4:14 PM, Lois Foltan wrote:
>>>>>>>>>>>> Hi Harold,
>>>>>>>>>>>>
>>>>>>>>>>>> Looks good. A couple of comments:
>>>>>>>>>>>>
>>>>>>>>>>>> src/share/vm/classfile/classLoader.cpp
>>>>>>>>>>>> line #828 - please take out the Module_lock when the
>>>>>>>>>>>> _exploded_entries is created
>>>>>>>>>>> This fix does not change the possible need for
>>>>>>>>>>> synchronization when
>>>>>>>>>>> _exploded_entries gets created during module system
>>>>>>>>>>> initialization.
>>>>>>>>>>> Are you saying that the existing code is wrong and should have
>>>>>>>>>>> taken
>>>>>>>>>>> out the Module_lock before creating _exploding_entries?
>>>>>>>>>>
>>>>>>>>>> The method ClassLoader::add_to_exploded_build_list can be
>>>>>>>>>> called at
>>>>>>>>>> anytime (during module system initialization and post module
>>>>>>>>>> system
>>>>>>>>>> initialization). Thus it seems prudent that no matter what the
>>>>>>>>>> current
>>>>>>>>>> JVM phase, that any access to ClassLoader::_exploding_entries be
>>>>>>>>>> protected via a lock. That includes creation as well as
>>>>>>>>>> insertion
>>>>>>>>>> into
>>>>>>>>>> the list.
>>>>>>>>>
>>>>>>>>> If creation is not guaranteed to occur before other threads
>>>>>>>>> that will
>>>>>>>>> call the same method and use the _exploded_entries list, then
>>>>>>>>> synchronization of some form is essential. If it is guaranteed
>>>>>>>>> then we
>>>>>>>>> don't need create time sync just to be prudent - but the guarantee
>>>>>>>>> must be firmly established and clearly documented.
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> line #1402 - I like the new factored out method
>>>>>>>>>>>> find_first_module_cpe(), however, instead of adding a new
>>>>>>>>>>>> "need_lock"
>>>>>>>>>>>> parameter, I would rather see code
>>>>>>>>>>>> in find_first_module_cpe() that
>>>>>>>>>>>> takes the
>>>>>>>>>>>> Module_lock if the module_list parameter equals
>>>>>>>>>>>> ClassLoader::_exploded_entries
>>>>>>>>>>> I think your suggestion makes the code less flexible and does
>>>>>>>>>>> not
>>>>>>>>>>> require potential future callers of search_module_entries() to
>>>>>>>>>>> think
>>>>>>>>>>> about synchronization. So, I'd prefer to leave that change
>>>>>>>>>>> as is.
>>>>>>>>>>
>>>>>>>>>> I see your point. My point was based on reduced code
>>>>>>>>>> readability, the
>>>>>>>>>> further away from the decision to establish the lock, the less
>>>>>>>>>> apparent
>>>>>>>>>> it is within the new method ClassLoader::find_first_module_cpe()
>>>>>>>>>> as to
>>>>>>>>>> what situations warranted the lock in the first place.
>>>>>>>>>
>>>>>>>>> Further, this:
>>>>>>>>>
>>>>>>>>> 1495 stream = search_module_entries(_exploded_entries,
>>>>>>>>> class_name, file_name, true, CHECK_NULL);
>>>>>>>>>
>>>>>>>>> is potentially unsafe if we can race with creation because we
>>>>>>>>> first
>>>>>>>>> access _exploded_entries without holding any lock! And the fact
>>>>>>>>> the
>>>>>>>>> method needs to do different things depending on which "list" was
>>>>>>>>> passed in suggests to me it should be two different initial entry
>>>>>>>>> points.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> David
>>>>>>>>> -----
>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Lois
>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Lois
>>>>>>>>>>>>
>>>>>>>>>>>> On 5/1/2017 3:36 PM, harold seigel wrote:
>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Please review this JDK-10 bug fix to allow defining of
>>>>>>>>>>>>> modules to
>>>>>>>>>>>>> the boot loader in exploded builds after module system
>>>>>>>>>>>>> initialization. The fix uses the module lock to synchronize
>>>>>>>>>>>>> access
>>>>>>>>>>>>> to the _exploded_entries data structure (which is only used by
>>>>>>>>>>>>> exploded builds).
>>>>>>>>>>>>>
>>>>>>>>>>>>> Note that the above capability already exists for regular
>>>>>>>>>>>>> builds.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Open Webrev:
>>>>>>>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8178604/webrev/index.html
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8178604
>>>>>>>>>>>>>
>>>>>>>>>>>>> The fix was tested with JCK tests, the JTreg hotspot, java/io,
>>>>>>>>>>>>> java/lang, java/util and other tests, the RBT tier2 -tier5
>>>>>>>>>>>>> tests,
>>>>>>>>>>>>> the co-located NSK tests, and with JPRT. The JTReg and JCK
>>>>>>>>>>>>> tests
>>>>>>>>>>>>> were run with an exploded build. Also, the example program in
>>>>>>>>>>>>> the
>>>>>>>>>>>>> JBS bug was run by hand to verify the fix.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks, Harold
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>
>
More information about the hotspot-runtime-dev
mailing list