RFR 8178604: JVM does not allow defining boot loader modules in exploded build after module system initialization
harold seigel
harold.seigel at oracle.com
Wed May 17 13:04:57 UTC 2017
Thanks David!
Harold
On 5/17/2017 9:02 AM, David Holmes wrote:
> 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