RFR (M) 8199282: Remove ValueObj class for allocation subclassing for gc code

[coleen.phillimore at oracle.com]

On 3/12/18 4:51 PM, Kim Barrett wrote:
>> On Mar 12, 2018, at 10:49 AM, coleen.phillimore at oracle.com wrote:
>>
>> This is the last of the installments of removing ValueObj for allocation subclassing.  This removes the type from allocation.hpp. This also removes some #includes for allocation.hpp from gc files that no longer need it for VALUE_OBJ_CLASS_SPEC subclassing.  We could do an additional pass to remove more allocation.hpp and globalDefinitions.hpp from files or remove this #include on a case-by-case basis.   That is, be aware that it may no longer be needed in any header files you happen to be working on.  Thanks to Kim for pointing this out.
>>
>> Tested with mach5 tier1 and 2 on linux-x64, solaris-sparcv9, macos-x64 and windows-x64.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8199282.01/webrev
>> bug link https://bugs.openjdk.java.net/browse/JDK-8199282
>>
>> Thanks,
>> Coleen
>
> ------------------------------------------------------------------------------
> src/hotspot/share/adlc/arena.hpp
>    28 // All classes in adlc may be subclassed
>    29 // by one of the following allocation classes:
>
> I think the change of "must" to "may" doesn't really describe the
> intent here.  This also has terminology problems similar to those
> discussed for allocation.hpp below.  Better would be to use similar
> wording, or perhaps even refer to allocation.hpp.

I just changed subclassed to derived in this comment.  adlc is it's own 
beast and it's not part of the jvm.  And CHeapObj calls the global 
operator new and delete here.  StackObj isn't used.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1YoungGenSizer.hpp
>
> Probably only needs globalDefinitions.hpp now, rather than allocation.hpp.

Fixed.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/shared/oopStorage.inline.hpp
>
> The include of oop.hpp should not be removed.

This was a mistake, I've restored it.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/memory/allocation.hpp
>    40 // For convenience and documentation of intended use, classes in the virtual machine
>    41 // may be subclassed by one of the following allocation classes. These provide default
>
> [pre-existing messed up terminology]
>
> These allocation classes don't subclass anything; the phrase
> "subclassed by" is wrong.  It should be "derived from".

You are right, see below:
>
> ------------------------------------------------------------------------------
> src/hotspot/share/memory/allocation.hpp
>    42 // [...].  If not subclassed with the following,
>    43 // the global operator new may not be used and will give a fatal error at runtime.
>
> The phrase "If not subclassed with"
>
> I think better would be something like "The virtual machine must never
> call one of the global allocation or deallocation functions
> (operator new(size_t), operator new[](size_t), operator delete(void*), or
> operator delete[](void*)).
>
> Note that the set of functions changes with C++14, and again with
> C++17.
How about wording thus.  I don't want to specify list of new and delete 
operators for that reason.


// For convenience and documentation of intended use, classes in the 
virtual machine
// may be derived from one of the following allocation classes. These 
provide default
// operator new and delete functions.  If not derived with the following,
// the global operators for new and delete may not be used and will give 
a fatal error at
// both linktime and runtime.
// Note: std::malloc and std::free should never called directly.

> ------------------------------------------------------------------------------
> src/hotspot/share/memory/allocation.hpp
>    91 // We avoid the superclass in product mode since some C++ compilers add
>    92 // a word overhead for empty super classes, also this has a vptr for print functions.
>
> It's not just some compilers, as there are situations where no compiler
> can provide the empty base optimization; see the corresponding
> discussion in JDK-8173070.  How about just saying "We avoid the
> superclass in product mode to save space."

Okay, I'll just say that.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/memory/memRegion.hpp
>    37 // objects. These should never be allocated in heap but we do
>    38 // create MemRegions (in CardTableModRefBS) in heap so operator
>    39 // new and operator new [] added for this special case.
>
> "These should never be allocated in heap", except we do.  Bleh!
>
> And really, we only allocate MemRegion[]'s. It would probably be
> better to just make the couple of places that do so instead use
> NEW_C_HEAP_ARRAY and the like, rather than cluttering this class with
> allocation functions.

That's pre-existing.  I'm not going to touch that.   Please file an RFE 
because I'm not exactly sure what the deal is here.
> ------------------------------------------------------------------------------
> src/hotspot/share/runtime/osThread.hpp
>
>   56 // I'd make OSThread a ValueObj embedded in Thread to avoid an indirection, but
>    57 // the assembler test in java.cpp expects that it can install the OSThread of
>    58 // the main thread into its own Thread at will.
>
> Removal of the comment doesn't seem right, unless we're really never
> going to change this.  Just deleting "a ValueObj" would be sufficient
> here.
>
> Any idea what assembler test is being referred to here?  I didn't look
> too hard, but I didn't find what prevents embedding OSThread into
> Thread.

I think the entire comment is obsolete and is something that doesn't 
make sense to change.  No idea what the assembler test is.  Rather than 
trying to reword a now meaningless comment, I removed it.

thanks,
Coleen


>
> ------------------------------------------------------------------------------
>