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

Lois Foltan lois.foltan at oracle.com
Thu May 11 11:13:31 UTC 2017 

This looks good.
Lois

On 5/10/2017 5:18 PM, 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
>
> 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