RFR: JDK-8302989: Add missing INCLUDE_CDS checks

[Matthias Baesken]

On Mon, 27 Feb 2023 06:21:14 GMT, David Holmes <dholmes at openjdk.org> wrote:

> So you avoid doing e.g.
> 
> ```
> if (RewriteBytecodes CDS_ONLY(&& !UseSharedSpaces && !DumpSharedSpaces)) {
> ```
> 
> and
> 
> ```
> CDS_ONLY(assert(!UseSharedSpaces, "UsedSharedSpaces not supported with exploded module builds");) ***
> ```
> 
> but what about the code in `Metaspace::global_initialize()`? Looks the the `INCLUDE_CDS` section there needs expanding. And `Universe::genesis` is a bit inconsistent about excluding or not. And `InstanceRefKlass::update_nonstatic_oop_maps` looks like a candidate too. And realistically we shouldn't even be compiling systemDictionaryShared.cpp in a non-CDS build, but that would take a lot of additional conditionalization.
> 

Hi David,  systemDictionaryShared.cpp  is already not compiled in non-CDS mode, see  hotspot/lib/JvmFeatures.gmk .
Regarding the other places you mentioned, `InstanceRefKlass::update_nonstatic_oop_maps` , the UseSharedSpaces section there might indeed be guarded by   `INCLUDE_CDS`  - should I add  this ?   (on the other hand this section includes just asserts so I would expect that the whole code gets optimized out anyway in a product build.

> I'm just not seeing that converting a handful of sites is really beneficial. The flags themselves are not normal flags defined for CDS_ONLY precisely because we wanted them available without having to conditionalize every single usage. I'm not looking to "block" this but I can't really endorse it either.
> 

The conversion of a handful of sites is just an **addition** to the already rather common usage of the macro. Nothin new really ...

> *** there is probably potential for adding a `cds_assert` macro to handle a bunch of CDS-only assertions.

Maybe in a separate issue, that do you think ? Do you have some example places where to use it ?

-------------

PR: https://git.openjdk.org/jdk/pull/12691