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

Tue Oct 16 06:08:08 PDT 2012

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