Hi , thanks for the review .
>Related to the change in universe_init() (universe.cpp), the 'if >(DumpSharedSpaces)’ block at line 715 is also CDS only. >To be consistent, you might want to include it under #if INCLUDE_CDS as well. I added the suggested change and created a second webrev : http://cr.openjdk.java.net/~mbaesken/webrevs/8184323.1/ Is a second review needed ? Best regards, Matthias From: Jiangli Zhou [mailto:jiangli.z...@oracle.com] Sent: Samstag, 15. Juli 2017 06:37 To: Baesken, Matthias <matthias.baes...@sap.com> Cc: hotspot-...@openjdk.java.net; build-dev@openjdk.java.net Subject: Re: RFR: compile-time guard some UseSharedSpaces-only coding with the INCLUDE_CDS macro - was :RE: jdk10 : UseSharedSpaces flag and INCLUDE_CDS macro Hi Baesken, Thank you for making the changes. Related to the change in universe_init() (universe.cpp), the 'if (DumpSharedSpaces)’ block at line 715 is also CDS only. To be consistent, you might want to include it under #if INCLUDE_CDS as well. --- 695,716 ---- Universe::_loader_addClass_cache = new LatestMethodCache(); Universe::_pd_implies_cache = new LatestMethodCache(); Universe::_throw_illegal_access_error_cache = new LatestMethodCache(); Universe::_do_stack_walk_cache = new LatestMethodCache(); + #if INCLUDE_CDS if (UseSharedSpaces) { // Read the data structures supporting the shared spaces (shared // system dictionary, symbol table, etc.). After that, access to // the file (other than the mapped regions) is no longer needed, and // the file is closed. Closing the file does not affect the // currently mapped regions. MetaspaceShared::initialize_shared_spaces(); StringTable::create_table(); ! } else ! #endif ! { SymbolTable::create_table(); StringTable::create_table(); if (DumpSharedSpaces) { MetaspaceShared::prepare_for_dumping(); In the past we have been trying to avoid adding too many #if in the code for better readability. For the CDS only code in universe.cpp (and a few other files), since the callee functions (e.g. initialize_shared_spaces()) are already defined with two versions (with and without INCLUDE_CDS). The builds without CDS also works fine even #if INCLUDE_CDS are not added. Thanks, Jiangli On Jul 13, 2017, at 5:18 AM, Baesken, Matthias <matthias.baes...@sap.com<mailto:matthias.baes...@sap.com>> wrote: Thank you for noticing that. Yes, it would be helpful if you can add the #if INCLUDE_CDS for CDS only code. I can help you integrate the changes after they are reviewed. Thanks for your feedback ! I created a bug : https://bugs.openjdk.java.net/browse/JDK-8184323 and a webrev : http://cr.openjdk.java.net/~mbaesken/webrevs/8184323/ Best regards, Matthias -----Original Message----- From: Jiangli Zhou [mailto:jiangli.z...@oracle.com] Sent: Montag, 10. Juli 2017 17:54 To: Baesken, Matthias <matthias.baes...@sap.com<mailto:matthias.baes...@sap.com>> Cc: hotspot-...@openjdk.java.net<mailto:hotspot-...@openjdk.java.net>; build-dev@openjdk.java.net<mailto:build-dev@openjdk.java.net> Subject: Re: jdk10 : UseSharedSpaces flag and INCLUDE_CDS macro Hi Matthias, Thank you for noticing that. Yes, it would be helpful if you can add the #if INCLUDE_CDS for CDS only code. I can help you integrate the changes after they are reviewed. Thanks! Jiangli On Jul 5, 2017, at 6:36 AM, Baesken, Matthias <matthias.baes...@sap.com<mailto:matthias.baes...@sap.com>> wrote: Hello, when looking into CDS related build options, I noticed that most code-parts that are executed only when UseSharedSpaces is set, are guarded by the compile-time macro INCLUDE_CDS to support switching off compilation of this coding when CDS is disabled at compile time : See hotspot/make/lib/JvmFeatures.gmk : ifneq ($(call check-jvm-feature, cds), true) JVM_CFLAGS_FEATURES += -DINCLUDE_CDS=0 However some places miss the compile-time guarding ( #if INCLUDE_CDS ....) for example in share/vm/prims/jvmtiRedefineClasses.cpp share/vm/memory/universe.cpp (also some other places) Should I prepare a change and add the compile-time guard at the places where missing as well ? Best regards, Matthias
