RFR (M): 8238854: Remove superfluous C heap allocation failure checks

[Kim Barrett]

> On Feb 12, 2020, at 7:16 AM, Stefan Johansson <stefan.johansson at oracle.com> wrote:
> 
> Hi Thomas,
> 
>> 12 feb. 2020 kl. 13:10 skrev Thomas Schatzl <thomas.schatzl at oracle.com>:
>> 
>> Hi Stefan,
>> 
>> thanks for your review.
>> 
>> On 12.02.20 12:38, Stefan Johansson wrote:
>>> Hi Thomas,
>>>> 11 feb. 2020 kl. 14:30 skrev Thomas Schatzl <thomas.schatzl at oracle.com>:
>>>> 
>>>> Hi all,
>>>> 
>>>> can I have reviews for this change that removes superfluous C heap allocation failure checks (basically hard-exiting the VM) because by default C allocation already exits the VM.
>>>> 
>>>> CR:
>>>> https://bugs.openjdk.java.net/browse/JDK-8238854
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~tschatzl/8238854/webrev/
>>> Nice cleanup in general, but MemRegion doesn’t derive from CHeapObj and will return NULL on failure:
>>> void* MemRegion::operator new(size_t size) throw() {
>>>  return (address)AllocateHeap(size, mtGC, CURRENT_PC,
>>>    AllocFailStrategy::RETURN_NULL);
>>> }
>>> So we should either change this to use the default AllocFailStrategy or keep the checks.
>> 
>> Nice catch. I opted to revert the changes for MemRegion allocation.
>> 
>> Although I think all users of new for MemRegion expect it to fail (the only other user in filemap.cpp will crash with NPE a few lines after allocation), this needs more investigation because the change introducing the new operator talks about some clang compatibility issue (from 2003). But it also indicates that the problem occurred only on a use that is not in the code base any more (JDK-8021954 ftr, it is a closed issue that can't be opened).
>> 
>> (Note that my testing did not reproduce the failure, but, the code is not used in the crashing component, i.e. metaspace handling, any more).
>> 
>> http://cr.openjdk.java.net/~tschatzl/8238854/webrev.0_to_1/ (diff)
>> http://cr.openjdk.java.net/~tschatzl/8238854/webrev.1/ (full)
> 
> I agree with your reasoning above and think this is good.
> 
> Thanks,
> Stefan

I agree too.  Looks good.

Maybe file an RFE to look at this?  MemRegion allocator functions are declared throw(), which is
atypical and definitely strange for us.  When building with gcc we use -fcheck-new.  I’m not sure
how those interact, or exactly what -fcheck-new does, or whether we actually need -fcheck-new.