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

[Calvin Cheung]

Hi Jiangli,

On 4/5/18, 2: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.
Thanks for your review. I've made all of your suggested changes except 
for one. Please refer to comment inline below.
I'm updating the tests per Ioi's suggestions and will send updated 
webrev after that.
>
> 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().
Same assert is already in the caller 
(MetaspaceShared::initialize_runtime_shared_and_meta_spaces()) within 
the call hierarchy.

Call hierarchy:

FileMapInfo::validate_shared_path_table() : bool
     MetaspaceShared::map_shared_spaces(FileMapInfo *) : bool
         MetaspaceShared::initialize_runtime_shared_and_meta_spaces() : void
             Metaspace::global_initialize() : void
                 universe_init() : jint
                     init_globals() : jint
                         Threads::create_vm(JavaVMInitArgs *, bool *) : jint

FileMapInfo::FileMapHeader::validate() : bool
     FileMapHeaderExt::validate() : bool
         FileMapInfo::validate_header() : bool
             FileMapInfo::initialize() : bool
                 
MetaspaceShared::initialize_runtime_shared_and_meta_spaces() : void
                     Metaspace::global_initialize() : void
     FileMapInfo::validate_header() : bool
         FileMapInfo::initialize() : bool
             
MetaspaceShared::initialize_runtime_shared_and_meta_spaces() : void
                 Metaspace::global_initialize() : void
                     universe_init() : jint

So I don't think it's necessary to add assert in those 2 functions.

thanks,
Calvin
>
> 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               }
> - 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 
>> <mailto: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/ 
>> <http://cr.openjdk.java.net/%7Eccheung/8194812/webrev.00/>
>>
>> Design write-up: 
>> http://cr.openjdk.java.net/~ccheung/8194812/write-up/module-path-design.pdf 
>> <http://cr.openjdk.java.net/%7Eccheung/8194812/write-up/module-path-design.pdf>
>>
>> Testing: hs-tier1 through hs-tier4 (in progress, no new failures so far).
>>
>> thanks,
>> Calvin
>