RFR(M): 8164011: --patch-module support for CDS
Calvin Cheung
calvin.cheung at oracle.com
Thu Sep 15 00:12:07 UTC 2016
On 9/14/16, 4:32 PM, Daniel D. Daugherty wrote:
> On 9/14/16 12:52 PM, Calvin Cheung wrote:
>> Hi Ioi,
>>
>> I think I've addressed all your review comments in this updated webrev:
>> http://cr.openjdk.java.net/~ccheung/8164011/webrev.01/
>
> src/share/vm/classfile/classLoader.cpp
> L1708: if ((st.st_mode & S_IFREG) != S_IFREG) { // is
> directory
> L1709: vm_exit_during_initialization(
> L1710: "Cannot have directory in --patch-module
> during dumping", buf);
>
> This check should be done like this:
>
> if ((st.st_mode & S_IFMT) != S_IFREG) { // is not a
> regular file
Either way is fine, I think.
In /usr/include/sys/stat.h:
# define S_IFMT __S_IFMT
# define S_IFDIR __S_IFDIR
# define S_IFCHR __S_IFCHR
# define S_IFBLK __S_IFBLK
# define S_IFREG __S_IFREG
In /usr/include/bits/stat.h:
#define __S_IFMT 0170000 /* These bits determine file type. */
/* File types. */
#define __S_IFDIR 0040000 /* Directory. */
#define __S_IFCHR 0020000 /* Character device. */
#define __S_IFBLK 0060000 /* Block device. */
#define __S_IFREG 0100000 /* Regular file. */
There are 3 other places in classLoader.cpp using the same check as in
my webrev.
Should I change those too?
> vm_exit_during_initialization(
> "--patch-module requires a regular file during
> dumping", buf);
>
> src/share/vm/classfile/classLoader.hpp
> No comments.
>
> src/share/vm/classfile/sharedPathsMiscInfo.cpp
> L152: if ((st.st_mode & S_IFREG) != S_IFREG) {
> L153: return fail("Wanted a file but it has become a
> directory");
> L154: }
>
> This check should be done like this:
>
> if ((st.st_mode & S_IFMT) != S_IFREG) {
> return fail("Did not get a regular file as expected.");
> }
>
> L181 if ((strncmp(module_name, path, n) != 0 ) ||
> nit: extra blank after '0'.
>
> src/share/vm/classfile/sharedPathsMiscInfo.hpp
> No comments.
>
> src/share/vm/memory/filemap.cpp
> L952: // More check will be performed on individual
> --patch-module entry in the
> typo: 'check' -> 'checks'
>
> src/share/vm/memory/filemap.hpp
> No comments.
>
> src/share/vm/runtime/arguments.cpp
> No comments.
>
> test/runtime/modules/PatchModule/PatchModuleCDS.java
> L43: // Case 1: Test that --patch-module and -Xshare:dump
> are compatibable
> Typo: 'compatibable' -> 'compatible'
Ok. I will update all the above typos and error messages.
>
> L76: .shouldContain("Cannot have directory in
> --patch-module during dumping");
> Update to the new error message if you choose to change it above.
Yes, I can update that too.
I will post an updated webrev after testing.
Thanks for your review.
Calvin
>
> Dan
>
>>
>> The jdk changes for exporting the ZIP_FreeEntry function:
>> http://cr.openjdk.java.net/~ccheung/8164011/jdk/webrev.00/
>>
>> thanks,
>> Calvin
>>
>>
>> On 9/12/16, 9:00 PM, Calvin Cheung wrote:
>>>
>>>
>>> On 9/12/16, 3:20 PM, Ioi Lam wrote:
>>>> Hi Calvin,
>>>> *
>>>> **src/share/vm/classfile/classLoader.cpp*
>>>>
>>>> There's a memory leak here - the entry is not freed:
>>>>
>>>> 322 bool ClassPathZipEntry::stream_exists(const char* name) {
>>>> 323 // enable call to C land
>>>> 324 JavaThread* thread = JavaThread::current();
>>>> 325 ThreadToNativeFromVM ttn(thread);
>>>> 326 // check whether zip archive contains name
>>>> 327 jint name_len, filesize;
>>>> 328 jzentry* entry = (*FindEntry)(_zip, name, &filesize,
>>>> &name_len);
>>>> 329 return (entry != NULL) ? true : false;
>>>> 330 }
>>>>
>>>> I think you need to add a new function pointer like this, and call
>>>> it if <entry> is not NULL.
>>>> FreeEntry = CAST_TO_FN_PTR(FreeEntry_t,
>>>> os::dll_lookup(handle,
>>>> "ZIP_FreeEntry"));
>>>>
>>>> But, ZIP_FreeEntry (in
>>>> jdk/src/java.base/share/native/libzip/zip_util.c) is not exported.
>>>> To export it, you need to edit all the files under
>>>> jdk/make/mapfiles/libzip (grep for ZIP_FindEntry as an example).
>>> It turns out changing only jdk/make/mapfiles/libzip/mapfile-vers is
>>> enough.
>>> The other three files already contain ZIP_FreeEntry.
>>>
>>> While testing my change, I noticed the ReadMappedEntry in
>>> ClassLoader::load_zip_library() is NULL:
>>> ReadMappedEntry = CAST_TO_FN_PTR(ReadMappedEntry_t,
>>> os::dll_lookup(handle, "ZIP_ReadMappedEntry"));
>>>
>>> It will be used in ClassPathZipEntry::open_entry(). I did a search
>>> in the jdk/src/java.base/share/native/libzip and couldn't find the
>>> function. This is unrelated to the current change. I can file a
>>> separate bug if needed.
>>>>
>>>> This line can be removed
>>>> 1416 bool found;
>>>>
>>>> and these lines:
>>>> 1444 found = e->stream_exists(file_name);
>>>> 1445 if (found) {
>>>> can be changed to "if (e->stream_exists(file_name) {"
>>>>
>>>> Typo on line 1489: exploeded -> exploded
>>>>
>>>> This variable seems to be redundant:
>>>> 1504 bool found_patch_mod_entry = false;
>>>>
>>>> I think it's better to move 1523~1525 into here:
>>>>
>>>> 1517 #if INCLUDE_CDS
>>>> 1518 if (is_in_patch_module(file_name)) {
>>>> tty->print_cr("Preload Warning: Skip archiving
>>>> class %s found in --patch-module entry", class_name);
>>>> return NULL;
>>>> }
>>>> 1519 #endif
>>> I've fixed the above.
>>>>
>>>> *test/runtime/modules/PatchModule/PatchModuleCDS.java**
>>>> *
>>>>
>>>> I think this line should be removed. High level description of the
>>>> test should be included as @summary in the header.
>>>> 40 System.out.println("Test that --patch-module and
>>>> -Xshare:dump are compatibable");
>>>>
>>>> Is mods/java.naming supposed to be a directory that doesn't
>>>> exist? I
>>>> don't see any code in the test that creates this. If so, it's
>>>> better
>>>> to use something like "java.naming=no/such/directory"
>>>> 46 "--patch-module=java.naming=mods/java.naming",
>>>
>>> I will fix the above and send a new webrev after more testing.
>>>
>>> thanks,
>>> Calvin
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>> On 9/12/16 11:01 AM, Calvin Cheung wrote:
>>>>> Hi,
>>>>>
>>>>> Please review the following change to allow the use of the
>>>>> --patch-module option with CDS.
>>>>>
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8164011
>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8164011/webrev.00/
>>>>>
>>>>> During dump time, this change checks if a class can be found in
>>>>> the jar entries of the _patch_mod_entries. If so, the class won't
>>>>> be included in the shared archive. It currently does not support
>>>>> non-jar entries in the _patch_mod_entries.
>>>>>
>>>>> Tested with JPRT and hotspot/runtime tests.
>>>>>
>>>>> thanks,
>>>>> Calvin
>>>>
>
More information about the hotspot-runtime-dev
mailing list