RFR: 8217450: Add PackageEntry::locked_lookup_only

[Lois Foltan]

On 1/22/2019 4:53 PM, Lois Foltan wrote:
> On 1/22/2019 2:35 PM, Claes Redestad wrote:
>>
>>
>> On 2019-01-22 18:10, Lois Foltan wrote:
>>>>>
>>>>> Ok - updated http://cr.openjdk.java.net/~redestad/8217450/open.01/ in
>>>>> place and running some sanity tests before push.
>>>>>
>>>>> Thanks!
>>>>>
>>>>> /Claes
>>>>
>>>> Hi Claes,
>>>>
>>>> The code change in modules.cpp lines #211-214 does not look 
>>>> correct. PackageEntry::locked_create_entry_or_null() will return 
>>>> NULL if the package exists and has already been created.  The code 
>>>> at lines #211-214 need a PackageEntry to be successfully returned 
>>>> for java.base packages created during bootstrapping so we can 
>>>> decrement the refcount for it appropriately.
>>>
>>> Just to clarify further.  Obtaining a PackageEntry itself is not a 
>>> requirement for decrementing the refcount of the Symbol* within the 
>>> table.  However, the assert at line #213 will trigger if 
>>> PackageEntry::locked_create_entry_or_null() returns NULL for an 
>>> existing package.  A PackageEntry can be created for any class 
>>> loaded, such as java/lang/Class and java/lang/Object for example, 
>>> during bootstrapping before java.base is defined to the JVM.  These 
>>> classes are put on a fixup_module_list so that once the java.base 
>>> module is defined the JVM can then fixup their java.lang.Module 
>>> field to be correctly set to java.base.
>>
>> Hi Lois,
>>
>> thanks for looking at this!
>>
>> As you say: when locked_create_entry_or_null() returns NULL this
>> *should* be because the package already exists.  The assert is thus not
>> asserting if pkg == NULL alone, locked_lookup_only() must also return
>> NULL for the assert to trigger: i.e., I'm checking that a NULL really
>> meant that the package already existed, and not something else, such as
>> a failure to allocate the PackageEntry.
>>
>> I thought this was the purpose of the original assert: getting a NULL
>> back from locked_create_entry_or_null() when we *know* there was no pre-
>> existing package should be impossible (we're holding the lock, so that a
>> NULL must be a failure to allocate or worse).
>>
>> Am I missing something here?
>
> Hi Claes,
> No you're not!  I wrongfully assumed you had not changed the 
> conditional of the assert and was only checking if pkg != NULL. Thanks 
> for clarifying, I'm ok with the change.
> Lois

One more minor note though, you could remove the check in the assert for 
pkg != NULL, since the locked_lookup_only will always indicate if a 
PackageEntry was found whether it was already there or just newly created.
Lois


>
>>
>> /Claes
>