RFR(M): 8164011: --patch-module support for CDS
Calvin Cheung
calvin.cheung at oracle.com
Thu Sep 15 17:35:49 UTC 2016
Hi Dan,
Here's an updated webev:
http://cr.openjdk.java.net/~ccheung/8164011/webrev.02/
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