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

Mon Oct 15 05:01:22 PDT 2012

On 15/10/2012 9:40 PM, Coleen Phillmore wrote:
>
> Nils,
> I think you already know my opinion, which hasn't changed.
> In webrev #3, if you revert one of the NMT changes, does it still
> compile? Which "new" does it call?
> If you add a new class and inherit from ResourceObj and like a good C++
> programmer define a new with and without std::nothrow, which new does
> that call? You might not like the name std::nothrow but it's what the
> C++ committee picked.
> Scott Meyers "Effective C++" p27 Memory Management Item 9 "Avoid hiding
> the global new". This was written before nothrow though and was a
> guideline for other reasons. I haven't gotten to the modern version of
> this book but I assume the advice is the same.
> I think the enum is great for documentation for the functions that are
> Hotspot specific. For functions in the c++ standard that we overload,
> they should have the correct signature if the meaning is the same, ie
> std::nothrow.

That's a hard argument to make when the "standard" operator new is 
supposed to throw an exception and ours don't.

Plus you can't just add subclasses with their own operator new without 
fully understanding what the hotspot memory allocation strategy is - the 
whole thing is completely customized. So arguments for "standard" are 
somewhat tenuous in my opinion.

But, as I said in my PS email, if all this was rewritten we'd be passing 
null back for OOM, so adding all this additional policy selection seems 
to be a step in the wrong direction. A minimal change to ResourceObj 
along the lines of what exists (std::nothrow) for CHeapObj seems the 
path of least resistance to me.

David

> Thanks,
> Coleen
>
> On 10/15/2012 2:49 AM, David Holmes wrote:
>> 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
>>>>>
>>>