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

Wed Oct 17 03:50:41 PDT 2012

On 17/10/2012 6:51 PM, Nils Loodin wrote:
>> 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 !)
>>
>
> You're wrong, and here's why!
> (I'm sorry, it's really subjective, but it sounded so dramatic!)
>
> I think the point of the type safety of enums is a good one here. It
> will keep people from passing in other booleans by mistake.

Fine. I think it is overkill. :)

> Regarding the name, I think it's clearer with the purpose of the class
> that it's something to actaully do with a strategy on allocation failure
> in the current name. AllocFail might be a class with all kinds of
> responsibilities :)
>
> Also ponder the actual consequences of the additional eight characters.
> It's really not that hard to break lines to fit 80 characters, so the
> upside clearly beats the downside...
>
> Still open for suggestions though!

Well the important characters are the EXIT_OOM and RETURN_NULL, so when 
the prefix is longer than the real information I consider that 
unnecessary noise. Even no_throw is just std::no_throw not 
AllocationFailureStrategy::no_throw ;-)

> Any references to 'throw_constant' instead of 'nothrow_constant' is a
> typo and can safely be disregarded!
>
> About direct os::malloc vs a call to AllocateHeap instead:
> AllocateHeap contains code for tracing os:malloc. I assumed (and am of
> the oppinion that) the reason there was a 'naked' call to os::malloc
> without any tracing was there wasn't any support in the infrastructure
> to accomodate this before. IE, an outage.

Ummm there is tracing of the malloc calls:

  94     void* p = os::malloc(size, F, (caller_pc != 0 ? caller_pc : 
CALLER_PC));

  95     if (PrintMallocFree) trace_heap_malloc(size, "CHeapObj-new", p);

> In its place now, is another call to a function that also calls
> os::malloc, with tracking. Also, the 'chain' of allocation-calls now
> ends at the same place, which is tidier and easier to maintain.

Well now you end up with two traces per malloc call - but we already had 
that for some of the alloc paths anyway. Though if every path ends in 
AllocateHeap there's no really no reason to add AllocateHeap to the 
trace output.

David
-----

>
>
> Regards,
> Nils Loodin
>
>> 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
>>>>>>>>>
>>>>>>>
>