RFR(M): 8164011: --patch-module support for CDS
Ioi Lam
ioi.lam at oracle.com
Fri Sep 16 17:28:43 UTC 2016
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