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

Tue Oct 16 06:22:54 PDT 2012

For the record, here is webrev.03 dug out from the thread.

http://cr.openjdk.java.net/~nloodin/8000617/webrev.03/

thanks,
Coleen

On 10/16/2012 9:08 AM, Coleen Phillmore wrote:
>
> Hi, I still disagree.
>
> On 10/15/2012 8:01 AM, David Holmes wrote:
>> 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.
>
> That's why it's overloaded.   The std::nothrow variant does exactly 
> what the one would expect from that variation, on the other hand.
>>
>> 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.
>
> Enforcing this with the proper overloading and in the code beats 
> having attentive human code reviewers!   If we follow the standard, 
> then new developers or openjdk developers already understand at least 
> this part of the code because they know C++.
>
>>
>> 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.
>>
>
> Actually to throw OOM, you'd change the throw version of the operator 
> new, not the nothrow version.   The nothrow version should be just that.
>
> My main concern is unintended consequences of hiding or overloading to 
> the wrong operator new.  There's some code in allocation.cpp that 
> tries to catch code that unintentionally uses the global operator new, 
> which is disabled.    Part of adding the nonstandard overloading 
> operator new should be enabling this code with the nothrow version.   
> I think this is more additional work.
>
> I think Nils version _03 of this change is really good and they need 
> it for JFR now.   I don't think we should hold it hostage to this 
> disagreement on this issue.  I also don't have time to read the rest 
> of Scott Meyer's books right now to use his arguments why this is 
> dangerous.
>
> Thanks,
> Coleen
>> 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
>>>>>>>
>>>>>