RFR: compile-time guard some UseSharedSpaces-only coding with the INCLUDE_CDS macro - was :RE: jdk10 : UseSharedSpaces flag and INCLUDE_CDS macro

[Jiangli Zhou]

Hi Matthias,

> On Jul 18, 2017, at 12:24 AM, Baesken, Matthias <matthias.baesken at 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.zhou at oracle.com <mailto:jiangli.zhou at oracle.com>] 
> Sent: Samstag, 15. Juli 2017 06:37
> To: Baesken, Matthias <matthias.baesken at sap.com <mailto:matthias.baesken at sap.com>>
> Cc: hotspot-dev at openjdk.java.net <mailto:hotspot-dev at openjdk.java.net>; build-dev at openjdk.java.net <mailto:build-dev at 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.baesken at sap.com <mailto:matthias.baesken at 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.zhou at oracle.com <mailto:jiangli.zhou at oracle.com>] 
> Sent: Montag, 10. Juli 2017 17:54
> To: Baesken, Matthias <matthias.baesken at sap.com <mailto:matthias.baesken at sap.com>>
> Cc: hotspot-dev at openjdk.java.net <mailto:hotspot-dev at openjdk.java.net>; build-dev at openjdk.java.net <mailto:build-dev at 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.baesken at sap.com <mailto:matthias.baesken at 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