Thanks for re-reviewing.
-Doug
> On 5 Oct 2018, at 18:05, Vladimir Kozlov <vladimir.koz...@oracle.com> wrote:
>
> Looks good.
>
> Thanks,
> Vladimir
>
> On 10/5/18 8:22 AM, Doug Simon wrote:
>> Hi,
>> Testing found a bug in the original webrev. Namely, when clearing out a
>> pending exception and returning null in the JVMCI ..._or_null stubs, the
>> JavaThread::_vm_result field was not being set to NULL. I've addressed this
>> in v2 of the webrev:
>> Relative diff for bug fix:
>> -----------------------------------------------------------------------------------------------
>> -// Manages a scope in which a failed heap allocation will throw an
>> exception.
>> -// The pending exception is cleared when leaving the scope.
>> +// Manages a scope for a JVMCI runtime call that attempts a heap allocation.
>> +// If there is a pending exception upon closing the scope and the runtime
>> +// call is of the variety where allocation failure returns NULL without an
>> +// exception, the following action is taken:
>> +// 1. The pending exception is cleared
>> +// 2. NULL is written to JavaThread::_vm_result
>> +// 3. Checks that an OutOfMemoryError is
>> Universe::out_of_memory_error_retry().
>> class RetryableAllocationMark: public StackObj {
>> private:
>> JavaThread* _thread;
>> public:
>> RetryableAllocationMark(JavaThread* thread, bool activate) {
>> if (activate) {
>> - assert(thread->in_retryable_allocation(), "retryable allocation scope
>> is non-reentrant");
>> + assert(!thread->in_retryable_allocation(), "retryable allocation
>> scope is non-reentrant");
>> _thread = thread;
>> _thread->set_in_retryable_allocation(true);
>> } else {
>> @@ -136,6 +141,7 @@
>> ResourceMark rm;
>> fatal("Unexpected exception in scope of retryable allocation: "
>> INTPTR_FORMAT " of type %s", p2i(ex), ex->klass()->external_name());
>> }
>> + _thread->set_vm_result(NULL);
>> }
>> }
>> }
>> -----------------------------------------------------------------------------------------------
>> I also took the opportunity to factor out negative array length checking:
>> -----------------------------------------------------------------------------------------------
>> diff -r 4d36f5998a8b src/hotspot/share/oops/arrayKlass.cpp
>> --- a/src/hotspot/share/oops/arrayKlass.cpp Fri Oct 05 17:04:06 2018 +0200
>> +++ b/src/hotspot/share/oops/arrayKlass.cpp Fri Oct 05 17:17:17 2018 +0200
>> @@ -130,9 +130,6 @@
>> }
>> objArrayOop ArrayKlass::allocate_arrayArray(int n, int length, TRAPS) {
>> - if (length < 0) {
>> - THROW_MSG_0(vmSymbols::java_lang_NegativeArraySizeException(),
>> err_msg("%d", length));
>> - }
>> check_array_allocation_length(length,
>> arrayOopDesc::max_array_length(T_ARRAY), CHECK_0);
>> int size = objArrayOopDesc::object_size(length);
>> Klass* k = array_klass(n+dimension(), CHECK_0);
>> diff -r 4d36f5998a8b src/hotspot/share/oops/instanceKlass.cpp
>> --- a/src/hotspot/share/oops/instanceKlass.cpp Fri Oct 05 17:04:06
>> 2018 +0200
>> +++ b/src/hotspot/share/oops/instanceKlass.cpp Fri Oct 05 17:17:17
>> 2018 +0200
>> @@ -1201,9 +1201,6 @@
>> }
>> objArrayOop InstanceKlass::allocate_objArray(int n, int length, TRAPS) {
>> - if (length < 0) {
>> - THROW_MSG_0(vmSymbols::java_lang_NegativeArraySizeException(),
>> err_msg("%d", length));
>> - }
>> check_array_allocation_length(length,
>> arrayOopDesc::max_array_length(T_OBJECT), CHECK_NULL);
>> int size = objArrayOopDesc::object_size(length);
>> Klass* ak = array_klass(n, CHECK_NULL);
>> diff -r 4d36f5998a8b src/hotspot/share/oops/klass.cpp
>> --- a/src/hotspot/share/oops/klass.cpp Fri Oct 05 17:04:06 2018 +0200
>> +++ b/src/hotspot/share/oops/klass.cpp Fri Oct 05 17:17:17 2018 +0200
>> @@ -620,6 +620,8 @@
>> } else {
>> THROW_OOP(Universe::out_of_memory_error_retry());
>> }
>> + } else if (length < 0) {
>> + THROW_MSG(vmSymbols::java_lang_NegativeArraySizeException(),
>> err_msg("%d", length));
>> }
>> }
>> diff -r 4d36f5998a8b src/hotspot/share/oops/klass.hpp
>> --- a/src/hotspot/share/oops/klass.hpp Fri Oct 05 17:04:06 2018 +0200
>> +++ b/src/hotspot/share/oops/klass.hpp Fri Oct 05 17:17:17 2018 +0200
>> @@ -514,7 +514,7 @@
>> virtual Klass* array_klass_impl(bool or_null, int rank, TRAPS);
>> virtual Klass* array_klass_impl(bool or_null, TRAPS);
>> - // Error handling when length > max_length
>> + // Error handling when length > max_length or length < 0
>> static void check_array_allocation_length(int length, int max_length,
>> TRAPS);
>> void set_vtable_length(int len) { _vtable_len= len; }
>> diff -r 4d36f5998a8b src/hotspot/share/oops/objArrayKlass.cpp
>> --- a/src/hotspot/share/oops/objArrayKlass.cpp Fri Oct 05 17:04:06
>> 2018 +0200
>> +++ b/src/hotspot/share/oops/objArrayKlass.cpp Fri Oct 05 17:17:17
>> 2018 +0200
>> @@ -170,14 +170,10 @@
>> }
>> objArrayOop ObjArrayKlass::allocate(int length, TRAPS) {
>> - if (length >= 0) {
>> - check_array_allocation_length(length,
>> arrayOopDesc::max_array_length(T_OBJECT), CHECK_0);
>> - int size = objArrayOopDesc::object_size(length);
>> - return (objArrayOop)Universe::heap()->array_allocate(this, size, length,
>> - /* do_zero */
>> true, THREAD);
>> - } else {
>> - THROW_MSG_0(vmSymbols::java_lang_NegativeArraySizeException(),
>> err_msg("%d", length));
>> - }
>> + check_array_allocation_length(length,
>> arrayOopDesc::max_array_length(T_OBJECT), CHECK_0);
>> + int size = objArrayOopDesc::object_size(length);
>> + return (objArrayOop)Universe::heap()->array_allocate(this, size, length,
>> + /* do_zero */ true,
>> THREAD);
>> }
>> static int multi_alloc_counter = 0;
>> diff -r 4d36f5998a8b src/hotspot/share/oops/typeArrayKlass.cpp
>> --- a/src/hotspot/share/oops/typeArrayKlass.cpp Fri Oct 05 17:04:06
>> 2018 +0200
>> +++ b/src/hotspot/share/oops/typeArrayKlass.cpp Fri Oct 05 17:17:17
>> 2018 +0200
>> @@ -99,14 +99,10 @@
>> typeArrayOop TypeArrayKlass::allocate_common(int length, bool do_zero,
>> TRAPS) {
>> assert(log2_element_size() >= 0, "bad scale");
>> - if (length >= 0) {
>> - check_array_allocation_length(length, max_length(), CHECK_NULL);
>> - size_t size = typeArrayOopDesc::object_size(layout_helper(), length);
>> - return (typeArrayOop)Universe::heap()->array_allocate(this, (int)size,
>> length,
>> - do_zero,
>> CHECK_NULL);
>> - } else {
>> - THROW_MSG_0(vmSymbols::java_lang_NegativeArraySizeException(),
>> err_msg("%d", length));
>> - }
>> + check_array_allocation_length(length, max_length(), CHECK_NULL);
>> + size_t size = typeArrayOopDesc::object_size(layout_helper(), length);
>> + return (typeArrayOop)Universe::heap()->array_allocate(this, (int)size,
>> length,
>> + do_zero,
>> CHECK_NULL);
>> }
>> oop TypeArrayKlass::multi_allocate(int rank, jint* last_size, TRAPS) {
>> -----------------------------------------------------------------------------------------------
>> Please confirm review these new changes:
>> http://cr.openjdk.java.net/~dnsimon/8208686v2
>> -Doug
>>> On 4 Oct 2018, at 00:20, Doug Simon <doug.si...@oracle.com> wrote:
>>>
>>> Thanks for the reviews Serguei and Vladimir.
>>>
>>> Unless I hear objections in the next 24 hours, I'll push this webrev.
>>>
>>> -Doug
>>>
>>>> On 3 Oct 2018, at 03:14, serguei.spit...@oracle.com wrote:
>>>>
>>>> Hi Doug,
>>>>
>>>> The JVMTI related part looks good to me.
>>>> Thank you for fixing it!
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>> On 10/2/18 1:11 AM, Doug Simon wrote:
>>>>> It would be great to get some input from the non-compilers teams on this
>>>>> RFR.
>>>>>
>>>>> -Doug
>>>>>
>>>>>> On 28 Sep 2018, at 19:51, Vladimir Kozlov <vladimir.koz...@oracle.com>
>>>>>> wrote:
>>>>>>
>>>>>> To let you know, me and Tom R. did review these changes and agreed that
>>>>>> it is the least intrusive changes for Hotspot shared code.
>>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>> On 9/25/18 8:11 AM, Daniel D. Daugherty wrote:
>>>>>>> Adding serviceability-dev@... since this is JVM/TI...
>>>>>>> Dan
>>>>>>> On 9/25/18 10:48 AM, Doug Simon wrote:
>>>>>>>> A major design point of Graal is to treat allocations as non-side
>>>>>>>> effecting to give more freedom to the optimizer by reducing the number
>>>>>>>> of distinct FrameStates that need to be managed. When failing an
>>>>>>>> allocation, Graal will deoptimize to the last side effecting
>>>>>>>> instruction before the allocation. This mean the VM code for heap
>>>>>>>> allocation will potentially be executed twice, once from Graal
>>>>>>>> compiled code and then again in the interpreter. While this is
>>>>>>>> perfectly fine according to the JVM specification, it can cause
>>>>>>>> confusing behavior for JVMTI based tools. They will receive 2
>>>>>>>> ResourceExhausted events for a single allocation. Furthermore, the
>>>>>>>> first ResourceExhausted event (on the Graal allocation slow path)
>>>>>>>> might denote a bytecode instruction that performs no allocation,
>>>>>>>> making it hard to debug the memory failure.
>>>>>>>>
>>>>>>>> The proposed solution is to add an extra set of JVMCI VM runtime calls
>>>>>>>> for allocation. These entry points will attempt the allocation and
>>>>>>>> upon failure,
>>>>>>>> skip side-effects such as posting JVMTI events or handling
>>>>>>>> -XX:OnOutOfMemoryError. The compiled code using these entry points is
>>>>>>>> expected deoptmize on null.
>>>>>>>>
>>>>>>>> The path from these new entry points to where allocation can fail goes
>>>>>>>> through quite a bit of VM code. One could modify all these paths by:
>>>>>>>> * Returning null instead of throwing an exception on failure.
>>>>>>>> * Adding a `bool null_on_fail` argument to all relevant methods.
>>>>>>>> * Adding extra null checking where necessary after each call to these
>>>>>>>> methods when `null_on_fail == true`.
>>>>>>>> This represents a significant number of changes.
>>>>>>>>
>>>>>>>> Instead, the proposed solution introduces a new
>>>>>>>> _in_retryable_allocation thread-local. This way, only the entry points
>>>>>>>> and allocation routines that raise an exception need to be modified.
>>>>>>>> Failure is communicated back to the new entry points by throwing a
>>>>>>>> special pre-allocated OOME object (i.e.,
>>>>>>>> Universe::out_of_memory_error_retry()) which must not propagate back
>>>>>>>> to Java code. Use of this object is not strictly necessary; it is
>>>>>>>> introduced to highlight/document the special allocation mode.
>>>>>>>>
>>>>>>>> The proposed solution is at
>>>>>>>> http://cr.openjdk.java.net/~dnsimon/8208686.
>>>>>>>> THE JBS bug is: https://bugs.openjdk.java.net/browse/JDK-8208686
>>>>>>>>
>>>>>>>> -Doug
>>>>
>>>
