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

[Lois Foltan]

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.

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

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