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 00:52:01 UTC 2017


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.

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