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

David Holmes david.holmes at oracle.com
Tue May 9 21:18:46 UTC 2017 

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.

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