'Re: RFR 8178604: JVM does not allow defining boot loader modules in exploded build after module syst'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR 8178604: JVM does not allow defining boot loader modules in exploded build after module syst
From:       harold seigel <harold.seigel () oracle ! com>
Date:       2017-05-17 13:04:57
Message-ID: cd6a7260-edab-444b-40bb-c7547b56778a () oracle ! com
[Download RAW message or body]

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

[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic