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

Tue Mar 13 11:22:00 UTC 2018

On 3/12/18 11:11 PM, Vladimir Kozlov wrote:
> On 3/12/18 7: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"
>>
>> 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?
>
>
> I don't think anything shared - adlc has own adlc/arena.hpp. I think 
> it was copied from memory/allocation.hpp and simplified.
>
> src/share/vm/memory/allocation.hpp
> date and time created 97/04/23 20:56:44 by jrose
>
> src/share/vm/adlc/arena.hpp
> date and time created 98/05/25 19:57:07 by renes
>
> adlc only share few C2 definitions opto/opcodes.hpp and 
> opto/adlcVMDeps.hpp
>
>
> I looked on history and ValueObj was never used by adlc code. I am 
> fine with its removal there.
>
> Thanks,
> Vladimir K

Thank you, Vladimir.  It doesn't seem to use CHeapObj very much or 
StackObj at all.  I think as you said, it was copied from the hotspot code.

Coleen

>
>>
>>
>>>> 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."
>>
>> (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.
>>
>>>> ------------------------------------------------------------------------------ 
>>>>
>>>> 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?
>>
>>