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

David Holmes david.holmes at oracle.com
Tue Oct 16 19:18:28 PDT 2012 

On 16/10/2012 11:08 PM, Coleen Phillmore wrote:
>
> Hi, I still disagree.

That's okay, this is completely subjective :)

> On 10/15/2012 8:01 AM, David Holmes wrote:
<snip>
>>
>> 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.

Note I didn't say anything about throwing OOM (OOME really), just 
reporting OOM. Given we don't use C++ exceptions we would have to return 
NULL to indicate an OOM condition.

> 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 don't quite follow that as we already have various custom overloads of 
operator new, but lets not dwell on this part ...

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

I don't think _03 is inconsistent with my "path of least resistance". :) 
Notwithstanding my preference to not use an Enum (and greater preference 
to use something much shorter named - eg AllocFail rather than 
AllocFailStrategy !)

src/share/vm/memory/allocation.inline.hpp

line 94: throw_constant should be nothrow_constant

But is there any real need to change this to use the updated 
AllocateHeap methods instead of continuing to use os::malloc?

---

src/share/vm/runtime/thread.cpp

   void* Thread::allocate(size_t size, bool throw_excpt, MEMFLAGS flags) {
     if (UseBiasedLocking) {
       const int alignment = markOopDesc::biased_lock_alignment;
       size_t aligned_size = size + (alignment - sizeof(intptr_t));
       void* real_malloc_addr = throw_excpt? AllocateHeap(aligned_size, 
flags, CURRENT_PC)
!                                           : AllocateHeap(aligned_size, 
flags, CURRENT_PC,
! 
AllocFailStrategy::RETURN_NULL);

throw_excpt should be renamed "exit_on_oom".

The allocation logic is simpler, in my opinion, if we simply select the 
OOM policy eg:

void* real_malloc_addr = AllocateHeap(aligned_size, flags, CURRENT_PC,
    (exit_on_oom ? AllocFailStrategy::EXIT_OOM : 
AllocFailStrategy::RETURN_NULL)

Thanks,
David
-------

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

More information about the hotspot-dev mailing list