RFR (L) 8194812: Extend class-data sharing to support the module path
Calvin Cheung
calvin.cheung at oracle.com
Mon Apr 9 19:39:32 UTC 2018
Thanks, Jiangli!
Calvin
On 4/9/18, 10:04 AM, Jiangli Zhou wrote:
> Looks ok.
>
> Thanks,
> Jiangli
>
>> On Apr 8, 2018, at 11:48 PM, Calvin Cheung<calvin.cheung at oracle.com> wrote:
>>
>>
>>
>> On 4/6/18, 6:26 PM, Jiangli Zhou wrote:
>>> Hi Calvin,
>>>
>>> The incremental changes look good.
>>>
>>> Lois’ comment regarding the boot loader was correct (and made me look at the code more closely, thanks Lois!). I think the confusing came form line 1628, which I missed in previous review.
>>>
>>>
>>> 1627 if ((i>= 1)&&
>>> 1628 (i< ClassLoaderExt::app_module_paths_start_index())) {
>>> 1629 // The class must be from -Xbootclasspath/a
>>>
>>> Checking ‘i< ClassLoaderExt::app_module_paths_start_index()’ would include the path component(s) from the -cp, which is incorrect for boot loader. The check needs to be narrowed to ‘i>=0&& i< ClassLoaderExt::app_class_paths_start_index()’.
>> I've fixed the error in line #1628. Thanks for catching it.
>> I think line #1627 is fine because for the 'i == 0' case, it is being handled in:
>>
>> 1647 // for index 0 and the stream->source() is the modules image or has the jrt: protocol.
>> 1648 // The class must be from the runtime modules image.
>> 1649 if (i == 0&& (is_modules_image(src) || string_starts_with(src, "jrt:"))) {
>> 1650 classpath_index = i;
>> 1651 break;
>> 1652 }
>>
>>
>>> Please see more comments below.
>>>
>>>> On Apr 6, 2018, at 11:34 AM, Calvin Cheung<calvin.cheung at oracle.com> wrote:
>>>>
>>>> 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.
>>> The asserts are more useful to be included in the callee functions to prevent any misuse. They are also served as a reminder that the functions are runtime only. How about adding comments specifying these are runtime-only, if you feel strongly against the additional asserts.
>> I've added comments to the 2 functions.
>>
>> Updated webrevs:
>>
>> incremental: http://cr.openjdk.java.net/~ccheung/8194812/webrev.01_to_02/
>>
>> full: http://cr.openjdk.java.net/~ccheung/8194812/webrev.02/
>>
>> thanks,
>> Calvin
>>> Thanks,
>>> Jiangli
>>>
>>>> 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> 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
More information about the hotspot-runtime-dev
mailing list