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

Stefan Johansson stefan.johansson at oracle.com
Wed Feb 12 12:16:15 UTC 2020

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,
>  Thomas

More information about the hotspot-gc-dev mailing list