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

Sun Oct 14 23:49:14 PDT 2012

On 15/10/2012 4:29 PM, Nils Loodin wrote:
>
> On Oct 15, 2012, at 01:10 , 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.
>>
> Remember that the point of not having a boolean wasn't to enable a potential third option, but to make callsites clearer.
> Alloc(size, false) is less descriptive than Alloc(size, no_throw).

Okay, but a bool constant has the same properties (and is used elsewhere 
in the VM).

David
------

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