RFR 8190235: Clarify ClassLoaderData::is_*_class_loader_data() method implementations

[harold seigel]

Hi Volker,

Thanks for looking at this.  Unfortunately, I have already pushed the 
change.

A class loader, including the boot loader, can have multiple class 
loader data's.  These CLD's include the original CLD that is used 
normally used by the class loader and the CLD's that get created by 
calls to Unsafe.defineAnonymousClass().  Every successful call to 
Unsafe.defineAnonymousClass() creates another CLD for the loader that 
loads the anonymous class, even if the loader is the boot loader.

The difference between is_the_null_class_loader_data() and 
is_boot_class_loader_data() is that function 
is_the_null_class_loader_data() only returns TRUE if the CLD is the boot 
loader's original CLD.  Function is_boot_class_loader_data() returns 
TRUE, if it is a boot loader CLD, even if the CLD was created as a 
result of a call to Unsafe.defineAnonymousClass().  So, the two 
functions are not the same.  Perhaps, in retrospect, they could have 
been given better names.

Thanks, Harold


On 2/6/2018 1:49 PM, Volker Simonis wrote:
> Hi Harold,
>
> the change looks good, but why we now need two methods (i.e.
> is_the_null_class_loader_data() and is_boot_class_loader_data()) for
> the same thing?
>
> In general, I more like the name is_boot_class_loader_data(), but
> then, shouldn't we replace all the occurrences of
> is_the_null_class_loader_data() with is_boot_class_loader_data().
> Introducing is_boot_class_loader_data() just for using it a single
> time seem weird to me, or did I missed something?
>
> Regards,
> Volker
>
>
> On Tue, Feb 6, 2018 at 7:38 PM, harold seigel <harold.seigel at oracle.com> wrote:
>> Hi Coleen,
>>
>> Thanks for reviewing this.
>>
>> Note that the rfr adds an assert to ModuleEntry::set_loader_data() to help
>> ensure that a module's class loader data is not anonymous.
>>
>> Harold
>>
>>
>>
>> On 2/6/2018 1:35 PM, coleen.phillimore at oracle.com wrote:
>>>
>>> This looks good.   I don't see any code that assumes
>>> is_builtin_class_loader_data means the same thing as
>>> is_permanent_class_loader_data() but there are a couple of instances in
>>> ModuleEntry::set_read_walk_required and
>>> PackageEntry::set_export_walk_required, but these might be okay because
>>> there isn't a module reads list or exports list on anonymous CLD.
>>> thanks,
>>> Coleen
>>>
>>> On 2/5/18 3:37 PM, harold seigel wrote:
>>>> Hi,
>>>>
>>>> Please review this small JDK-11 RFR to clarify and clean up the
>>>> implementations of the ClassLoaderData::is_*_class_loader_data()
>>>> implementations.  The changes primarily consist of adding some comments.
>>>>
>>>> Open Webrev:
>>>> http://cr.openjdk.java.net/~hseigel/bug_8190235/webrev/index.html
>>>>
>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8190235
>>>>
>>>> The changes were tested with Mach 5 tier1 - tier5 tests and JTReg JDK
>>>> tests.
>>>>
>>>> Thanks, Harold
>>>>