Hi Matthias, > On Jul 18, 2017, at 12:24 AM, Baesken, Matthias <matthias.baes...@sap.com> > wrote: > > 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/ > <http://cr.openjdk.java.net/~mbaesken/webrevs/8184323.1/> Looks good. I see Coleen already said she will push the change for you.
Thanks, Jiangli > > > Is a second review needed ? > > > Best regards, Matthias > > From: Jiangli Zhou [mailto:jiangli.z...@oracle.com > <mailto:jiangli.z...@oracle.com>] > Sent: Samstag, 15. Juli 2017 06:37 > 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: 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 > <https://bugs.openjdk.java.net/browse/JDK-8184323> > > > and a webrev : > > http://cr.openjdk.java.net/~mbaesken/webrevs/8184323/ > <http://cr.openjdk.java.net/~mbaesken/webrevs/8184323/> > > > Best regards, Matthias > > > -----Original Message----- > From: Jiangli Zhou [mailto:jiangli.z...@oracle.com > <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
