RFR(M): 8164011: --patch-module support for CDS
Calvin Cheung
calvin.cheung at oracle.com
Fri Sep 16 17:34:30 UTC 2016
Thanks, Dan and Ioi, for your review.
On 9/16/16, 10:28 AM, Ioi Lam wrote:
> Looks good. Thanks
> - Ioi
>
> On 9/16/16 10:20 AM, Daniel D. Daugherty wrote:
>> Looks good to me.
>>
>> Dan
>>
>>
>> On 9/16/16 11:10 AM, Calvin Cheung wrote:
>>> Hi,
>>>
>>> One more update to the webrev:
>>> - remove the following else block in classLoader.cpp since the
>>> checking is being done in ClassLoader::setup_patch_mod_path()
>>> 1456 } else {
>>> 1457 if (DumpSharedSpaces) {
>>> 1458 vm_exit_during_initialization("Only jar file in
>>> --patch-module is supported for CDS dumping ", e->name());
>>> 1459 }
>>> 1460 }
>>>
>>> - update the comment at line 660 in the same file.
>>>
>>> Only change is in classLoader.cpp:
>>> 1,2c1,2
>>> < --- old/src/share/vm/classfile/classLoader.cpp 2016-09-16
>>> 09:33:50.598085454 -0700
>>> < +++ new/src/share/vm/classfile/classLoader.cpp 2016-09-16
>>> 09:33:50.459075684 -0700
>>> ---
>>> > --- old/src/share/vm/classfile/classLoader.cpp 2016-09-15
>>> 09:43:56.146724124 -0700
>>> > +++ new/src/share/vm/classfile/classLoader.cpp 2016-09-15
>>> 09:43:55.986712875 -0700
>>> 53c53
>>> < + if ((st.st_mode & S_IFMT) != S_IFREG) { // is not a regular file
>>> ---
>>> > + if ((st.st_mode & S_IFMT) != S_IFREG) { // is directory
>>> 92c92
>>> < @@ -1395,6 +1411,57 @@
>>> ---
>>> > @@ -1395,6 +1411,61 @@
>>> 137a138,141
>>> > + } else {
>>> > + if (DumpSharedSpaces) {
>>> > + vm_exit_during_initialization("Only jar file in
>>> --patch-module is supported for CDS dumping ", e->name());
>>> > + }
>>>
>>> new webrev:
>>> http://cr.openjdk.java.net/~ccheung/8164011/webrev.03/
>>>
>>> thanks,
>>> Calvin
>>>
>>> On 9/15/16, 3:14 PM, Daniel D. Daugherty wrote:
>>>> On 9/15/16 3:42 PM, Calvin Cheung wrote:
>>>>>
>>>>>
>>>>> On 9/15/16, 1:39 PM, Daniel D. Daugherty wrote:
>>>>>> On 9/15/16 11:35 AM, Calvin Cheung wrote:
>>>>>>> Hi Dan,
>>>>>>>
>>>>>>> Here's an updated webev:
>>>>>>> http://cr.openjdk.java.net/~ccheung/8164011/webrev.02/
>>>>>>
>>>>>> src/share/vm/classfile/classLoader.cpp
>>>>>> 660 if ((st.st_mode & S_IFMT) != S_IFREG) { // is directory
>>>>>> The comment is wrong. It needs to be:
>>>>>>
>>>>>> if ((st.st_mode & S_IFMT) != S_IFREG) { // is not a
>>>>>> regular file
>>>>> For this fix, I'd prefer to only further update the comment as
>>>>> above since I didn't need to modify the function initially until
>>>>> you noticed the potential issue of not using S_IFMT.
>>>>>
>>>>> Hope it's fine with you.
>>>>
>>>> Sure. A took a quick look a few of the os::dir_is_empty()
>>>> implementations
>>>> and it looks like they all return true if the os::opendir() call
>>>> returns
>>>> NULL. os::opendir() returns NULL when it's asked to open something
>>>> that
>>>> is not a directory.
>>>>
>>>> So in a round about way, we're getting the results we want.
>>>>
>>>> Dan
>>>>
>>>>
>>>>>
>>>>> thanks,
>>>>> Calvin
>>>>>>
>>>>>> Of course, that raises the question of what you really want
>>>>>> this code to do if given a 'path' that refers to something
>>>>>> that is not a directory.
>>>>>>
>>>>>> Perhaps what you want here is this:
>>>>>>
>>>>>> if ((st.st_mode & S_IFMT) == S_IFDIR) { // is a directory
>>>>>> <snip your existing if-block>
>>>>>> } else {
>>>>>> // some new error handling for a 'path' that exists,
>>>>>> but is
>>>>>> // not a directory.
>>>>>> }
>>>>>>
>>>>>> src/share/vm/classfile/classLoader.hpp
>>>>>> No comments.
>>>>>>
>>>>>> src/share/vm/classfile/sharedPathsMiscInfo.cpp
>>>>>> No comments.
>>>>>>
>>>>>> src/share/vm/classfile/sharedPathsMiscInfo.hpp
>>>>>> No comments.
>>>>>>
>>>>>> src/share/vm/memory/filemap.cpp
>>>>>> No comments.
>>>>>>
>>>>>> src/share/vm/memory/filemap.hpp
>>>>>> No comments.
>>>>>>
>>>>>> src/share/vm/runtime/arguments.cpp
>>>>>> No comments.
>>>>>>
>>>>>> test/runtime/modules/PatchModule/PatchModuleCDS.java
>>>>>> No comments.
>>>>>>
>>>>>>
>>>>>> Dan
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> It passed all jtreg tests under runtime dir.
>>>>>>>
>>>>>>> thanks,
>>>>>>> Calvin
>>>>>>>
>>>>>>> On 9/14/16, 7:07 PM, Daniel D. Daugherty wrote:
>>>>>>>> On 9/14/16 6:12 PM, Calvin Cheung wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> No. The S_IFMT value should be used to mask and then a
>>>>>>>> comparison made in order to handle implementations where
>>>>>>>> there are overlaps in the bits used...
>>>>>>>>
>>>>>>>>
>>>>>>>>> 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. */
>>>>>>>>
>>>>>>>> Notice that S_IFDIR overlaps with S_IFBLK so this programming
>>>>>>>> pattern:
>>>>>>>>
>>>>>>>> ((st.st_mode & S_IF<type>) != S_IF<type>)
>>>>>>>>
>>>>>>>> is not a good thing. Yes, it happens to work with S_IFREG, but...
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> There are 3 other places in classLoader.cpp using the same
>>>>>>>>> check as in my webrev.
>>>>>>>>> Should I change those too?
>>>>>>>>
>>>>>>>> Yes. Please fix those other uses.
>>>>>>>>
>>>>>>>>
>>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> No problem. Sorry it took me a while to get to it.
>>>>>>>>
>>>>>>>> Dan
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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