RFR (L) 8194812: Extend class-data sharing to support the module path

[Calvin Cheung]

Hi Lois,

On 4/6/18, 7:48 AM, Lois Foltan wrote:
> On 4/5/2018 5:46 PM, Jiangli Zhou wrote:
>
>> Hi Calvin,
>>
>> The changes for supporting module path for CDS/AppCDS looks very good 
>> now. I have just few comments below.
> Hi Calvin,
>
> I agree, changes for this support look very good.  I am interspersing 
> my review comments with Jiangli's because I share here concerns about 
> ClassLoader::record_result().
Thanks for taking a look. Please see my reply below.
>
>>
>> filemap.cpp
>>
>> - The following check should be applied for the module path as well. 
>> We can’t support directory in module path. I think ‘&& is_class_path’ 
>> should be removed.
>>   242   } else if (is_dir() && is_class_path) {
>>   243     if (!os::dir_is_empty(name)) {
>>   244       FileMapInfo::fail_continue("directory is not empty: %s", 
>> name);
>>   245       ok = false;
>>   246     }
>> Also, could you please add comments for the special handling for 
>> module path entry at line 239. If the archived module path entry does 
>> not exist at runtime, it is not fatal (no need to invalid the shared 
>> archive) because the shared runtime visibility check filters out any 
>> archived module classes that do not have a matching runtime module 
>> path location.
>>
>> - Please add assert for UseSharedSpaces in validate() and 
>> validate_shared_path_table().
>>
>> classLoader.cpp
>>
>> - The following change looks new in the latest webrev. These are for 
>> the NULL class loader case, right? Can you please assert the loader 
>> indeed is the NULL class loader.
>> 1627               if ((i >= 1) &&
>> 1628                   (i < 
>> ClassLoaderExt::app_module_paths_start_index())) {
>> 1629                 classpath_index = i;
>> 1630                 break;
>> 1631               }
>
> Hmm, I had concerns about this as well.  If this was for the boot 
> loader then it would imply that this class was being loaded from the 
> class path?  That would be incorrect since the boot loader should only 
> ever load a class from 1. --patch-module, 2. jimage, & 3. boot loader 
> append path.  I read this and thought that section #1627-1631 would 
> only ever apply to either the platform class loader or a non-builtin 
> loader?  Did I misunderstand this?
The above block is actually for a class from the -Xbootclasspath/a. I've 
added an assert to ensure the loader is null as suggested by Jiangli.
>
> This leads into one of my review comments.  I would prefer 
> ClassLoader::record_result() to be very explicit ahead of the "if 
> statement" on line #1605 for the boot loader.  Would you consider 
> adding a "if (loader==NULL)" statement to handle classes for the boot 
> loader prior to line #1605?
Adding "if (loader==NULL)" before line #1605 won't work because line 
#1621-1624 is for a class from the -cp.
I've adjusted the block of code in question to the following (mostly 
comment changes and added an assert):

           // NULL pkg_entry and pkg_entry in an unnamed module implies 
the class
           // is from the -cp or -Xbootclasspath/a.
           if ((pkg_entry == NULL) || (pkg_entry->in_unnamed_module())) {
             // Ensure the index is within the -cp range before assigning
             // to the classpath_index.
             if (SystemDictionary::is_system_class_loader(loader) &&
                 (i >= ClassLoaderExt::app_class_paths_start_index()) &&
                 (i < ClassLoaderExt::app_module_paths_start_index())) {
               classpath_index = i;
               break;
             } else {
               if ((i >= 1) &&
                   (i < ClassLoaderExt::app_module_paths_start_index())) {
                 // The class must be from -Xbootclasspath/a
                 assert(loader == NULL, "sanity");
                 classpath_index = i;
                 break;
               }
             }
           } else {

I'll send an updated webrev after I'm done with tests changes.

thanks,
Calvin
>
> Thanks,
> Lois
>
>> - The following comment could be misleading and needs a little 
>> rework. Named modules can from the runtime image. The code below the 
>> comment handles the module path case only. Please clarify that in the 
>> comment.
>> 1634             // non-NULL pkg_entry and is a named module implies 
>> the class is from the
>> 1635             // --module-path. Ensure the index is within the 
>> --module-path range before
>> 1636             // assigning to the classpath_index.
>>
>> Minor nits:
>>
>> classLoader.cpp
>>
>> - Please fix the indentation at line 1100.
>>
>> - The variable ‘e’ is not used. Please remove.
>> 1102   ClassPathEntry* e = _module_path_entries;
>> - Please add comments for the ‘else’ case at line 1661-1665. The 
>> shared path table is set up after module system initialization. The 
>> path table contains no entry before that. Any classes loaded priorly 
>> must be from the runtime image.
>>
>> - Move the block of code at 1098 ~ 1131 to line 730, so these 
>> functions are close to the code that actually uses them. And we can 
>> also have one less #if INCLUDE_CDS.
>>
>> - Please assert DumpSharedSpaces for these dump-time-only functions 
>> in add_tomodule_path_entries() and update_module_path_entry_list().
>>
>> Thanks,
>> Jiangli
>>
>>> On Apr 2, 2018, at 2:30 PM, Calvin Cheung <calvin.cheung at oracle.com> 
>>> wrote:
>>>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8194812
>>>
>>> webrev: http://cr.openjdk.java.net/~ccheung/8194812/webrev.00/
>>>
>>> Design write-up: 
>>> http://cr.openjdk.java.net/~ccheung/8194812/write-up/module-path-design.pdf
>>>
>>> Testing: hs-tier1 through hs-tier4 (in progress, no new failures so 
>>> far).
>>>
>>> thanks,
>>> Calvin
>