RFR: 8208686: [AOT] JVMTI ResourceExhausted event repeated for same allocation

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Fri Oct 5 19:56:12 UTC 2018


+1

Thanks,
Serguei

On 10/5/18 9:05 AM, Vladimir Kozlov 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.simon at 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.spitsyn at 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.kozlov at 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 at ... 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
>>>>
>>>
>>



More information about the hotspot-gc-dev mailing list