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

[Lois Foltan]

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().

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

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?

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