RFR: 8200759: Move GC entries in vmStructs.cpp to GC specific files

[Stefan Karlsson]

On 2018-04-05 13:00, Aleksey Shipilev wrote:
> On 04/04/2018 09:37 PM, Stefan Karlsson wrote:
>> Hi all,
>>
>> Please review this patch to move GC specific entries in vmStructs.cpp out to GC specific files.
>>
>> http://cr.openjdk.java.net/~stefank/8200759/webrev.01/
>> https://bugs.openjdk.java.net/browse/JDK-8200759
> 
> *) It feels awkward to move JavaThread::_satb_mark_queue and JavaThread::_dirty_card_queue, to
> VM_STRUCTS_G1GC, since it is guarded by INCLUDE_ALL_GCS. VM_STRUCTS_GC seems more fitting. (Trivia:
> Shenandoah uses those things too, so they are "shared", even though the implementation is indeed in
> vm/gc/g1).

My follow-up changes change this to INCLUDE_G1GC, so in that sense this 
I don't think it's that awkward.

However, if Shenandoah needs to use this then I agree that it needs to 
be moved. I don't think it needs to be done for this change though.

> 
> *) I'd do:
> 
> #ifdef INCLUDE_ALL_GCS
>    declare_constant_with_value("satbMarkQueueBufferOffset", ...
>    declare_constant_with_value("satbMarkQueueIndexOffset", ...
>    declare_constant_with_value("satbMarkQueueActiveOffset", ...
> #endif
> 
> ...instead of:
> 
>   ALL_GCS_ONLY(declare_constant_with_value("satbMarkQueueBufferOffset", ...)
>   ALL_GCS_ONLY(declare_constant_with_value("satbMarkQueueIndexOffset", ...)
>   ALL_GCS_ONLY(declare_constant_with_value("satbMarkQueueActiveOffset", ...)
> 
> It would be easier with multiple GCs wanting to have a piece of it. For example, it would be easier
> for Shenandoah to do:
> 
> #ifdef (INCLUDE_G1 || INCLUDE_SHENANDOAH)
>    declare_constant_with_value("satbMarkQueueBufferOffset", ...
>    declare_constant_with_value("satbMarkQueueIndexOffset", ...
>    declare_constant_with_value("satbMarkQueueActiveOffset", ...
> #endif
> 
> In other words, ease off on ALL_GCS_ONLY macro, because it is not easily composable.
> 
> Ditto for e.g.:
> 
>   154   ALL_GCS_ONLY(VM_TYPES_CMSGC(declare_type,                               \
>   155                              declare_toplevel_type,                       \
>   156                              declare_integer_type))                       \
>   157   ALL_GCS_ONLY(VM_TYPES_G1GC(declare_type,                                \
>   158                              declare_toplevel_type,                       \
>   159                              declare_integer_type))                       \
>   160   ALL_GCS_ONLY(VM_TYPES_PARALLELGC(declare_type,                          \
>   161                                    declare_toplevel_type,                 \
>   162                                    declare_integer_type))                 \
> 
> ...because you would later like to do:
> 
> #ifdef INCLUDE_CMS
>     VM_TYPES_CMSGC(declare_type,                        \
>                    declare_toplevel_type,               \
>                    integer_type))                       \
> #endif
> #ifdef INCLUDE_G1
>     VM_TYPES_G1GC(declare_type,                        \
>                    declare_toplevel_type,              \
>                    integer_type))                      \
> #endif

I can't really use your suggested constructs, because it won't compile.

StefanK

> 
> Thanks,
> -Aleksey
>