> On Jul 6, 2018, at 1:36 PM, Ioi Lam <ioi....@oracle.com> wrote: > > Hi Jiangli, > > The VM changes look good to me.
Thanks! > > For the tests: I think we need a comment here saying that "mods" is > intentionally empty, and also an explanation why it's not necessary to > actually fill with actual modules? Will do. Thanks, Jiangli > > Thanks > > - Ioi > > >>> 3) ArchivedModuleComboTest.java >>> >>> 55 Path moduleDir = Files.createTempDirectory(userDir, "mods"); >>> >>> I don't see anything got placed under the "mods" dir, is it by design? >> Yes. >> > > > On 7/6/18 12:34 PM, Jiangli Zhou wrote: >> Hi Calvin, >> >> Thanks for the review! Here is the updated webrevs that address the >> feedbacks from you and Ioi: >> >> http://cr.openjdk.java.net/~jiangli/8202035/webrev_inc.01/ >> >> Full webrev: http://cr.openjdk.java.net/~jiangli/8202035/webrev_full.01/ >> >>> On Jul 6, 2018, at 9:15 AM, Calvin Cheung <calvin.che...@oracle.com> wrote: >>> >>> Hi Jiangli, >>> >>> Thanks for this start-up improvement. The changes look good overall. I've >>> the following minor comments. >>> >>> 1) make/hotspot/symbols/symbols-unix >>> >>> 134 JVM_InitializeFromArchive >>> >>> If you want the symbols to be in alphabetical order, the above should be >>> moved after JVM_InitStackTraceElementArray. >> Fixed. >> >>> 2) metaspaceShared.cpp >>> >>> 1927 oop MetaspaceShared::materialize_archived_object(oop obj) { >>> 1928 if (obj != NULL) { >>> 1929 return >>> G1CollectedHeap::heap()->materialize_archived_object(obj); >>> 1930 } >>> 1931 return NULL; >>> 1932 } >>> >>> Instead of two return statements, how about replacing lines 1928 - 1931 >>> with the following? >>> >>> return (obj != NULL) ? >>> G1CollectedHeap::heap()->materialize_archived_object(obj) : NULL; >> The original format probably is slightly easier to read, so I left it >> unchanged. Hope that’s okay with you. >> >>> 3) ArchivedModuleComboTest.java >>> >>> 55 Path moduleDir = Files.createTempDirectory(userDir, "mods"); >>> >>> I don't see anything got placed under the "mods" dir, is it by design? >> Yes. >> >>> For the "dump with --module-path" cases, there seems to be a missing test >>> case with "--show-module-resolution" (similar to Test case 2). >> When --module-path is specified at dump time, system module graph is not >> archived currently. There is no need for additional test case with >> --show-module-resolution in this case since all module objects are created >> as normal. >> >>> >>> 4) CheckArchivedModuleApp.java >>> >>> 53 if (expectArchived && wb.isShared(md)) { >>> 54 System.out.println(name + " is archived. Expected."); >>> 55 } else if (!expectArchived && !wb.isShared(md)) { >>> 56 System.out.println(name + " is not archived. >>> Expected."); >>> 57 } else if (expectArchived) { >>> 58 throw new RuntimeException( >>> 59 "FAILED. " + name + " is not archived. Expect >>> archived."); >>> 60 } else { >>> 61 throw new RuntimeException( >>> 62 "FAILED. " + name + " is archived. Expect not >>> archived."); >>> 63 } >>> >>> I'd suggest the following so that the code is easier to understand: >>> >>> if (expectArchived) { >>> if (wb.isShared(md)) { >>> System.out.println(name + " is archived. Expected."); >>> } else { >>> throw new RuntimeException( >>> "FAILED. " + name + " is not archived. Expect archived."); >>> } >>> } else { >>> if (!wb.isShared(md)) { >>> System.out.println(name + " is not archived. Expected."); >>> } else { >>> throw new RuntimeException( >>> "FAILED. " + name + " is archived. Expect not archived."); >>> } >>> } >> Reformatted as suggested. >> >>> 5) ArchivedModuleWithCustomImageTest.java >>> >>> 178 private static void printCommand(String opts[]) { >>> 179 StringBuilder cmdLine = new StringBuilder(); >>> 180 for (String cmd : opts) >>> 181 cmdLine.append(cmd).append(' '); >>> 182 System.out.println("Command line: [" + cmdLine.toString() + >>> "]"); >>> 183 } >>> >>> Consider putting the above method in ProcessTools.java so that >>> ProcessTools.createJavaProcessBuilder() and the above test can call it and >>> avoiding duplicate code. >>> A separate follow-up bug to address this is fine. >> That sounds good to me. We might need some reformatting for consolidation. I >> will file a follow-up RFE. >> >>> 6) PrintSystemModulesApp.java >>> >>> I don't think it is being used? >> It’s used by ArchivedModuleCompareTest.java. Looks like it was missing from >> the earlier webrev. Thanks for catching that. The file is included in the >> updated webrev. >> >> Thanks! >> Jiangli >> >>> thanks, >>> Calvin >>> >>> On 6/28/18, 4:15 PM, Jiangli Zhou wrote: >>>> This is a follow-up RFE of JDK-8201650 (Move iteration order randomization >>>> of unmodifiable Set and Map to iterators), which was resolved to allow >>>> Set/Map objects being archived at CDS dump time (thanks Claes and Stuart >>>> Marks). In the current RFE, it archives the set of system ModuleReference >>>> and ModuleDescriptor objects (including their referenced objects) in >>>> 'open' archive heap region at CDS dump time. It allows reusing of the >>>> objects and bypassing the process of creating the system ModuleDescriptors >>>> and ModuleReferences at runtime for startup improvement. My preliminary >>>> measurements on linux-x64 showed ~5% startup improvement when running >>>> HelloWorld from -cp using archived module objects at runtime (without >>>> extra tuning). >>>> >>>> The library changes in the following webrev are contributed by Alan >>>> Bateman. Thanks Alan and Mandy for discussions and help. Thanks Karen, >>>> Lois and Ioi for discussion and suggestions on initialization ordering. >>>> >>>> The majority of the module object archiving code are in heapShared.hpp and >>>> heapShared.cpp. Thanks Coleen for pre-review and Eric Caspole for helping >>>> performance tests. >>>> >>>> webrev: http://cr.openjdk.java.net/~jiangli/8202035/webrev.00/ >>>> RFE: https://bugs.openjdk.java.net/browse/JDK-8202035?filter=14921 >>>> >>>> Tested using tier1 - tier6 via mach5 including all new test cases added in >>>> the webrev. >>>> >>>> Following are the details of system module archiving, which are duplicated >>>> in above bug report. >>>> --------------------------------------------------------------------------------------------------------------------------- >>>> Support archiving system module graph when the initial module is unnamed >>>> module from -cp currently. >>>> >>>> Support G1 GC, 64-bit (non-Windows). Requires UseCompressedOops and >>>> UseCompressedClassPointers. >>>> >>>> Dump time system module object archiving >>>> ================================= >>>> At dump time, the following fields in ArchivedModuleGraph are set to >>>> record the system module information created by ModuleBootstrap for >>>> archiving. >>>> >>>> private static SystemModules archivedSystemModules; >>>> private static ModuleFinder archivedSystemModuleFinder; >>>> private static String archivedMainModule; >>>> >>>> The archiving process starts from a given static field in >>>> ArchivedModuleGraph class instance (java mirror object). The process >>>> archives the complete network of java heap objects that are reachable >>>> directly or indirectly from the starting object by following references. >>>> >>>> 1. Starts from a given static field within the Class instance (java >>>> mirror). If the static field is a refererence field and points to a >>>> non-null java object, proceed to the next step. The static field and it's >>>> value is recorded and stored outside the archived mirror. >>>> 2. Archives the referenced java object. If an archived copy of the current >>>> object already exists, updates the pointer in the archived copy of the >>>> referencing object to point to the current archived object. Otherwise, >>>> proceed to the next step. >>>> 3. Follows all references within the current java object and recursively >>>> archive the sub-graph of objects starting from each reference encountered >>>> within the object. >>>> 4. Updates the pointer in the archived copy of referecing object to point >>>> to the current archived object. >>>> 5. The Klass of the current java object is added to a list of Klasses for >>>> loading and initializing before any object in the archived graph can be >>>> accessed at runtime. >>>> >>>> Runtime initialization from archived system module objects >>>> ============================================ >>>> VM.initializeFromArchive(<class>) is called from ArchivedModuleGraph's >>>> static initializer to initialize from the archived module information. >>>> Klasses in the recorded list are loaded, linked and initialized. The >>>> static fields in ArchivedModuleGraph class instance are initialized using >>>> the archived field values. After initialization, the archived system >>>> module objects can be used directly. >>>> >>>> If the archived java heap data is not successfully mapped at runtime, or >>>> there is an error during VM.initializeFromArchive(), then all static >>>> fields in ArchivedModuleGraph are not initialized. In that case, system >>>> ModuleDescriptor and ModuleReference objects are created as normal. >>>> >>>> In non-CDS mode, VM.initializeFromArchive(<class>) returns immediately >>>> with minimum added overhead for normal execution. >>>> >>>> Thanks, >>>> Jiangli >>>> >>>> >
