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 13:02:48 UTC 2017
For the record I post-re-reviewed what was pushed.
Doing the allocation in ClassLoader::classLoader_init2 seems much
cleaner given that immediately after the allocation we call
add_to_exploded_build_list() - where the allocation previously was.
Thanks,
David
On 17/05/2017 1:48 PM, David Holmes wrote:
> 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