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

[Kim Barrett]

> On Mar 13, 2018, at 8:14 PM, coleen.phillimore at oracle.com wrote:
> 
> 
> 
> On 3/12/18 10:23 PM, Kim Barrett wrote:
>>> On Mar 12, 2018, at 5:21 PM, coleen.phillimore at oracle.com wrote:
>>> 
>>> 
>>> 
>>> 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.
>> Should be "subclassed by" => "derived from"
> 
> // All classes in adlc may be derived
> // from one of the following allocation classes:
> //
> // For objects allocated in the C-heap (managed by: malloc & free).
> // - CHeapObj
> //
> // For classes used as name spaces.
> // - AllStatic
> //
> 
>> 
>> So why does adlc have any of this allocation base class stuff at all?
>> Is there some source code sharing between adlc and hotspot that needs
>> that?
> 
> No.
>> 
>>>> 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.
>> Hm, not all of them provide operator new or operator delete; AllStatic
>> for example.
>> 
>> How about:
>> 
>> "The virtual machine must never call one of the implicitly declared
>> global allocation or deletion functions.  (Such calls may result in
>> link-time or run-time errors.)  For convenience and documentation of
>> intended use, classes in the virtual machine may be derived from one
>> of the following allocation classes, some of which define allocation
>> and deletion functions."
> 
> Sure.  Copied directly.
>> 
>> (Not proposing  any change regarding std::malloc/free.)
>> 
>>>> 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.
>> I've added this to my list of problems with MemRegion.  I'll try to
>> finally turn that list into an RFE.
> 
> Ok.
>> 
>>>> ------------------------------------------------------------------------------
>>>> 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.
>> That just begs the question.  Why isn't OSThread directly embedded in
>> Thread, rather than being a separate object?  Eliminating the
>> indirection seems like it might be useful.  Is there a downside?
>> 
>> 
> I think this was answered.
> 
> How is this now?
> 
> Thanks,
> Coleen

Looks good.