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

Mon Oct 15 04:40:41 PDT 2012

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