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

David Holmes david.holmes at oracle.com
Wed May 17 03:48:27 UTC 2017 

Hi Harold,

On 16/05/2017 11:36 PM, harold seigel wrote:
> Hi David,
>
> Thanks for the review!  I'll change the initialization of
> _exploded_entries so that it is done eagerly, before I push the change.

??? If you do that then it needs to be re-reviewed!

David

> Harold
>
>
> On 5/10/2017 10:11 PM, David Holmes wrote:
>> Hi Harold,
>>
>> On 11/05/2017 7:18 AM, 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
>>
>> Ok.
>>
>> It would be a lot clearer, in my opinion, that the initialization is
>> indeed thread-safe, if it were done eagerly instead of lazily. The
>> execution contexts of the first and second calls (in separate threads)
>> to that chunk of code are not readily discernible by any reasonable
>> means. But the extensive comment is appreciated.
>>
>> Thanks,
>> David
>>
>>> 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