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

[Calvin Cheung]

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/

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