RFR 8178604: JVM does not allow defining boot loader modules in exploded build after module system initialization

Lois Foltan lois.foltan at oracle.com
Wed May 10 17:19:30 UTC 2017 

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