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

Wed May 10 21:18:22 UTC 2017

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
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>