RFR 8178604: JVM does not allow defining boot loader modules in exploded build after module system initialization
harold seigel
harold.seigel at oracle.com
Tue May 16 13:36:25 UTC 2017
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.
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