RFR: 8000617: It should be possible to allocate memory without the VM dying.

Sun Oct 14 16:16:00 PDT 2012

PS. I should also add that if we were to fix this all properly then 
there would be no "alloc-fail-mode" flag and every allocation function 
would be capable of returning null - with the "top-level" deciding how 
the OOM condition should be handled. Ironically because we don't use C++ 
exceptions we have no way to communicate incidental allocation failures 
up the call chain, so we can't fix this properly without changing a lot 
of code (which is why it remains a long standing open RFE).

With all that said perhaps the way forward for Nils is to augment the 
ResourceArea allocation routines in exactly the same way as the CHeapObj 
allocation routines - minimal changes using std::no_throw, and perhaps 
keeping the no_throw=>os::malloc mapping?

David
-----

On 15/10/2012 9:10 AM, David Holmes wrote:
> On 13/10/2012 1:40 AM, Nils Loodin wrote:
>> So in general we have two competing wishes here:
>>
>> 1. Use std::nothrow, in operator new() and enum in the allocation
>> methods. (basically webrev 03)
>> Pros: more c++ standard-ish
>> cons: more boiler plate, not descriptive of what actually happens, two
>> different mechanisms.
>>
>>
>> 2. Use enums everywhere (basically a bugfree version of 04):
>> pros: more accurate description, cleaner implementation
>> cons: it's an instance of "not invented here"-syndrom, ie, we're
>> making up our own stuff.
>
> I'm less supportive of using an Enum over a boolean now. A two-valued
> enum is a boolean. And we are treating it as a boolean any time we use
> if-else rather than a switch-style selector - we couldn't add a third
> option without revisiting every place it is currently checked. And I
> just don't see there being any third option.
>
> I never liked std::nothrow because we never threw in the first place,
> but if it had instead been std::return_null I would have been fine with
> it. But given we use custom multi-arg variants of operator new I don't
> see that the "familiarity"/"standard" argument really has that much
> weight. If anything using a standard feature in a non-standard
> environment is likely to be more confusing to people who might naturally
> expect that if they see a no_throw variant then the other variant will
> in fact throw, not abort the VM!
>
> I have some specific comments on specific changes but there doesn't seem
> much point going to that level of discussion yet. Except I will mention
> that in thread.cpp the pre-existing throw_excpt argument to allocate
> seems to really mean "exit_out_of_memory" and should be renamed.
>
> David
> -----
>
>> So.. discuss! John, what do you think having seen the different
>> implementations? It would be nice to be able to get in any version of
>> it so I can develop the feature that will depend on it... :)
>>
>> Regards,
>> Nisse
>>
>> On Oct 12, 2012, at 17:03 , Coleen
>> Phillimore<coleen.phillimore at oracle.com> wrote:
>>
>>>
>>> I still prefer webrev 03 to webrev 04. std::nothrow as a parameter
>>> means something to any C++ developer and AllocationStrategy::OOM only
>>> means something to a hotspot developer. I think we should use the
>>> standard C++ apis when possible. The duplication of operator new is
>>> not significant. Plus if someone adds new (std::nothrow) call or you
>>> forgot to clean up one, it think it will overload to the
>>> std::operator new() which could lead to a subtle bug.
>>>
>>> Coleen
>>>
>>> On 10/12/2012 10:40 AM, Nils Loodin wrote:
>>>>
>>>>
>>>>
>>>> 12 okt 2012 kl. 16:25 skrev Keith McGuigan<keith.mcguigan at oracle.com>:
>>>>
>>>>> May as well lift the allocation fail strategy up into
>>>>> Thread::allocate() as well, replacing the boolean parameter there.
>>>>>
>>>> Ah, good idea.
>>>>
>>>>> I think you're replacements of new(std::nothrow) XXX() wit new
>>>>> (EXIT_OOM) XXX() are all reversed: if it was nothrow you want to
>>>>> replace it with RETURN_NULL.
>>>>>
>>>>> -
>>>> Argh, darn, you're right of course.
>>>>
>>>> Will fix.
>>>>
>>>>> - Keith
>>>>>
>>>>> On Oct 12, 2012, at 9:14 AM, Nils Loodin wrote:
>>>>>
>>>>>> Hey guys!
>>>>>>
>>>>>> Thanks for yet another round of informative code reviews!
>>>>>> So I got rid of all the instances of std::nothrow throughout the
>>>>>> code and replaced them with a new shiny and descriptive enum.
>>>>>>
>>>>>> Was able to fold together a few specialized operator new() because
>>>>>> of it, with more shared code as a result.
>>>>>>
>>>>>> What do you think now?
>>>>>> http://cr.openjdk.java.net/~nloodin/8000617/webrev.04/
>>>>>>
>>>>>> Have a nice weekend!
>>>>>>
>>>>>> Regards,
>>>>>> Nils Loodin
>>>>>>
>>>>>>
>>>>>> On 10/12/2012 06:47 AM, David Holmes wrote:
>>>>>>> On 12/10/2012 2:21 PM, David Holmes wrote:
>>>>>>>> Hi Nils,
>>>>>>>>
>>>>>>>> There are a number of existing bugs/RFEs in this area - not sure we
>>>>>>>> needed yet another.
>>>>>>>>
>>>>>>>> On 11/10/2012 10:55 PM, Nils Loodin wrote:
>>>>>>>>> Hey guys.
>>>>>>>>>
>>>>>>>>> Your comments on this issue are great! So here's another
>>>>>>>>> version that
>>>>>>>>> uses an enum instead of std::nothrow_t trickery!
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~nloodin/8000617/webrev.03/
>>>>>>>> I dislike the std::nothrow usage that NMT introduced (there
>>>>>>>> aren't even
>>>>>>> Apologies it wasn't NMT that introduced this - it is a bit older
>>>>>>> than
>>>>>>> that: April 2011. See
>>>>>>> https://jbs.oracle.com/bugs/browse/JDK-7036747
>>>>>>>
>>>>>>> and some comments in
>>>>>>>
>>>>>>> https://jbs.oracle.com/bugs/browse/JDK-4719004
>>>>>>>
>>>>>>> Till then we had avoided any use of C++ std:: stuff - particularly
>>>>>>> anything related to the exception mechanism, which we don't use
>>>>>>> at all.
>>>>>>>
>>>>>>>> any exceptions involved!) but introducing a completely different
>>>>>>>> mechanism seems counter-productive. I prefer the "alloc fail
>>>>>>>> strategy"
>>>>>>>> approach but would like to see this solved holistically, replacing
>>>>>>>> std::nothrow. Given there exist bigger RFE's to have the VM
>>>>>>>> handle all
>>>>>>>> OOM situations gracefully rather than just aborting, I'd rather
>>>>>>>> not see
>>>>>>>> just another point-patch put in place.
>>>>>>> The bigger problem is probably still too big to really handle - so
>>>>>>> either copy what we already introduced for CHeapObj to
>>>>>>> ResourceObj for
>>>>>>> consistency, or replace both with something better.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>>> For the record I found the original proposal extremely confusing:
>>>>>>>> nothrow_constant vs throw_constant.
>>>>>>>>
>>>>>>>> Sorry.
>>>>>>>>
>>>>>>>> David
>>>>>>>> -----
>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Nils Loodin
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 10/11/2012 01:21 PM, Dmitry Samersoff wrote:
>>>>>>>>>> Keith,
>>>>>>>>>>
>>>>>>>>>> Personally, I would prefer explicit AllocWithoutThrow(...)
>>>>>>>>>> function.
>>>>>>>>>>
>>>>>>>>>> -Dmitry
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 2012-10-11 03:40, Keith McGuigan wrote:
>>>>>>>>>>> Personally, I don't have strong feelings on how this is
>>>>>>>>>>> implemented,
>>>>>>>>>>> other than it should be done in a way that's maintainable going
>>>>>>>>>>> forward
>>>>>>>>>>> and easily understandable by future generations of hotspot
>>>>>>>>>>> developers.
>>>>>>>>>>> With this in mind, the only potential solution that I don't
>>>>>>>>>>> like is
>>>>>>>>>>> using a boolean with naked true/false values as discriminators.
>>>>>>>>>>>
>>>>>>>>>>> Using some sort of "failure mode" parameter is the natural
>>>>>>>>>>> way to do
>>>>>>>>>>> this, whether it be enums, std::nothrow_t, or whatever. Since
>>>>>>>>>>> std::nothrow_t already has a type and one value, and is already
>>>>>>>>>>> present
>>>>>>>>>>> in the few places we're interested in, it seemed easy to
>>>>>>>>>>> simple just
>>>>>>>>>>> add
>>>>>>>>>>> a new value to use. However if this ends up being confusing
>>>>>>>>>>> because
>>>>>>>>>>> this is not the normal use of std::notype_t, then fine, we
>>>>>>>>>>> can do
>>>>>>>>>>> something else.
>>>>>>>>>>>
>>>>>>>>>>> --
>>>>>>>>>>> - Keith
>>