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

[Baesken, Matthias]

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.zhou at oracle.com]
Sent: Samstag, 15. Juli 2017 06:37
To: Baesken, Matthias <matthias.baesken at sap.com>
Cc: hotspot-dev at openjdk.java.net; 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


and a webrev :

http://cr.openjdk.java.net/~mbaesken/webrevs/8184323/


Best regards, Matthias


-----Original Message-----
From: Jiangli Zhou [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