RFR(M): 8164011: --patch-module support for CDS

[Calvin Cheung]

On 9/14/16, 4:36 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/
>>
>> The jdk changes for exporting the ZIP_FreeEntry function:
>> http://cr.openjdk.java.net/~ccheung/8164011/jdk/webrev.00/
>
> Forgot to post on this part:
>
> make/lib/CoreLibraries.gmk
>     No comments.
>
> make/mapfiles/libzip/mapfile-vers
>     No comments.
>
> src/java.base/share/native/libzip/zip_util.c
>     No comments.
>
> src/java.base/share/native/libzip/zip_util.h
>     Don't like the new line-break, but it looks like that's
>     the style for this file.
Yes, I was following the same style in that file.
>
> Thumbs up.
Thanks again.

Calvin
>
>
> Dan
>
>>
>> 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
>>>>
>