RFR(S): 8166931: Do not include classes which are unusable during run time in the classlist file

[Calvin Cheung]

On 10/11/16, 10:33 AM, Lois Foltan wrote:
>
> On 10/11/2016 1:19 PM, Calvin Cheung wrote:
>> Hi Lois,
>>
>> Thanks for your review.
>>
>> On 10/11/16, 4:38 AM, Lois Foltan wrote:
>>>
>>> On 10/10/2016 11:59 PM, Calvin Cheung wrote:
>>>>
>>>> Please review this small fix for not including classes in the 
>>>> classlist file which are unusable during run time.
>>>>
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8166931
>>>>
>>>> webrev: http://cr.openjdk.java.net/~ccheung/8166931/webrev.00/
>>>
>>> Hi Calvin,
>>>
>>> src/share/vm/classfile/classFileParser.cpp
>>> - line #5781, I find the if statement logic to be somewhat 
>>> confusing.  This check seems to be only for classes defined to the 
>>> boot and platform class loader.  I am assuming it does not apply to 
>>> the application class loader because there is no way to 
>>> differentiate a class defined to the application class loader from 
>>> being on the --patch-module list and the -classpath?  Is that why 
>>> the if statement logic does not include the application class loader?
>> Yes. We do want to include the classes defined to the app class loader.
> Even if those classes defined to the app class loader are located in 
> --patch-module entries?
Yes. With the fix for JDK-8164011, we currently don't archive any 
classes found in the --patch-module entries.

thanks,
Calvin
>
>>>   Maybe it is enough to improve the comment to something like:
>>>
>>>   // For the boot and platform class loaders, check if the class is 
>>> not found in the java runtime image
>>>   // or the boot loader's appended entries.  This indicates that the 
>>> class must be located on the --patch-module list and
>>>   // is not useable during run time, so should be skipped.
>> I've modified it a little. How about the following?
>> // For the boot and platform class loaders, check if the class is not 
>> found in the java runtime image.
>> // Additional check for the boot class loader is if the class is not 
>> found in the boot loader’s appended
>> // entries. This indicates that the class is not useable during run 
>> time, such as the ones found in the
>> // —patch-module entries, so it should not be included in the 
>> classlist file.
> Looks good, thanks for rewording!
>
>>
>>>
>>> Then please indent the start of line #5782 by one space to show that 
>>> the check for the platform class loader is part of that first || 
>>> expression.
>> I'll fix it.
>>>
>>> test/runtime/modules/PatchModule/PatchModuleClassList.java
>>> - good test!
>> Let me know if you want to see another webrev.
>
> No, I'm all set.
> Thanks,
> Lois
>
>>
>> thanks,
>> Calvin
>>>
>>> Thanks,
>>> Lois
>>>
>>>>
>>>> Testing:
>>>>     JPRT with -testset hotspot
>>>>     jtreg tests under hotspot/runtime on all supported platforms 
>>>> (in progress)
>>>>
>>>> thanks,
>>>> Calvin
>>>
>